Skip to content

fix(tesseract): CASE handling fixes in multi-stage measures#10764

Merged
waralexrom merged 4 commits intomasterfrom
tesseract-string-dimension-in-switch-case
Apr 29, 2026
Merged

fix(tesseract): CASE handling fixes in multi-stage measures#10764
waralexrom merged 4 commits intomasterfrom
tesseract-string-dimension-in-switch-case

Conversation

@waralexrom
Copy link
Copy Markdown
Member

Check List

  • Tests have been run in packages where changes have been made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

@github-actions github-actions Bot added the rust Pull requests that update Rust code label Apr 28, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 28, 2026

Claude finished @waralexrom's task in 2m 43s —— View job


Code Review: fix(tesseract): CASE handling fixes in multi-stage measures

  • Gather context and read changed files
  • Review main logic changes in multi_stage_query_planner.rs
  • Review test fixtures and test cases
  • Review snapshot files
  • Post comprehensive review

Summary

This PR fixes a correctness bug in how multi-stage CASE/SWITCH measures generate leaf CTEs. The core issue: when the same dependency (e.g., {CUBE.total_amount}) appeared across multiple WHEN branches, the old code would only apply the first branch's filter to the CTE, discarding subsequent branches. This produced incorrect results — only one status's data was aggregated instead of all relevant statuses.

What Changed

Root Cause Fix (multi_stage_query_planner.rs):
The old code iterated per-branch, using a processed_deps HashSet to skip already-seen dependencies. If total_amount appeared in both the "completed" and "pending" branches, only the "completed" filter was applied.

The new approach is a two-pass design:

  1. Collection pass: Walks all WHEN and ELSE branches, collecting dependencies into a HashMap that merges the union of switch values each dep needs.
  2. Emission pass: Creates one CTE per unique dependency with a combined IN filter (e.g., status IN ('completed', 'pending')).

This correctly handles:

  • Same dep in multiple WHEN branches → merged filter values
  • ELSE branch with known values → uses complement values
  • ELSE branch with unknown universe → unrestricted (no prefilter), safe fallback

Schema Fix (integration_multi_stage.yaml): Changed status_weighted_amount from type: number to type: sum. This is a necessary correction — number type doesn't aggregate in multi-stage context.

Tests: Removed #[ignore] from 3 previously-known-broken tests, added a new test_case_switch_dimension_only test, and added stable ordering to test_case_switch_with_dimension. All 4 snapshot files contain plausible expected results.

Code Quality

The refactored code is cleaner than the original (the //TODO refactor needed comment was rightfully removed). The record closure pattern is a clean way to merge values, and the match on (&mut entry.1, branch_values) handles the three cases (already unrestricted, becoming unrestricted, accumulating values) concisely.

Potential Issues

  1. order + HashMap dual-tracking: The order: Vec<String> and deps: HashMap<...> pattern works correctly but introduces a subtle invariant — they must stay in sync. An IndexMap would be a cleaner single data structure, though this is not a correctness concern. (See inline comment.)

  2. Linear contains() for dedup: The values.contains(&v) check on line 273 is O(n) per value. This is fine for typical CASE expressions with a handful of branches, but is worth noting.

  3. No negative test coverage: There's no test for CASE with CaseSwitchItem::Sql(...) (non-member switch), which takes the early return Ok(false) path. Not a regression from this PR, but worth adding eventually.

Verdict

Looks good. The fix is correct, well-structured, and properly tested. The two-pass approach cleanly solves the dependency merging problem that the old per-branch iteration couldn't handle. The snapshot results (e.g., 375/600/750 for weighted amounts across all statuses) match expected behavior.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.78%. Comparing base (187ee17) to head (75443a2).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #10764   +/-   ##
=======================================
  Coverage   78.78%   78.78%           
=======================================
  Files         466      466           
  Lines       92061    92061           
  Branches     3383     3383           
=======================================
+ Hits        72527    72532    +5     
+ Misses      19042    19037    -5     
  Partials      492      492           
Flag Coverage Δ
cube-backend 58.01% <ø> (ø)
cubesql 83.41% <ø> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Copy Markdown
Member

@ovr ovr left a comment

Choose a reason for hiding this comment

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

Nit: about usage of Result instead of panic, otherwise LGMT 👍

@waralexrom waralexrom requested a review from a team as a code owner April 28, 2026 17:38
@waralexrom waralexrom merged commit cb66e8e into master Apr 29, 2026
130 of 131 checks passed
@waralexrom waralexrom deleted the tesseract-string-dimension-in-switch-case branch April 29, 2026 09:43
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