fix(tesseract): fold nested multi-fact join legs into one scan - #11643

Todo

• Gather context / read diff (git diff 6f3e66d..HEAD)
• Review multi_fact_join_groups.rs merge logic
• Trace the safety argument end-to-end (is_multiplied → multiplicationFactor → render form)
• Review guards (query_properties, query_tools, measure_kinds)
• Review tests, fixtures, seed data and snapshot values
• Post inline comments + summary

What I verified

I traced the safety argument rather than taking the PR description at face value, and it holds:

• Every cube join is LEFT — physical_plan_builder/processors/logical_join.rs:67 and :106 only ever call left_join_cube / left_join_source. So the "extra joins replicate, never drop" premise is enforced by construction, not by convention.
• The render form really is decided from the measure's own tree, not the merged one: full_key_aggregate_measures (query_properties.rs:601-604) resolves the join per single measure via compute_join_multi_fact_groups_with_measures, which routes through for_measures → build(..., merge_nested = false). This is exactly why survives_row_multiplication must be narrower than regular_in_multiplied, and the code gets that right — a Count that would need switching to MultipliedCount is correctly refused.
• The multiplicationFactor gap is genuinely closed. JoinTree::is_multiplied falls back to false for a cube absent from the map (join_tree.rs:87-92), which is the trap the PR describes. Rebuilding from the union of hints puts the moving measure's cube into cubesToJoin, and the key != other.key bail means a rebuild that produced different joins is skipped rather than trusted. Since the factor per cube is derived from the join edges and the edges are asserted identical, the rebuild only adds entries — it cannot flip an existing one, so the target group's own measures are unaffected.
• Chained merges stay honest: hints/measures are both accumulated into the target, so a group that already absorbed another is re-validated with the full measure set on the next pass, and its absorbed hints are part of every subsequent rebuild.
• Termination: each pass removes exactly one group, so the loop is bounded by group count. Iteration is over key_order, so merge selection is deterministic.
• Member expressions stand down via item.measure.as_measure() failing — collect_multiplied_measures returns the expression symbol itself for the dimension-only case (multiplied_measures_collector.rs:124-162) and through the visitor otherwise. Covered by test_nested_trees_count_star_expression_keeps_its_own_scan.
• Cache safety: JoinTreeCache is keyed by JoinHints, so the speculative rebuilds add entries under their own union key and cannot poison the entry for either original group's hints.
• Snapshot values check out by hand against the seed: US carts {m1, m1, m2} → 2 distinct; US checkouts {x1, x2} (cart 1) ∪ {x1} (cart 3) → 2; DE cart 4 → 1 cart, 0 checkouts, which is the LEFT JOIN-preserves-the-row case. The seed is deliberately built so distinct ≠ count and so fan-out and no-fan-out both occur — good test design.

Findings

Security: nothing relevant — no new input paths, no user-controlled SQL, no auth surface. The added SQL text is entirely planner-generated from already-validated members.

Praise

The doc comments are the strongest part of this PR — merge_nested_groups and group_survives_join explain why each condition exists, including the non-obvious multiplicationFactor-over-cubesToJoin trap, and survives_row_multiplication explicitly names its relationship to the neighbouring regular_in_multiplied so the next reader doesn't "simplify" one into the other. The key != other.key assertion on the rebuilt tree is the right instinct: it converts an assumption into a checked invariant with a safe fallback. The PR description's honesty about the coarse pre-aggregation guard and the is_simple_query()-only reach is also appreciated.

Notes on scope decisions I agree with

• Not merging in for_measures is correct — regrouping a subset after the plan exists would cost rollups.
• Keeping the star-shaped multi-fact case split is right and clearly explained; test_sibling_trees_do_not_merge pins it.
• Deferring the two-pass TopLevelPlanner work is the right call for this PR's size.

@@             Coverage Diff             @@
##           master   #11643       +/-   ##
===========================================
+ Coverage   59.36%   84.42%   +25.05%     
===========================================
  Files         229      260       +31     
  Lines       18241    84473    +66232     
  Branches     3679        0     -3679     
===========================================
+ Hits        10829    71313    +60484     
- Misses       6861    13160     +6299     
+ Partials      551        0      -551     

Flags with carried forward coverage won't be shown. Click here to find out more.

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Todo

• Read new commit 902fa44 diff (git show 902fa44)
• Verify findings 1 (probe error) & 2 (pre-agg gating) are actually fixed
• Check the new tests, snapshot values against the seed
• Re-check remaining findings (3, 4, 5) against the new diff
• Resolve stale review threads
• Post updated summary

What changed since my last review

902fa44 addresses three of the five findings.

Finding 1 — probe error propagation: fixed (multi_fact_join_groups.rs:426). resolve(&hints)? is now let Ok((key, tree)) = resolve(&hints) else { continue }, with a comment stating the reason. This closes the only path by which the optimization could turn a query that plans on master into a failing one. The remaining ? in group_survives_join is has_multi_stage_members, which walks the measure symbol independently of the candidate tree — not a probe, so propagating is right there.

Finding 2 — guard ordering: fixed, and with the better fix. The new has_nested_pair (:344) is a pure in-memory is_nested_in scan and runs before any_cube_has_pre_aggregations, so the sibling multi-fact case (orders/returns) also skips the bridge calls — not just the single-group case my groups.len() > 1 suggestion would have covered. is_nested_in requires strictly fewer joins, so the self-pair can't false-positive and no i != j guard is needed.

Finding 4 — doc comment: fixed (query_tools.rs:26-31). Now says "containment of the edge set, not of a path" and explicitly notes other may extend along a sibling branch, with the reason that is still safe.

Finding 5 — the two coverage gaps are closed. test_nested_trees_merge_with_filter_on_shallower_cube, test_nested_trees_filter_on_deeper_cube_leaves_one_tree, and test_nested_trees_pre_aggregation_query_keeps_separate_scans land exactly the cases I flagged. The base_scan_count alias-coupling half of that comment is unaddressed — left unresolved as a nit, not a request.

New snapshots verified by hand

Both re-derived from the seed rather than trusted:

• filter carts.msid = 'm1' → carts 1, 2 (both site 1, US). carts.unique_msid = |{m1}| = 1 ✓. Their checkouts are 1 (x1) and 2 (x2) → 2 ✓. Site 2 (cart 3, m2) and site 3 (cart 4, m3) are filtered out, so a single US row ✓. Note this is the interesting direction: the filtered cube is the shallower one, the trees stay nested, and the merge still fires (base_scan_count == 1) — while count-style measures would still have split.
• filter checkouts.msid = 'x1' → checkouts 1 (cart 1, site 1) and 3 (cart 3, site 2), both US. carts distinct = |{m1, m2}| = 2 ✓, checkouts distinct = |{x1}| = 1 ✓. base_scan_count == 1 here is not a merge — the filter pulls checkouts into the base hints so both measures resolve to one tree from the start. The doc comment on the test says precisely that, which is worth keeping; a future reader could easily misread the 1 as evidence the merge fired.
• pre_aggregation_query: true → asserts 2, pinning the guard in query_properties.rs. Good choice to make this a plain #[test] (no PG needed) since only the plan shape matters.

Remaining findings

Findings 1, 2 and 4 resolved on their threads.

Verification I could not run

cargo check / cargo test are not in this job's allowlist, so I reviewed the new code by reading it. The changes are small and locally type-obvious (an if let ... else { continue } inside the labeled loop; a &[GroupBuild] → bool helper whose immutable borrow ends before merge_nested_groups takes &mut), but CI is the authority on that. The PR states 1287 tests green on real Postgres at b2ddcb2; the three new tests here are additive and the two new snapshots are the only new expected output.

Verdict

The two things I'd have wanted changed before merge are changed, and the fix for finding 2 is better than what I proposed. The safety argument I traced at b2ddcb2 is untouched by this commit — same LEFT-join premise, same per-measure render-form resolution, same key != other.key rebuild assertion. Ship-ready from my side; the two low items are follow-up material.

Todo

