fix(cubesql): Keep distinct count pushdown grouped

[MazterQyou]

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

Issue Reference this PR resolves

#11542

Description of Changes Made

This PR fixes a COUNT(DISTINCT measure) + MAX(CASE ...) query with an outer ORDER BY silently returning 1 per group instead of the real distinct count, by folding the MAX(CASE ...) half into a literal 0 in the SQL API wrapper the same way the existing grouped pushdown rule does, so the aggregation reaches Cube as a grouped request. Related test is included.

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
cubed3	🟢 Ready	View Preview	Sep 2, 2026, 12:04 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

[claude]

Claude finished @MazterQyou's task in 3m 54s —— View job

---

Review: looks correct, ships the right fix — 1 medium (guard broader than documented), 3 low (comment volume). No blockers.

Full review

• Gather context (read diff + surrounding code)
• Understand the request
• Review code quality / correctness
• Review tests
• Post review

Summary

The fix is well targeted. The root cause is real and clearly diagnosed: without a wrapper rule for the MAX(CASE WHEN m IS NULL THEN 1 ELSE 0 END) half, the aggregation can't push to Cube, the plan falls back to an ungrouped scan, and COUNT(DISTINCT ...) gets rebuilt as SQL over row-level values a countDistinct measure never renders — hence the silent 1-per-group. Folding that half to a literal 0 in the wrapper, exactly as aggregate-function-powerbi-count-distinct-max-case already does on the split path, is the minimal change that keeps the two pushdown paths agreeing.

Details that check out:

• The pattern requires WrapperReplacerContextPushToCube:true, and the output is a fully-resolved alias_expr(literal_int(0), ...) under a wrapper_pullup_replacer — correct shape, unlike the neighbouring filtered-measure rules which must keep a pushdown replacer inside because they still contain unresolved subtrees.
• The transform only decides scalar facts about pattern-bound nodes (measure agg type, output alias), per the rewrite-rule guidance in rust/cubesql/CLAUDE.md. member_name_to_expr access via index_mut matches pushdown_measure_impl.
• ?distinct left unconstrained on the Max is harmless (MAX(DISTINCT x) == MAX(x)), and consistent with the split rule.
• literal_int(0) needs no data-source template check — transform_literal in wrapper/literal.rs:111 accepts non-timestamp/non-interval literals unconditionally.
• The docs claim holds up: dropping the term subtracts at most 1 per group, so it can turn a tie into a strict order but never invert a strict one. [ref-count-distinct]: /reference/data-modeling/measures#type matches the anchor used elsewhere in the docs, and the link definition is in the right alphabetical slot.

Findings

Sev	Where	Issue
Medium	wrapper/aggregate.rs:1381	is_same_agg_type("countDistinct", false) also accepts countDistinctApprox and number, so the guard is wider than the "real distinct-counting measure" the doc comment describes; combined with no requirement that a sibling COUNT(DISTINCT) exists, a standalone MAX(CASE WHEN <number measure> IS NULL ...) folds to constant 0. Same as the pre-existing split rule, so path parity is preserved — but the comment should match the check, and the negative test should pin a number measure.
Low	split/aggregate_function.rs:275-296	22-line comment with an ASCII result table on a rule this PR doesn't change; the only load-bearing part is "keep in sync with the wrapper rule".
Low	wrapper/aggregate.rs:468-479	12-line comment whose second half narrates the bug this PR fixed.
Low	test_bi_workarounds.rs:59-69	The same approximation story is repeated in three doc comments, plus per-assert! comments that restate their own assertion messages.

Nits (not worth a thread)

