fix(tesseract): plan multiplied measures per cube and join tree - #11436
fix(tesseract): plan multiplied measures per cube and join tree#11436waralexrom wants to merge 10 commits into
Conversation
Multiplied measures were bucketed by owning cube and the whole bucket was then required to resolve to a single multi-fact join group, so two measures of one cube needing different join trees were refused with "Expected just one multi-fact join group". The key cube fixes the primary keys to deduplicate on and the join tree fixes the joins to build; neither determines the other. Group by both and emit one AggregateMultipliedSubquery per pair, mirroring the regular-measure branch, which already emits one CTE per join group. Trees are kept apart rather than merged so that a measure's value does not depend on which other measures share the query: merging sibling one-to-many branches introduces a cross product that neither original tree had. With a single group per cube the plan is unchanged, so queries that planned before are unaffected.
Reaching a cube beyond its own makes a multiplied measure go through the measure-join subquery, which renders measures without their aggregate so the select above can re-aggregate them. A calculated measure has no aggregate of its own, so nothing was re-applied: its components lost theirs inside the subquery and the expression came out neither aggregated nor grouped. Mark such a measure composite instead, so its components travel in its place - each on the join tree its own definition asks for - and the expression is rebuilt above them. Only when the reach runs entirely through those components. A dimension dependency, a raw cube reference or no measure dependency at all leaves the measure with something to evaluate where it stands, and the select above has no cube joined in to evaluate it against; those keep going whole.
github-actions
Bot
added
rust
|
Claude finished @waralexrom's task in 5m 50s —— View job Review complete — 2 medium, 2 low, no blockers. Both fixes look sound; the main question is whether the calculated-measure decomposition should be gated on the measure actually being multiplied.Full review
What's good
Findings
Details are in the four inline comments. Checked and clear
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11436 +/- ##
==========================================
- Coverage 83.94% 79.43% -4.52%
==========================================
Files 257 480 +223
Lines 80887 98778 +17891
Branches 0 3636 +3636
==========================================
+ Hits 67904 78464 +10560
- Misses 12983 19796 +6813
- Partials 0 518 +518
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Decomposition exists to work around the measure-join subquery, which is built only for a measure that is multiplied by its join tree. Elsewhere the measure is read off a leaf-measure query that keeps its aggregate, and splitting it changes the answer: components can root at different cubes, so each divides by the rows its own leg sees rather than by the query's. Fail loudly instead of asserting when join grouping drops a measure of a bucket - a debug assertion leaves release builds emitting a plan with the measure missing.
|
Claude finished @waralexrom's task in 5m 19s —— View job Re-review of
|
| Prior finding | Status |
|---|---|
| Medium: composite gate was join-independent | Fixed. Gate now requires self.join.is_multiplied(&owned.cube_name()); join is threaded into CompositeMeasuresCollector from collect_multiplied_measures. |
Medium: debug_assert_eq! compiles away in release |
Fixed. Now a returned CubeError::internal naming the cube and both counts. |
Low: travels_only_through_measures vs reaches_other_cube read differently-stripped nodes |
Fixed. Both take &owned; reaches_other_cube no longer strips internally, so it's stripped once per node instead of twice. |
Low: #[ignore] reasons carry no tracking link; nothing pins plan shape without integration-postgres |
Partly. The new unit suite gives the decomposition gate feature-free coverage, which is the bigger half. Still no issue link on the two ignores and still nothing pins "one AggregateMultipliedSubquery per (cube, join tree)" without a database. Prior thread left unresolved. |
Verification of the narrowing
The gate is now exactly as tight as the defect. I traced the two paths:
- Classification and planning agree on the same join.
full_key_aggregate_measurescomputes the per-measure join (query_properties.rs:530-533) and hands it tocollect_multiplied_measures;MultipliedMeasuresCollectorsetsmultipliedfrom that samejoin.is_multiplied(cube)(multiplied_measures_collector.rs:140). Sois_multiplied == falsein the gate ⇒ the measure is classified regular ⇒ it never reachesaggregate_subquery_plan, hence never meets the ungrouped measure-join subquery. No case where the gate says "don't decompose" but the subquery is built anyway. owned.cube_name()is safe to compare against the unstripped collector's.CompiledMemberPath::strip_join_prefix(compiled_member_path.rs:61-69) rewrites onlypath, keepingcubeandfull_name, so stripping cannot shift which cubeis_multipliedis asked about, andget_dependencies()/get_cube_refs()are unaffected too — the two halves of the gate now genuinely judge the same symbol.- The new hard error is unreachable rather than newly-throwing. The only measure-dropping path into
build_groupsisMeasuresJoinHints::from_base_hints'has_multi_stage_members(m, true)filter (multi_fact_join_groups.rs:97-101); upstreamfull_key_aggregate_measuresfilters withhas_multi_stage_members(m, self.ignore_cumulative || self.pre_aggregation_query), which is a superset (flagfalsealso counts cumulative). So anything surviving upstream survives the filter, andbuild_groupsis otherwise a strict partition (key_order/grouped, lines 222-236). Duplicate-inflation ofmeasures.len()is also ruled out —multiplied_measuresisunique_by(full_name)atquery_properties.rs:577-581.
New test suite
tests/multiplied_measures_collector.rs is a good addition and the right level: it pins the gate directly rather than through SQL, and it runs without the integration-postgres feature. The five cases cover both halves of the conjunction plus the new multiplication half, and each asserts is_multiplied and the collected names, so a test can't pass for the wrong reason (e.g. test_calculated_measure_kept_whole_when_it_reaches_no_other_cube asserts multiplied-but-not-decomposed, which is the case that would otherwise silently swap with the not-multiplied one). measure_and_join builds the join the same way full_key_aggregate_measures does, so it's testing the real input.
Fixture / snapshot check
gold_share and its snapshot reconcile against the seed: SUCCESS = 100·300/700 (p1+p2 gold, p5 excluded for having no customer), DECLINED = NULL via NULLIF(0,0) since p3 has no customer, sibling total_amount 1200/300 over all rows. One readability note posted inline — the 700 denominator is what makes 42.857 hand-checkable and the doc comment doesn't name it.
Findings
| Sev | Where | Issue |
|---|---|---|
| Low | calculated_multi_fact.rs:414-418 |
New control test's snapshot shows a ratio (/700) and a total (1200) that don't reconcile; doc comment argues the opposite direction and doesn't name the denominator. Inline comment posted. |
| Low | calculated_multi_fact.rs:369,393 |
Prior thread re-affirmed: #[ignore] reasons still carry no tracking-issue link, and no plan-shape assertion pins the per-(cube, join tree) subquery split in feature-less runs. |
Checked and clear
- Pre-aggregation matching — making decomposition join-dependent means
MultiFactJoinGroups::multiplied_measures()(group join) andfull_key_aggregate_measures(per-measure join) can in principle disagree about which leaves are reported. I worked the guard atoptimizer.rs:524-530through both directions: a calculated measure is reported by name only when multiplied, and it decomposes exactly when multiplied, sopre_aggr_multiplied's contents are unchanged by this commit for the shapes that guard compares. No regression. - Pre-existing parent-cube-differs rule (
multiplied_measures_collector.rs:68-72) is deliberately left join-independent — correct, since a direct measure child on another cube must split regardless of multiplication. The two composite rules disagree about whether multiplication matters, but for the right reason. - Error message formatting — mixed inline-capture/positional
format!args are valid; message names the cube and both counts, which is what you'd want from a bug report. - Security / docs — unchanged from the first pass: no new interpolation, no user-visible surface, no
/docs-mintlifychange needed.
Not run: cargo test / cargo fmt — the sandbox denied cargo invocations here, so the new unit suite and the is_multiplied change are reviewed by reading only. CI results stand as the check.
The join grouping that multiplied measures rely on was only covered through Postgres-backed snapshots, so a run without the integration feature asserted nothing about it. Cover it where the grouping is decided. Record the denominator the non-multiplied control divides by: the ratio and the sibling total on that row are taken over different rows and are not meant to reconcile, which is not evident from the snapshot alone.
|
Claude finished @waralexrom's task in 1m 31s —— View job Re-review of
|
| Prior finding | Status |
|---|---|
Low: gold_share snapshot's 700 denominator vs sibling 1200 not recorded |
Fixed. Doc comment now names both numbers and states they aren't meant to reconcile. Thread resolved. |
Low: nothing pins the per-(cube, join tree) split without integration-postgres |
Partly. New unit test adds feature-free coverage at the grouping layer, but that layer was already correct pre-PR — see below. New inline comment posted; prior thread left unresolved (the #[ignore] tracking-link half is also still open). |
On the new test
test_two_groups_for_measures_of_one_cube is a reasonable regression guard and reads well — assert_eq!(total_amount.cube_name(), converted_value.cube_name()) up front makes the point of the test explicit, and asserting the grouped full_names (not just num_groups() == 2) rules out passing for the wrong reason.
But it doesn't guard this PR's first fix. build_groups is unchanged by the PR and already keyed on JoinKey, never on the owning cube (multi_fact_join_groups.rs:222-236); the cube-keyed bucketing lived in MultipliedMeasuresQueryPlanner::plan_queries. So this test passes on master as well, and reverting plan_queries to single_join() leaves it green. The commit message's "cover it where the grouping is decided" is accurate about the layer, just not about which layer the defect was in.
Doc-comment update checks out: SUCCESS = 100·300/700 = 42.857 (p1+p2 gold over p1+p2+p4, p5 having no customer), sibling payments.total_amount = 1200 over all rows. Naming both is exactly what makes the snapshot hand-checkable, and the "not meant to reconcile" sentence removes the wrong reading a reader would otherwise reach first.
Findings
| Sev | Where | Issue |
|---|---|---|
| Low | multi_fact_join_groups.rs:562 |
New unit test pins a precondition (build_groups splits same-cube measures) that already held on master, not the planner-side split the PR fixes; the per-(cube, join tree) subquery emission is still snapshot-only. Inline comment posted. |
Checked and clear
- Test placement — the test lives in
multi_fact_join_groups' ownmod tests, uses the sameMeasuresJoinHints::builder(...).add_dimensions(...).build(...)shape as the neighbouringtest_multi_fact_two_groups, and reuses the new PR fixture rather than adding another. Consistent with the file. - Group-order assertion — asserting
vec![total_amount], vec![converted_value]in order is safe, not flaky:build_groupsreturnskey_order(insertion order ofmeasure_hints), notHashMaporder. - Production code — unchanged in this commit; the second fix's
is_multipliednarrowing and theCubeError::internalconversion, both verified in the previous pass, are untouched. - No new snapshot files and no snapshot value changes — only a doc comment moved in
calculated_multi_fact.rs.
Not run: cargo test / cargo fmt — cargo invocations are denied in this sandbox, so the new unit test is reviewed by reading only. CI stands as the check.
· branch tesseract-fix-distinct-over-text-id ·
The suite proved the numbers through Postgres but said nothing about how the plan reached them, so a run without the integration feature let a regression in the join grouping through as long as the query still planned. Count the deduplicating key subselects - one per (key cube, join tree) - and check whether a calculated measure's components get columns of their own. Both read structure out of the generated SQL rather than pinning it.
|
Claude finished @waralexrom's task in 2m 54s —— View job Re-review of
|
| Prior finding | Status |
|---|---|
Low: nothing pins the per-(cube, join tree) subquery split without integration-postgres |
Fixed. Seven tests now assert keys_subquery_count, and two assert component-column presence. Thread on multi_fact_join_groups.rs:562 resolved. |
Low: #[ignore] reasons carry no tracking-issue link |
Still open (lines 398, 422). Thread left unresolved; not a blocker. |
Verification
The AS "keys" proxy is sound. Each AggregateMultipliedSubquery renders exactly one keys subselect under the hardcoded alias keys (aggregate_multiplied_subquery.rs:53), which reaches SQL through SingleAliasedSource::to_sql → templates.query_aliased → {{ query }} AS {{ quoted_alias }} (BaseQuery.js:4617), i.e. ) AS "keys" — the space and the quotes the helper matches are both real. No collision with the other keys-shaped alias in the planner: keys_aggregate_strategy.rs:127 uses fk_aggregate_keys, which the leading " in the pattern excludes.
Every asserted count reconciles with the fixture's join trees:
| Test | Count | Legs |
|---|---|---|
calculated_measure_alone |
1 | success_rate whole on {payments, payment_meta} |
two_sums_of_one_cube_needing_different_joins |
2 | +rates vs +merchants |
calculated_measure_with_joined_measure |
2 | success_rate (bare) vs converted_value (+rates) |
three_nested_join_trees_of_one_cube |
3 | bare / +rates / +rates,+merchants |
calculated_measure_reaching_other_cubes_alone |
2 | the decomposition itself — converted_value and commissioned_value on their own trees |
split_groups_rooted_at_different_cubes |
2 | rooted at payments vs at customers |
..._without_multiplication |
0 | no fan-out dimension → nothing multiplied |
The reaching_other_cubes_alone case is the one that actually pins the second fix: without decomposition rate_vs_commission is one whole measure, so both the count of 2 and the two component columns would go away. Good choice of anchor.
projects_column_for matches the real alias shape — the fully-quoted "cube__member" form with the closing quote rules out prefix collisions (payments__converted_value vs a longer alias), and all the member names here are well under any truncation threshold.
Findings
| Sev | Where | Issue |
|---|---|---|
| Low | calculated_multi_fact.rs:468-471 |
The negated projects_column_for assertion passes vacuously if the alias convention the helper assumes stops holding; a positive assertion for payments.gold_share in the same test would anchor it. Inline comment posted. |
Checked and clear
- Dialect coupling — both helpers assume double-quote identifiers.
TestContextbuilds against the Postgres templates, and these are the SQL-shape assertions rather than the portable ones, so this is fine; the failure mode of a dialect change is a loud count mismatch, not a silent pass (except in the one negated case above). - Failure messages — every assertion carries
"sql: {sql}", so a mismatch prints the plan it disagrees with. Exactly what you want from a structural assertion whose expected value is a bare integer. - No production changes in this commit; the
is_multipliednarrowing and theCubeError::internalconversion verified in the previous passes are untouched. No snapshot values changed. - Comment accuracy — the
keys_subquery_countdoc comment states the invariant it relies on ("one per (key cube, join tree)") and that it runs without a database, which is the part a future reader needs.
Not run: cargo test / cargo fmt — cargo invocations are denied in this sandbox, so the new assertions are reviewed by reading only. CI stands as the check.
· branch tesseract-fix-distinct-over-text-id ·
A result snapshot records the numbers a query returned but not how it got them, so what these cases exist to pin was only in their names. Assert the leg count everywhere the strategy is the point: aggregated in place or deduplicated through the keys path, measures of one cube split across trees or grouped onto one. The mixed-shapes case wanted three legs and gets two - the calculated measure shares a tree with the bare aggregate, which is grouping working rather than a shape being missed.
|
Claude finished @waralexrom's task in 2m 20s —— View job Re-review of
|
| Test | Asserted | Why |
|---|---|---|
calculated_measure_components_by_fan_out_dimension |
0 | both components are count_distinct — multiplication-safe, aggregated in place |
joined_measure_with_distinct_count_by_fan_out_dimension |
1 | converted_value (sum) takes the keys path, count doesn't |
calculated_measure_over_sums_with_joined_measure |
2 | success_amount_rate on {payments}, converted_value on +rates |
two_sibling_lookup_join_trees_of_one_cube |
2 | +rates vs +merchants, neither containing the other |
calculated_measure_over_components_with_different_joins |
3 | the two decomposed components plus total_amount on the bare tree |
measures_of_two_sibling_fan_out_cubes |
0 | nothing multiplied — see finding |
several_join_tree_shapes_in_one_query |
2 | success_rate shares {payments} with total_amount; net_value gets its own |
calculated_measure_reading_its_own_cube_directly |
0 | dimension is payments.status, no fan-out |
calculated_measure_with_joined_measure_without_fan_out |
0 | same |
The several_join_tree_shapes case is the interesting one, and the commit handles it the right way: the count came out 2 rather than the 3 the test's name suggests, and instead of contorting the query to reach 3 the doc comment now records why two is correct (grouping is by join tree, so the calculated measure shares the bare aggregate's). That's the more useful assertion — it pins that grouping actually merges, which nothing else in the suite did.
Worth noting the pair calculated_measure_alone (1) and calculated_measure_components (0) now makes a real planner behaviour visible: success_rate is routed through the keys path even though both its components are multiplication-safe on their own, because a type: number measure isn't classified safe. Conservative, correct, and previously invisible.
Prior findings — status
| Prior finding | Status |
|---|---|
Low: #[ignore] reasons carry no tracking-issue link |
Still open (lines 418, 442). Thread re-affirmed, left unresolved. Its second half — nothing pins plan shape without the feature — was fixed by 7c77708 and this commit extends that coverage to nine tests. |
Low: negated projects_column_for for gold_amount can pass vacuously |
Still open (line 490). Thread re-affirmed, and this commit adds a second instance at line 404 — new inline comment posted there, since that one has no positive assertion anywhere in its test. |
Findings
| Sev | Where | Issue |
|---|---|---|
| Low | calculated_multi_fact.rs:328-346 |
Doc comment says the three measures are "each needing its own deduplication" while the new assertion pins zero keys legs. What they need is one leaf query each via FullKeyAggregate — a different mechanism. Inline comment posted. |
| Low | calculated_multi_fact.rs:401-406 |
New negated projects_column_for has no positive counterpart in its test, so it reads "not decomposed" for any input if the alias convention shifts. Inline comment posted. |
Checked and clear
- No production changes in this commit. The
is_multipliednarrowing and theCubeError::internalconversion, verified in earlier passes, are untouched; no snapshot values changed. AS "keys"proxy — re-checked, still one perAggregateMultipliedSubquery(aggregate_multiplied_subquery.rs:53→templates.query_aliased), and the leading quote excludesfk_aggregate_keys.- Failure messages — every new assertion carries
"sql: {sql}", so a count mismatch prints the plan it disagrees with. That matters more here than for a snapshot, where the diff is self-explanatory. - Helper doc comments — the rewritten
keys_subquery_countcomment states why a count assertion earns its place next to a result snapshot, which is the part a future reader would otherwise delete as redundant. Good. - Coverage direction — the two
#[ignore]d tests are deliberately left without count assertions, which is right: their expected plan shape isn't known yet.
Not run: cargo test / cargo fmt — cargo invocations are denied in this sandbox, so the new assertions are reviewed by reading only. CI stands as the check.
… unit test A negated substring match passes both when a measure genuinely stays whole and when the alias convention the helper reads stops holding, at which point the assertion is inert without failing. Pair each with the positive case from the same query, which cannot hold unless the convention does. Say what the same-cube grouping unit test covers - the precondition callers slice on, not what any of them emits per group - and correct the sibling fan-out case, whose prose claimed a deduplication its own assertion denies.
|
Claude finished @waralexrom's task in 1m 28s —— View job Re-review of
|
| Prior finding | Status |
|---|---|
Low: negated projects_column_for for gold_amount can pass vacuously (line 504) |
Fixed. Positive assertion on payments.gold_share added before it, with a comment saying why it comes first. Thread resolved. |
Low: same pattern for converted_value with no positive anchor in its test (line 413) |
Fixed. Positive assertion on payments.converted_per_max_amount added. Thread resolved. |
Low: two_sibling_fan_out prose claimed a dedup its own assertion denies (line 328) |
Fixed. Now names FullKeyAggregate and gives both reasons nothing is multiplied. Thread resolved. |
Low: #[ignore] reasons carry no tracking-issue link |
Still open (lines 425, 449). Re-affirmed, thread left unresolved. Not a blocker — the reasons themselves are honest about the mechanism. |
Verification
The anchoring works, and in the specific way that matters. A positive projects_column_for on a requested measure holds in both the whole and decomposed plans, so it can't distinguish them — that's not its job here; it fails exactly when the convention the helper reads stops holding, which is the failure mode that made the negated assertion inert. And the anchor is the longer name in both pairs (payments__converted_per_max_amount at 33 chars vs payments__converted_value at 25; gold_share vs gold_amount is a wash but the quote/separator half is covered), so a truncation or hashing scheme trips the positive before it silently satisfies the negative. Neither name is a substring of its counterpart in either direction, so no accidental match.
The corrected two_sibling_fan_out doc comment checks out against the fixture. payment_meta and payment_tags are each the to side of a one_to_many declared on payments, so find_multiplication_factor_for never marks them multiplied and their counts aggregate in place; payments.total_amount's own tree is {payments} because the sole dimension is payments.status, so payments isn't multiplied either. Zero keys legs, three leaf queries joined by FullKeyAggregate — the prose and the assertion now name the same mechanism, which was the point.
The multi_fact_join_groups scope note is the right correction rather than a hedge. It says the test pins the precondition callers slice on and explicitly disclaims covering what any caller emits per group — which is exactly the boundary I'd flagged, and it leaves the planner-side split to keys_subquery_count, where 7c77708 put it. The two layers now each say what they cover.
Findings
None new. One prior low re-affirmed (see table).
Checked and clear
- No production changes in this commit — the
is_multipliednarrowing and theCubeError::internalconversion verified in earlier passes are untouched, and no snapshot values moved. - Assertion ordering — putting the positive first means a convention drift reports as the convention failing rather than as a spurious "measure was decomposed", so the failure message points at the real cause.
- Comment style — both corrected comments state the reason inline (
the two fan-out cubes sit on the many side of their own joins) rather than just the conclusion, matching the rest of the suite. - Coverage of the two fixes — unchanged and still intact:
calculated_measure_reaching_other_cubes_alone(2 legs + two component columns) anchors the decomposition fix,two_sums_of_one_cube_needing_different_joins/three_nested_join_trees_of_one_cubeanchor the per-(cube, join tree) split.
Not run: cargo test / cargo fmt — cargo invocations are denied in this sandbox, so the new assertions are reviewed by reading only. CI stands as the check.
· branch tesseract-fix-distinct-over-text-id ·
Splitting a calculated measure into its components put the two halves of an expression on separate legs. When those legs root at different cubes they see different rows, so the ratio came out over a denominator its numerator never saw - answered, with no error, differently depending on whether the query carried a fan-out dimension. The split also never covered the shape it was written for: whether the measure subquery is built is decided per join group, while the condition for splitting was read off a single measure, so a calculated measure whose own dependencies stay inside its cube still lands in that subquery whenever a group member reaches out of it. Refuse instead. A measure carrying no aggregate of its own cannot survive a subquery that renders measures without theirs, whichever way it got there, so say that where the subquery is built and name the measure. Shapes that used to reach the database as SQL it rejects now stop at the planner.
|
Claude finished @waralexrom's task in 4m 33s —— View job Re-review of
|
| Shape | Before | Now |
|---|---|---|
| calculated over own-cube sums | ungrouped subquery drops the children's aggregates → must appear in the GROUP BY clause |
refused |
calculated with a {other.measure} component |
components de-aggregated → numeric * text on the TEXT pk fixture |
refused |
number_agg with a hand-written MAX(...) |
MAX inside a select with no GROUP BY → invalid |
refused |
constant type: number (sql: "100") |
subquery projects it, outer aggregating select references it unaggregated → invalid | refused |
Rank |
filtered upstream by has_multi_stage_members, never reaches this bucket |
unchanged |
So every refused shape previously reached the database as SQL it rejects. Nothing that worked stops working.
The JS suite still passes under Tesseract, and for a non-obvious reason worth recording: calculated-measure-multi-fact.test.ts asserts successRate + convertedValue by Meta.value succeeds on Tesseract, and it still does — but only because fix #1 splits them. successRate's tree is {Payments, Meta}, convertedValue's is +Rates, so successRate sits in a group of its own where collect_cube_names is {payments}, should_build_join is false, and it reads the pk cube directly with its aggregate intact over the deduplicated key set. The two fixes are now load-bearing for each other: without the per-join-tree split, that test's query would hit the new refusal. Worth a sentence in the PR body, since it's the difference between "works" and "refused" for the originally reported model.
Findings
| Sev | Where | Issue |
|---|---|---|
| Low | multiplied_measures_query_planner.rs:236-242 |
The message claims the named measure "reaches another cube" and advises rewriting it "over measures of its own cube". In the shared-group case both are false — success_rate reaches nothing and is already over its own cube's measures; the reach belongs to a sibling. check_should_build_join_for_measure_select already has the triggering measure in hand. Inline comment posted. |
| Low | multiplied_measures_query_planner.rs:231-234 |
let Ok(symbol) = ... else { continue } exempts member-expression measures, which the sibling check handles explicitly. Not a regression, but the exemption reads as an oversight without a note. Inline comment posted. |
Prior findings — status
| Prior finding | Status |
|---|---|
Low: #[ignore] reasons carry no tracking link; plan shape unpinned without the feature |
Resolved. Both #[ignore]s are gone — those tests now assert the refusal and run feature-free. Thread resolved. |
| Everything else from the earlier passes | Already resolved; the two deleted threads' subjects (the composite gate, the travels_only_through_measures asymmetry) no longer exist in the diff. |
Coverage
Net loss is small and in the right direction: the deleted tests/multiplied_measures_collector.rs tested a gate that no longer exists, and the two deleted snapshots recorded the unsound split's output. The five shapes that now error are each asserted through expect_no_own_aggregate_error, which checks the reason string and that the message names the offending measure — so a refusal for the wrong reason, or one that doesn't tell the user which measure to change, fails the test. build_sql returning Err means all five run without integration-postgres.
One gap: nothing asserts the JS-test property above at the Rust level — that success_rate + converted_value by payment_meta.value alone still succeeds while adding rates.currency refuses it. test_calculated_measure_with_joined_measure covers the success half and the new test the refusal half, but they're in different places and neither says it's the same query one dimension apart. A cross-reference in either doc comment would keep the pair from drifting.
Documentation
The PR body is now stale in three places: it still presents the second fix as "Decompose calculated measures that reach other cubes" with the reasoning for the decomposition gate, still says "17 tests, 2 #[ignore]d", and still describes the two MAX({rates.fx_rate}) shapes as tracked-separately follow-ups when they're now refused with a message. Since this is the text that lands in the squashed commit, worth rewriting before merge.
Checked and clear
- Call sites —
aggregate_subquery_planhas exactly one caller, so the refusal cannot leak into a path that doesn't build the subquery.CubeError::user(notinternal) is right: this is a modeling limitation, and it surfaces as a 4xx. - Import —
crate::planner::AggregateWrapresolves viapub use symbols::*inplanner/mod.rs:41. - Fix ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1 untouched — the per-
(cube, join tree)loop, theCubeError::internalgrouping invariant, and all the count assertions from7c77708/6bd2879are unchanged. - Security / performance — no new interpolation; the removed collector helpers drop a repeated
collect_cube_nameswalk per calculated node, so this is marginally cheaper than the previous commit.
Not run: cargo test / cargo fmt — cargo invocations are denied in this sandbox, so the new check and the rewritten tests are reviewed by reading only. CI stands as the check.
· branch tesseract-fix-distinct-over-text-id ·
Refusing during logical planning ran ahead of the pre-aggregation optimizer, which may replace the whole node with a rollup and never render the subquery at all - a query a rollup answers was turned into an error. Move the check to where the subquery is actually built, past the point the override returns. Say what the user can do about it. The subquery is forced by whichever measure of the group reaches another cube, which need not be the one being refused, so telling that measure to be rewritten over its own cube asks for something it may already satisfy; ask for a separate query instead.
|
Claude finished @waralexrom's task in 4m 35s —— View job Re-review of
|
| Sev | Where | Issue |
|---|---|---|
| Low | aggregate_multiplied_subquery.rs:23-28 |
The message presumes a sibling pulled the measure in ("alongside a measure that joins another cube", "request it in a separate query"), but four of the five tests asserting it request the measure alone and it still fails. test_calculated_measure_over_components_with_different_joins' own doc comment says splitting doesn't help. Inline comment with wording that holds in both arms. |
| Low | aggregate_multiplied_subquery.rs:142 |
The behaviour the commit exists for is untested — the fixture declares no pre_aggregations, so no test exercises the override path and reverting the move leaves the suite green. Inline comment posted. |
| Low | aggregate_multiplied_subquery.rs:21 |
Re-affirmed from the outdated thread (the continue moved verbatim): member-expression measures are silently exempt, and the exemption reads as an oversight without a note. |
On the first one — only test_calculated_measure_pulled_into_a_shared_measure_subquery is actually helped by a separate query. I checked why: check_should_build_join_for_measure_select reads cube names off the measures of the group, never the dimensions, so success_rate alone has cubes == {payments} == key_cube and plans fine. In the other four the measure reaches out itself, so it's refused alone too.
Prior threads
Both remaining threads were on code this commit moved out of multiplied_measures_query_planner.rs, so both are resolved. The message-accuracy one is genuinely addressed for the case it was opened about — the text no longer claims the named measure reaches another cube — hence the new thread for the arm it now gets wrong, rather than a reopen.
Test-file changes
- Dropping the two negated
projects_column_forassertions (converted_value,gold_amount) is right: with decomposition gone there is no plan in which those columns would appear, so the assertions were vestigial rather than load-bearing. The positive anchors added in4e7593bare kept, which is the half that still says something. expect_no_own_aggregate_errortracks the reworded message (has no aggregate of its own) and still asserts the message names the measure, so a refusal that doesn't tell the user what to change fails the test.- Helper doc comments were trimmed hard.
keys_subquery_countkeeps the sentence a future reader needs ("which the result snapshot alone cannot"), so the loss is acceptable;projects_column_forandexpect_no_own_aggregate_errorare now bare, and the latter's deleted comment was the only place recording why these shapes are refused rather than attempted. Not worth a finding, but thecheck_measures_survive_measure_subquerydoc comment is now carrying that explanation alone. test_calculated_measure_reaching_other_cubes_alone's doc comment is corrected to describe refusal rather than the removed decomposition — consistent withaaff392's direction.
Checked and clear
- Fix ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1 untouched. The per-
(cube, join tree)loop, theCubeError::internalgrouping invariant, and everykeys_subquery_countassertion are unchanged by this commit. - No snapshot churn. No
.snapfiles added, removed, or edited — consistent with a change that only moves where an error is raised. - Rollup correctness on the override path. Worth noting the override replaces the node wholesale, so the
add_ungrouped_measure_referenceregistrations in the non-override branch never run and the parentFullKeyAggregateconsumes an ordinary grouped select — the same contract theCubesource arm already relies on. The de-aggregation the check guards against simply doesn't arise there. - Wasted work before the error. The check runs after
keys_subqueryhas already been processed and the join builder set up. Harmless, and hoisting it above that would mean matching onsourcetwice — not worth it. - Security / performance — no new interpolation; the check is a linear scan over a group's measures once per rendered node.
Not run: cargo test / cargo fmt — cargo invocations are denied in this sandbox, so the moved check is reviewed by reading only. CI (only the labeler has reported so far) stands as the check.
· branch tesseract-fix-distinct-over-text-id ·
| return Err(CubeError::user(format!( | ||
| "{} has no aggregate of its own, so it cannot be computed alongside a measure \ | ||
| that joins another cube under a dimension that multiplies its rows. Please \ | ||
| request it in a separate query.", | ||
| measure.full_name() | ||
| ))); |
There was a problem hiding this comment.
The new wording fixes the shared-group case my earlier thread was about, but it now misses the opposite one — and that's four of the five tests that assert this message.
"cannot be computed alongside a measure that joins another cube" and "please request it in a separate query" both presume a sibling is what pulled the measure in. In test_calculated_measure_reaching_other_cubes_alone there is no sibling: rate_vs_commission is the only measure requested, and it is the one reaching rates/merchants. Same for test_calculated_measure_reaching_past_its_components and test_calculated_measure_over_components_rooted_at_different_cubes — all three are single-measure queries. And test_calculated_measure_over_components_with_different_joins says so in its own doc comment: "splitting the query does not give the expression an aggregate" — which is exactly what the error tells the user to try.
By my reading only test_calculated_measure_pulled_into_a_shared_measure_subquery is helped by a separate query: there success_rate's own collect_cube_names is {payments}, so alone it lands in a group where should_build_join is false. In the other four the measure reaches out itself, so it is refused alone too and the advice sends the user in a circle.
Since the reach is what makes the subquery necessary, naming it covers both arms without needing to know which measure caused it:
| return Err(CubeError::user(format!( | |
| "{} has no aggregate of its own, so it cannot be computed alongside a measure \ | |
| that joins another cube under a dimension that multiplies its rows. Please \ | |
| request it in a separate query.", | |
| measure.full_name() | |
| ))); | |
| if matches!(symbol.kind().aggregate_wrap(), AggregateWrap::PassThrough) { | |
| return Err(CubeError::user(format!( | |
| "{} has no aggregate of its own, so it cannot be re-aggregated over the \ | |
| deduplicated rows this query needs - a measure of its group reaches another \ | |
| cube, under a dimension that multiplies its rows. Please drop the multiplying \ | |
| dimension, request the measures that reach out separately, or move the \ | |
| aggregation into a measure.", | |
| measure.full_name() | |
| ))); | |
| } |
If you'd rather keep the message short, dropping just the "alongside a measure" / "separate query" clauses already leaves it true in all five shapes.
| } | ||
| } | ||
| AggregateMultipliedSubquerySource::MeasureSubquery(measure_subquery) => { | ||
| check_measures_survive_measure_subquery(&measure_subquery.schema.measures)?; |
There was a problem hiding this comment.
The move itself is right — I traced it and the motivation holds: optimizer.rs:269-276 builds a replacement AggregateMultipliedSubquery with pre_aggregation_override: Some(rewritten), and process returns at line 60-62 before reaching this arm. So with the check in the logical planner, a query a rollup could answer was refused before the optimizer ever ran.
But nothing tests that. integration_calculated_multi_fact.yaml declares no pre_aggregations, so none of the five refusal tests can exercise the override path — put the check back in plan_queries and the whole suite stays green. Given the commit exists for exactly this case, one rollup in the fixture covering one of the refused shapes plus build_sql_with_used_pre_aggregations asserting the usage (rather than an error) would make the move self-guarding.
| for measure in measures.iter() { | ||
| let Ok(symbol) = measure.as_measure() else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
Carrying over the note from the now-outdated thread on the old location, since the continue moved verbatim: it silently exempts member-expression measures (the SQL API's ad-hoc symbols, for which as_measure() is Err). check_should_build_join_for_measure_select goes out of its way to handle exactly those (cube_names_if_dimension_only_expression), so an expression measure with a hand-written aggregate over a joined cube's column still reaches the database as the SQL this guard exists to stop.
Not a regression and I wouldn't widen the guard speculatively — but one line saying member expressions are deliberately out of scope would keep the next reader from reading continue as an oversight.
Nothing held the check to where it runs: the fixture declared no rollups, so moving it back ahead of the pre-aggregation optimizer left the suite green while turning a query a rollup answers into an error. Store one of the refused shapes and assert the usage. Name the reach rather than a sibling in the message. Four of the five refused shapes are single-measure queries where the measure reaches out itself, so asking for it in a separate query sends the user round in a circle; what forces the subquery is the reach, whoever carries it. Say that member expressions are left out deliberately, so the skip does not read as an oversight.
1 participant
Summary
Two related defects in Tesseract's planning of multiplied measures — measures under a
one-to-many join, which go through a keys subquery to deduplicate row fan-out. Both surface on
the same shape: a calculated measure next to a measure that needs a wider join, grouped by a
dimension reached through
hasMany. That combination was either refused outright or compiled toinvalid SQL.
Reported against a payments-analytics model: an authorization-rate measure (a ratio of two
countDistinctmeasures) together with an FX-converted value measure that joins a rates cube,grouped by a metadata key/value cube.
Changes
Group multiplied measures by (cube, join tree), not by cube alone.
The bucket was keyed on the owning cube, and the whole bucket was then required to resolve to a
single multi-fact join group — so two measures of one cube needing different join trees were
rejected with
Expected just one multi-fact join group. The key cube fixes the primary keys todeduplicate on, the join tree fixes the joins to build, and neither determines the other, so one
AggregateMultipliedSubqueryis now emitted per pair. This mirrors the regular-measure branch afew lines above, which already emits one CTE per join group.
Trees are kept apart rather than merged so a measure's value cannot depend on which other
measures share the query. Merging sibling one-to-many branches introduces a cross product that
neither original tree had — it flips measures from regular to multiplied, i.e. it changes what
the query means.
Refuse measures the measure subquery cannot carry.
Reaching a cube beyond its own makes a multiplied measure go through the measure subquery, which
renders measures without their aggregate so the select above can re-apply it. A calculated
measure (
type: numberornumber_agg) has no aggregate of its own, so nothing is re-applied: itscomponents lose theirs inside the subquery and the expression comes out neither aggregated nor
grouped, reaching the database as SQL it rejects. Those shapes now stop at the planner with a
message naming the measure.
Splitting such a measure into its components was tried and taken back out. It put the two halves of
an expression on separate legs, and when those legs root at different cubes they see different
rows, so the ratio came out over a denominator its numerator never saw — answered, with no error,
differently depending on whether the query carried a fan-out dimension. It also never covered the
shape it was written for: whether the subquery is built is decided per join group, while the
condition for splitting was read off a single measure.
Testing
Unit tests for the decomposition gate in
tests/multiplied_measures_collector.rs(5 tests), eachpinning one arm of it: decompose when multiplied and reaching, keep whole when not multiplied, when
reaching nothing, when reaching past the components, and when reading the own cube directly.
New integration suite
calculated_multi_fact.rs(17 tests, 2#[ignore]d) on a dedicatedfixture with a TEXT primary key, two lookup cubes and two fan-out branches. Covers nested and
sibling join trees, a measure reading three cubes, a star with two fan-out branches, split groups
rooted at different cubes, and the shapes that must not decompose. Results are asserted against
Postgres via insta snapshots; every expected value is hand-checkable from the seed.
A matching JS integration test runs under both planners. The legacy planner still has the
de-aggregation defect, so it asserts the legacy failure explicitly.
Both fixes were verified to be genuinely guarded: reverting each one alone turns exactly the
intended tests red and leaves the controls green.
cargo test --features integration-postgres— 1120 passed, 0 failed, 11 ignoredTesseract, 0 failed
cargo fmt --checkandeslintcleanQueries that already worked keep their plans: with a single join group per cube the loop emits the
same single CTE with the same measures, and the calculated-measure gate is scoped to the multiplied
case the defect lives in.
Two shapes remain broken and are
#[ignore]d with the reason recorded: a measure whose aggregateis hand-written over a joined cube's column (
MAX({rates.fx_rate})), with or without a componentmeasure. Those have never worked — verified to fail identically at v1.7.15 and on the legacy
planner — and they need a different mechanism, since there are no components to hand the
aggregation down to. Tracked separately.