• Read the new commit (git diff 902fa44..HEAD)
• Verify the min/max safety argument end-to-end
• Check is_duplicate_insensitive against the full AggregationType enum
• Re-derive the three new snapshots from the seed by hand
• Re-check the linchpin assumption (rebuild can't flip an existing multiplicationFactor)
• Resolve the now-addressed thread, post summary

What changed since 902fa44

9b9d000 implements finding 3 — the last open non-nit item — and does it better than my suggestion did.

Rather than inlining a matches! over four variants at the MeasureKind level, it introduces AggregationType::is_duplicate_insensitive() (aggregation_type.rs:54) and has survives_row_multiplication call it. That's the right home for the property: it's a fact about the aggregation function, not about the measure kind, and it now sits next to is_additive/is_distinct where the next reader will find it. The doc comment explicitly contrasts it with is_additive — "sum is additive but sensitive to repeats, countDistinct is insensitive to them but not additive" — which pre-empts exactly the conflation someone would otherwise make. test_is_duplicate_insensitive pins all seven enum variants, so a new aggregation type can't silently default into the safe set.

The doc on survives_row_multiplication was also corrected, and the correction is substantive rather than cosmetic: it no longer claims to be "strictly narrower" than regular_in_multiplied (which is now false in both directions) and names the asymmetry in each direction.

Safety verified, not assumed

MIN/MAX are genuinely immune here, for the right reason. Two things both have to hold, and both do:

1. The value doesn't move under replication. The merge only ever adds LEFT joins, so rows are replicated, never added or dropped. MIN/MAX over a multiset are unchanged by repeating an element. Note this argument needs the "never added" half too — a RIGHT/FULL join could introduce a NULL-extended row on the measure's own cube and would move a MIN. logical_join.rs only ever emits left_join_cube/left_join_source, so that's structural.
2. The rendered SQL is identical either way. into_multiplied leaves Aggregated(_) untouched (measure_kinds/mod.rs:268), so there is no "switch to the distinct form" step that a Count needs and that the render-form resolution — done per measure against its own tree — would not know to perform. This is exactly why Count must stay refused while Min/Max may pass, and the code draws the line in the right place.

The fixture exercises the interesting direction. sites --one_to_many--> carts means carts is not multiplied in the narrow sites→carts tree, so carts.max_value classifies as a regular measure; adding checkouts makes carts multiplied in the merged tree. So the test genuinely covers "measure rendered by the narrow tree's classification, executed over the wider tree" — the case the predicate exists for, not a vacuous one.

Re-confirmed the linchpin. JoinGraph.ts:205 computes findMultiplicationFactorFor(cube, join.joins) — a function of the join edges only, with cubesToJoin deciding merely which cubes get an entry. Since the rebuild asserts key == other.key (same root, same edges), it can only add entries and provably cannot flip one, so the target group's own measures keep their classification. That closes the one way this could have gone wrong quietly.

New snapshots re-derived from the seed

All three checked by hand rather than accepted:

• min_max_share_one_base_scan — US = sites 1+2 → carts 1 (10), 2 (20), 3 (30): max 30, min 10 ✓. DE = site 3 → cart 4 (40): 40/40 ✓. checkouts.unique_msid: US = {x1, x2} ∪ {x1} = 2 ✓, DE = 0 ✓ (cart 4 has no checkouts — the LEFT JOIN-preserves-the-row case).
• min_max_alone — identical min/max columns to the merged query. This is the assertion that matters: the numbers are byte-identical with and without the fan-out present, which is the whole claim. ✓
• avg_keeps_its_own_scan — US = avg(10,20,30) = 20, DE = 40 ✓, with base_scan_count == 2. Good negative control: avg is the one addition to the fixture that must not merge, and if is_duplicate_insensitive ever wrongly admitted it, both the scan count and (given cart 1's two checkouts) the US value would change — US would become avg(10,10,20,30) = 17.5. The seed makes the wrong answer visibly wrong, which is the point of a negative test.

Findings

Nothing new. Status of the five findings across the three review passes:

Follow-up worth considering (not for this PR)

regular_in_multiplied still turns down a multiplied Min/Max, sending it to a multiplied subquery with its own keys CTE. By the same argument this commit just made, that subquery is unnecessary — MIN/MAX over a fanned-out join equals the value over the un-fanned one, with no form switch. Widening regular_in_multiplied the same way would remove a whole CTE from a common class of query, but it would move many existing snapshots and deserves its own PR. The new doc comment already flags the asymmetry, which is the right amount of signal to leave behind.

Verification I could not run

cargo check / cargo test are outside this job's allowlist, so the new code was reviewed by reading. It is small and locally type-obvious (one new matches! predicate, one call-site swap, three additive tests, three new snapshot files), and CI is the authority. The two fixture YAMLs gained the same three measures, so no existing test in either file changes shape.

Verdict

Ship-ready. The commit closes the last substantive item, the property it relies on is now named and unit-pinned rather than implied, and the tests include a negative control that would actually fail if the predicate were wrong. Only the alias-coupled base_scan_count nit remains open, and that is follow-up material at most.