fix(tesseract): keep query-level join hints while matching pre-aggregations

[waralexrom]

Problem

A raw multi-cube query that carries explicit joinHints — the shape the SQL API sends whenever
the queried members alone don't determine a join root — fails as soon as any cube declares a
cross-cube pre-aggregation:

Can't find join path to join 'users', 'line_items'

The query is fully specified: its hints name base_orders as the root, and base_orders is the
only cube that joins both users and line_items. Removing the pre-aggregation makes the same
query plan fine, so the failure comes from a rollup that cannot even serve the query.

The legacy planner does not reproduce this — matching simply finds no rollup and moves on.

Cause

PreAggregationOptimizer re-derives the query's join groups so it can compare join paths against
a candidate rollup (query_join_groups, and the multiplied-measure check next to it). Both seeded
MeasuresJoinHints::builder with an empty JoinHints, so the hints the query was planned with
were dropped and the hint set was rebuilt from members only. For a query whose members don't
determine a root, that set has no resolvable root and JoinGraph.buildJoin throws instead of the
candidate simply being rejected.

What was done

• PreAggregationOptimizer now takes the request's join hints and seeds both query-side hint
  builders with them, so matching derives the query's join groups exactly the way the query
  planner does.
• The hints of a pre-aggregation itself are unchanged: they still come from its own members.

The change is a no-op for queries that carry no joinHints — the seed is empty and every hint set
comes out byte-identical, so view queries (which resolve the same ambiguity through their own
join_path) are unaffected.

How it was verified

• New Rust fixture + tests in tests/integration/pre_aggregations/query_join_hints.rs: the rollup
  serves the query; the rollup cannot serve the query and the plan stays identical to the one built
  with no pre-aggregation at all; no pre-aggregation declared; a view query still matches.
  Reverting the fix fails the first two with the reported error and leaves the controls green.
• packages/cubejs-schema-compiler/test/unit/pre-aggregations.test.ts runs the same shape on both
  planners. Before the fix: legacy passes, native throws Can't find join path to join 'users', 'line_items'. After: both pass.
• cargo test -p cubesqlplanner --lib — 1378 passed, 0 failed.
• jest dist/test/unit in cubejs-schema-compiler — 954 passed; the 2 failures are
  error-reporter ANSI-colour snapshots that also fail on a clean tree without a TTY.

Risks

Matching now compares against the join tree the query actually uses rather than one re-derived from
members alone. For a query with explicit joinHints whose hint-derived tree differed from the
member-derived one, a rollup that used to be accepted may now be rejected (or the reverse) — that is
the intended correction, but it is a behaviour change for such queries. Queries without joinHints
are untouched.

Not covered here: a rollup whose members are written without a join path still throws during its
own compilation, from a hint set seeded only by its dimensions. That is a separate defect on the
pre-aggregation side and is out of scope for this change.

🤖 Generated with Claude Code

[claude]

Claude finished @waralexrom's task in 3m 48s —— View job

---

Review of #11858 — the fix is right and correctly scoped; 1 medium, 2 low

Full review

• Gather context (PR diff, linked issue, prior threads)
• Review Rust optimizer change
• Review test fixtures & JS test
• Post findings

No prior review threads on this PR, nothing to resolve or dedupe against.

Verdict

The diagnosis holds up on reading the code. MeasuresJoinHints::builder is the single seam through which query-level hints enter join-group derivation (multi_fact_join_groups.rs:95), and PreAggregationOptimizer was the only query-side caller passing JoinHints::new() — query_properties.rs:317 and member_query_planner.rs:565 both pass the request's hints, so the optimizer was the outlier. Seeding it from request.query_join_hints() makes matching derive the same tree the planner will render, which is the invariant are_join_paths_matching silently assumed all along.

Worth noting explicitly, because the PR body only frames it as a risk: the old behaviour also produced false accepts, not just spurious errors. A rollup declared on users alone, matched against a query whose hints root it at base_orders, compared [users] vs [users] and matched — while the rendered query fans users out over line_items, so users.count would have been served from a rollup storing a different number. The narrower member-derived tree hid the multiplication from has_multiplied_measures() too. The fix closes that, which is the more valuable half of the change.

The pre-aggregation side is untouched (join_hints_from_pre_aggregation, pre_aggregations_compiler.rs:205 still seeds empty), which is correct — a rollup's hints are a property of the rollup, not of the query asking for it.

Verification note: cargo is not in the allowlist for this run, so I could not execute cargo test -p cubesqlplanner --lib myself. The review below is from reading the code; I did confirm the new test helpers exist (MockSchema::only_pre_aggregations at mock_schema.rs:59, TestContext::build_sql_with_used_pre_aggregations at test_context.rs:599), that the mock BaseQueryOptions honours joinHints (base_query_options.rs:203-209), and that query_join_hints() returns &Rc<JoinHints> so the added .clone() is an Rc bump, not a deep copy.