• !sql.contains(r#""ungrouped": true"#) is a whitespace-sensitive negative substring assertion — it passes silently forever if serialization ever drops the space. It does follow existing convention (test_wrapper.rs:2019), but asserting on the typed request (find_cube_scan_wrapped_sql_deep().request.ungrouped, as at test_wrapper.rs:2004) would be sturdier for a negative check.
• test_powerbi_max_case_over_non_count_distinct_is_not_dropped would also pass on the old lossy fallback path, since MAX(CASE WHEN appears in the rebuilt outer SQL too. That's fine for what it claims, just weaker than it reads.

Not verified

I did not build or run the Rust tests — there's no target/ cache in this checkout and a cold cubesql build is far too slow for this job. Correctness claims above are from reading the rules and surrounding code; CI covers the rest.

• branch `cubesql/count-distinct-max-case-wrapper`

[codecov]

Codecov Report

❌ Patch coverage is 94.37229% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.67%. Comparing base (66210e3) to head (2c0775f).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...ql/cubesql/src/compile/test/test_bi_workarounds.rs	94.11%	9 Missing ⚠️
...sql/src/compile/rewrite/rules/wrapper/aggregate.rs	94.87%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11737       +/-   ##
===========================================
+ Coverage   59.08%   84.67%   +25.58%     
===========================================
  Files         237      261       +24     
  Lines       19022    86332    +67310     
  Branches     3865        0     -3865     
===========================================
+ Hits        11239    73098    +61859     
- Misses       7222    13234     +6012     
+ Partials      561        0      -561     

Flag	Coverage Δ
cube-backend	?
cubesql	84.67% <94.37%> (?)

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.

[claude]

Claude finished @MazterQyou's task in 1m 42s —— View job

---

Re-review after push: all four prior threads addressed, no new findings. Approve-equivalent (I can't formally approve).

Full review

• Fetch base and diff the updated branch
• Check which prior threads are now stale
• Review the current state of the change
• Post review

What changed since the last round

The four threads I opened were all acted on, so I've resolved them:

Prior thread	Resolution
wrapper/aggregate.rs:1379 — guard broader than the doc comment claims	Comment now states the truth outright: is_same_agg_type("countDistinct") accepts countDistinct, countDistinctApprox and number, and the paired COUNT(DISTINCT …) isn't required. Plus a new test, test_powerbi_max_case_over_number_measure_is_dropped, that pins the number case.
split/aggregate_function.rs — 22-line comment	Trimmed to 17 and the load-bearing "keep in sync with wrapper-push-down-powerbi-count-distinct-max-case" pointer is now explicit.
wrapper/aggregate.rs — 12-line rule comment	Down to 6; the bug narration is gone, the sync pointer stayed.
test_bi_workarounds.rs — repeated approximation story	Each doc comment now states only what its own test pins.

The earlier nit about !sql.contains(r#""ungrouped": true"#) being whitespace-sensitive is also gone — it's !sql.contains(r#""ungrouped""#) now, with a one-line note on why absence is the right check for a grouped request.

Verification of the new test

Fixture members check out against compile/test/mod.rs:

• MultiTypeCube.measure_num0 has agg_type: Some("number") (mod.rs:433-435) and MultiTypeCube.dim_str0 is a string dimension (mod.rs:415) — so test_powerbi_max_case_over_number_measure_is_dropped really does exercise the number branch of is_same_agg_type, which is exactly the widening the comment now documents.
• KibanaSampleDataEcommerce.maxPrice has agg_type: Some("max") (mod.rs:119-125) — genuinely rejected by the guard, so the negative test is sound. Its second assertion (!sql.contains(r#"\"sql\":\"0\""#)) also fixes the weakness I flagged, since that's the part the old lossy fallback path wouldn't produce either way.
• is_null_expr (rewrite/mod.rs:1878) and literal_int (rewrite/mod.rs:1964) both exist as pattern helpers.

Remaining, not pressing

The split-rule comment is still 17 lines including the ASCII result table. The table is illustrative rather than load-bearing — the sentence above it already says a NULL-holding group comes back one lower — but keeping a worked example for a deliberately-lossy rewrite is a defensible call, and the sync pointer I actually cared about is there. Not raising it again.

Not verified

Still haven't built or run the Rust tests — no target/ cache in this checkout and a cold cubesql build is too slow for this job. Everything above is from reading the rules and fixtures; CI covers the rest.

• branch `cubesql/count-distinct-max-case-wrapper`