fix(tesseract): Push ad-hoc filters into view measure aggregation#11137
fix(tesseract): Push ad-hoc filters into view measure aggregation#11137ovr wants to merge 2 commits into
Conversation
A measure exposed through a view is a reference wrapper whose declared `type` is collapsed to `number` (a Calculated kind), hiding the real aggregation. PatchMeasure then rejected ad-hoc CASE-WHEN filters with "Unsupported additional filters for measure ... type number", because Calculated measures don't support additional filters. Resolve the reference chain to the owning cube measure before patching, so the filter is pushed inside the real aggregation (`SUM(CASE WHEN ... END)`), matching the legacy planner's `patchMeasurePushDownFilterSql`. `resolve_reference_chain` is a no-op for plain cube measures, so non-view PatchMeasure behaviour is unchanged.
|
Claude finished @ovr's task in 48s —— View job Code reviewVerdict: LGTM. Small, well-targeted fix that mirrors the legacy planner's behaviour ( Details (1 minor observation, 0 blocking)Correctness
Minor: fallback branch (non-blocking)
Tests
Style
|
|
Claude finished @ovr's task in 37s —— View job Code review (re-review after
|
| // the reference chain to the owning cube measure so the filter is | ||
| // pushed inside the aggregation (`SUM(CASE WHEN ... END)`); a no-op | ||
| // for plain cube measures. | ||
| let resolved_source = source_measure_compiled.clone().resolve_reference_chain(); |
There was a problem hiding this comment.
@claude please double verify that without this change, test test_many_to_one_view_patched_measure_filter fails. Please verify it carefully
There was a problem hiding this comment.
Claude finished @ovr's task in 2m 45s —— View job
Verified: the test fails without the change ✔
- Locate the change and the regression test
- Trace the exact failure path (cargo is blocked by the sandbox in this env — verified by rigorous code-path analysis instead)
- Confirm it fails (and how)
- Report findings
Answer to @ovr: Yes — without this change test_many_to_one_view_patched_measure_filter fails, and it fails before the CASE WHEN assert or the PG snapshot ever run, on build_sql_from_options(...)? at member_expressions_on_views.rs:246.
⚠️ I was unable to runcargo testdirectly —cargo/git-mutation commands are blocked by the sandbox in this CI environment (every invocation returns "requires approval"). To compensate I traced the failure end-to-end through the source. If you want a live run, the earlier[View job]CI runs already exercised this test green with the fix; the trace below shows what happens once the fix is removed.
The failing path, step by step (with the fix reverted)
The view measure many_to_one_view.root_val_sum compiles to a MeasureSymbol with:
is_reference = true— set atmeasure_symbol.rs:748(is_view && is_sql_is_direct_ref), andkind = Calculated(Number)— because the view exposes the measure withmeasure_type = "number", soMeasureKind::from_type_str("number", …)yieldsCalculated(measure_symbol.rs:716).
With the fix removed, the branch reads if let Ok(source_measure) = source_measure_compiled.as_measure():
as_measure()succeeds. The symbol is aMemberSymbol::Measure, soas_measure()returnsOk(member_symbol.rs:297-305). So we enter thenew_patchedbranch — the error path, not the harmlesselsefallback.new_patchedrejects the filter.add_filtersis non-empty (the ad-hoc{…root_test_dim} = 'rt_x'),result_kindisCalculated(Number), andCalculatedhits the_ => falsearm ofsupports_additional_filters()(measure_kinds/mod.rs:184-197). Sonew_patchedreturnsErr("Unsupported additional filters for measure root_val_sum type number")(measure_symbol.rs:194-200).- The
?propagates.query_properties_compiler.rs:283uses?, so theErrbubbles out of the compile step → out ofbuild_sql_from_options. - The test fails. Line 246 is
let sql = ctx.build_sql_from_options(options.clone())?;— the?returns theErras the test'sResult, so the#[tokio::test]reports failure with that error message. (Earlier, beforechore: up code quality, this was an.unwrap()and would have panicked at the same point — same outcome, louder.)
Why the fix makes it pass
resolve_reference_chain() walks reference_member() (member_symbol.rs:214-220) from the view measure to the owning cube measure, whose kind is Aggregated(Sum). Sum does support additional filters, so new_patched succeeds and the filter is rendered inside the aggregation (SUM(CASE WHEN … END)) — which is exactly what the assert!(sql.contains("CASE WHEN")) and the PG snapshot (30.00 = 10 + 20) then verify.
Conclusion: The regression guard is genuine. Remove the two changed lines and the test stops at step 4 with Unsupported additional filters for measure root_val_sum type number — it never reaches the assertion or snapshot. The fix is load-bearing.
· fix/tesseract-ad-hoc-measure-aggregation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11137 +/- ##
=======================================
Coverage 83.63% 83.63%
=======================================
Files 256 256
Lines 79019 79019
=======================================
Hits 66085 66085
Misses 12934 12934
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:
|
A measure exposed through a view is a reference wrapper whose declared
typeis collapsed tonumber(a Calculated kind), hiding the real aggregation. PatchMeasure then rejected ad-hoc CASE-WHEN filters with "Unsupported additional filters for measure ... type number", because Calculated measures don't support additional filters.Resolve the reference chain to the owning cube measure before patching, so the filter is pushed inside the real aggregation (
SUM(CASE WHEN ... END)), matching the legacy planner'spatchMeasurePushDownFilterSql.resolve_reference_chainis a no-op for plain cube measures, so non-view PatchMeasure behaviour is unchanged.