Findings

#	Sev	Where	What
1	Medium	optimizer.rs:658, :711 (thread on :862)	Candidate matching still propagates a join-resolution error and aborts the whole query, so the reported Can't find join path… remains reachable through a rollup whose own members don't resolve — the case the PR lists as out of scope. Same call chain, same user-visible bug, one seed over. merge_nested_groups already treats resolution as a probe; the candidate scan should too.
2	Low	pre-aggregations.test.ts:1266-1267	The JS test can't fail the way its describe title promises: the rollup is named orders_and_line_items_of_users, so toMatch(/line_items/) passes even if the rollup were wrongly matched. Only "does not throw" is pinned.
3	Low	optimizer.rs:857-867	query_join_groups is rebuilt from scratch for every candidate pre-aggregation (try_rewrite_query loops candidates → is_schema_and_filters_match), even though it depends only on the node's schema and measures. resolve is memoised by join_tree_cache, but collect_join_hints over every dimension/measure and precompute_paths are not. Pre-existing, and the comment at :656 ("build them once") reads as if it were once per node. A memo next to primary_keys_cache keyed on the node would make it so. Not a blocker.

Test coverage

The Rust fixture is well chosen: base_orders as sole connector, users joining nothing, so the hints genuinely are the only thing that resolves a root — reverting the fix has to fail. The four cases (rollup serves / rollup can't serve with plan-equality against a no-pre-aggregation baseline / no pre-aggregation declared / view query as the no-hints control) cover the axis properly, and the plan-equality assertion in test_unusable_rollup_does_not_change_the_plan is the strongest of them.

Gap: nothing covers a query carrying hints whose member-derived tree would have differed — i.e. the behaviour change called out under "Risks", including the false-accept case above. That is the one shape where this patch changes an answer rather than turning an error into an answer, and it is currently unpinned in either suite. A rollup on users alone plus a hinted query rooted at base_orders would pin it in a few lines of YAML on the existing fixture.

Nothing security-relevant here — no new input paths, hints are already-parsed request data.

· branch `tesseract-preagg-matching-query-join-hints`

[claude]

Seeding the query's own hints is the right fix for the reported shape, but the failure mode itself is still live: query_join_groups is called with ? from is_schema_and_filters_match (line 658), so any hint set the join graph refuses while probing a candidate still aborts the whole query instead of rejecting that candidate.

With this patch the query-side seed can no longer be the cause, but the same Can't find join path… can still come out of a rollup whose own members resolve to an unbuildable tree — the case the PR body lists under "Not covered here". That is the same user-visible bug from the same call chain, one seed over.

MultiFactJoinGroups::merge_nested_groups already draws the distinction explicitly:

// Resolving is a probe: a hint set the join graph refuses
// means there is no merge to make here, not that the query
// the groups came from is unplannable.
let Ok((key, tree)) = resolve(&hints) else { continue; };

Candidate matching is a probe in exactly the same sense. Swallowing a resolve failure here is safe: if the query's own hints genuinely don't resolve, the query planner raises it anyway on the non-pre-aggregated path — so the error is never lost, only stopped from being raised by an irrelevant rollup. Worth applying the same discipline to the candidate scan (is_schema_and_filters_match returning Ok(None) on a resolve error) so no rollup can take down a query it cannot serve.

Fix this →

[claude]

This test can't fail the way its describe title promises ("a rollup that cannot serve the query does not change how it is planned"). The rollup here is base_orders.orders_and_line_items_of_users, so its table name contains line_items — toMatch(/line_items/) passes whether the query reads the raw tables or (wrongly) reads the rollup. The only thing actually pinned is "does not throw".

Since the query is the uncovered shape (base_orders.count is multiplied over the line_items fan-out), assert that no pre-aggregation was picked, which is what makes it a plan-unchanged test:

Suggested change

	expect(() => query.buildSqlAndParams()).not.toThrow();
	expect(query.buildSqlAndParams()[0]).toMatch(/line_items/);
	expect(() => query.buildSqlAndParams()).not.toThrow();
	expect(query.preAggregations?.preAggregationForQuery).toBeFalsy();
	expect(query.buildSqlAndParams()[0]).toMatch(/FROM\s+line_items/);

Also, buildSqlAndParams() is called twice here; the second call alone surfaces a throw, so the first expect is redundant once a real assertion follows.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.34%. Comparing base (5019312) to head (d85c0a0).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11858      +/-   ##
==========================================
- Coverage   80.35%   80.34%   -0.01%     
==========================================
  Files         500      500              
  Lines      106555   106555              
  Branches     3881     3881              
==========================================
- Hits        85619    85615       -4     
- Misses      20386    20390       +4     
  Partials      550      550              

Flag	Coverage Δ
cube-backend	60.23% <ø> (ø)
cubesql	84.76% <ø> (-0.01%)	⬇️

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

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ 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.