Skip to content

fix(tesseract): Push ad-hoc filters into view measure aggregation#11137

Open
ovr wants to merge 2 commits into
masterfrom
fix/tesseract-ad-hoc-measure-aggregation
Open

fix(tesseract): Push ad-hoc filters into view measure aggregation#11137
ovr wants to merge 2 commits into
masterfrom
fix/tesseract-ad-hoc-measure-aggregation

Conversation

@ovr

@ovr ovr commented Jun 22, 2026

Copy link
Copy Markdown
Member

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.

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.
@github-actions github-actions Bot added the rust Pull requests that update Rust code label Jun 22, 2026
@claude

claude Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ovr's task in 48s —— View job


Code review

Verdict: LGTM. Small, well-targeted fix that mirrors the legacy planner's behaviour (patchMeasurePushDownFilterSql) and uses an established pattern (resolve_reference_chain is called ~20 places in this crate). Regression covered by both a SQL-shape assertion and a Postgres execution snapshot.

Details (1 minor observation, 0 blocking)

Correctness

  • The resolve-before-patch is the right move: view measures wrap as Calculated/number, which new_patched rejects. Resolving to the owning cube measure puts the filter inside SUM(CASE WHEN ... END). ✔
  • resolve_reference_chain is documented (and used elsewhere) as a no-op when the symbol isn't a reference, so non-view PatchMeasure is preserved. ✔
  • The patched symbol now identifies as the underlying cube measure rather than the view measure. The test asserts both the SQL shape (CASE WHEN) and end-to-end execution against PG returns the expected value (30.00 = 10 + 20 from root_val_sum filtered by root_test_dim = 'rt_x'), so this is verified to behave correctly downstream. ✔

Minor: fallback branch (non-blocking)

query_properties_compiler.rs:281-287 — the else branch falls back to source_measure_compiled (the pre-resolve symbol). For consistency with the resolve-first intent it could use resolved_source. In practice this likely doesn't matter (if resolved_source.as_measure() is Err, the original almost certainly is too), but it would read more obviously correct. Optional.

Tests

  • test_many_to_one_view_patched_measure_filter is a proper regression test: it would have panicked at unwrap() before the fix, plus the CASE WHEN assert and PG snapshot lock in the actual SQL shape and result. ✔
  • Snapshot is minimal and deterministic. ✔
  • Helper make_patched_measure is a clean addition — likely reusable for future PatchMeasure regressions.

Style

  • Comment on :275-279 explains why, which is exactly what's helpful here. ✔
· `fix/tesseract-ad-hoc-measure-aggregation`

@claude

claude Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ovr's task in 37s —— View job


Code review (re-review after chore: up code quality)

Verdict: LGTM — still ship-ready. The new commit only swaps .unwrap() for ? propagation in the regression test (and threads Result<(), CubeError> through the signature). Behaviour is identical on failure (test fails instead of panicking), and the comment is updated to match. No new concerns; my prior review still stands.

Details
  • Read latest commit (01c0626)
  • Re-checked query_properties_compiler.rs:273-287 — unchanged since first review
  • Verified test still asserts CASE WHEN push-down and the PG snapshot
  • Confirmed no other files touched

The earlier optional nit (use resolved_source rather than source_measure_compiled in the else fallback at query_properties_compiler.rs:285-286) is still open but remains non-blocking — the fallback only fires when as_measure() errors on the resolved chain, in which case the un-resolved symbol is no more likely to succeed; it's a readability tweak, not a correctness issue.

· `fix/tesseract-ad-hoc-measure-aggregation`

// 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();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claude please double verify that without this change, test test_many_to_one_view_patched_measure_filter fails. Please verify it carefully

@claude claude Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 run cargo test directly — 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 at measure_symbol.rs:748 (is_view && is_sql_is_direct_ref), and
  • kind = Calculated(Number) — because the view exposes the measure with measure_type = "number", so MeasureKind::from_type_str("number", …) yields Calculated (measure_symbol.rs:716).

With the fix removed, the branch reads if let Ok(source_measure) = source_measure_compiled.as_measure():

  1. as_measure() succeeds. The symbol is a MemberSymbol::Measure, so as_measure() returns Ok (member_symbol.rs:297-305). So we enter the new_patched branch — the error path, not the harmless else fallback.
  2. new_patched rejects the filter. add_filters is non-empty (the ad-hoc {…root_test_dim} = 'rt_x'), result_kind is Calculated(Number), and Calculated hits the _ => false arm of supports_additional_filters() (measure_kinds/mod.rs:184-197). So new_patched returns Err("Unsupported additional filters for measure root_val_sum type number") (measure_symbol.rs:194-200).
  3. The ? propagates. query_properties_compiler.rs:283 uses ?, so the Err bubbles out of the compile step → out of build_sql_from_options.
  4. The test fails. Line 246 is let sql = ctx.build_sql_from_options(options.clone())?; — the ? returns the Err as the test's Result, so the #[tokio::test] reports failure with that error message. (Earlier, before chore: 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

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.63%. Comparing base (bd8b6e6) to head (01c0626).

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           
Flag Coverage Δ
cubesql 83.63% <ø> (ø)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants