feat: rewrite uniqExact to COUNT(DISTINCT ...) - #221
Merged
Merged
Conversation
…ion Analyzer uniqExact(x) has no DataFusion planner equivalent, so it always failed at "unknown function". Register a stub UDAF so the name resolves, then a FunctionRewrite that turns every call into count(x) DISTINCT before lower_plan runs -- lower_agg_intent already maps that to AggIntent::Cardinality, so it needs no ClickHouse-specific name of its own. Making the rewrite fire requires running DataFusion's Analyzer (a FunctionRewrite only applies as part of one), which the SQL front end previously skipped entirely. Scoped to a standalone zero-rule Analyzer carrying only this rewrite, not the default 5-rule one, to avoid pulling in unrelated normalization (TypeCoercion, ExpandWildcardRule, ...) for every SQL query in the system. One unavoidable side effect: the Analyzer's unconditional post-check now rejects a multi-column IN (subquery) before lower_in_subquery's own arity check runs, with a different error message (same rejection either way) -- one test updated to match. bgp_jan2024_workload corpus: Lowered 64 -> 85, Plan 105 (was 127). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_and_check Analyzer::execute_and_check (even with an otherwise-empty rule set) runs an unconditional post-check baked into that method, not gated by which rules are registered -- it independently rejected a multi-column IN (subquery) before lower_in_subquery's own arity check got a chance to. ApplyFunctionRewrites is the actual AnalyzerRule DataFusion's own Analyzer uses to apply FunctionRewrites; its analyze() already does a full transform_up_with_subqueries over the whole plan, so it needs no wrapping Analyzer at all. Calling it directly skips that unconditional check entirely: zero behavior change for every query that doesn't call uniqExact. Reverts the sql_lowering test for the multi-column IN (subquery) case, now back to its original assertion. Coverage unchanged (bgp_jan2024_workload: Lowered 85, Plan 105). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
milindsrivastava1997
marked this pull request as ready for review
August 21, 2026 21:15
zzylol
approved these changes
Aug 22, 2026
zzylol
added a commit
that referenced
this pull request
Aug 22, 2026
…f rewrite (#227) Pulls the SQL aggregate-function name -> canonical AggIntent mapping out of frontend-sql::lower_agg_intent's hand-written match and into a new independent crate, asap-sql-function-catalog (issue #225), structured after tobilg/polyglot's polyglot-sql-function-catalogs: flat data (name, arity, canonical semantic), no dependency on the SQL front end or DataFusion. lower_agg_intent keeps only call-site logic that isn't a function of the name alone (the DISTINCT rule, the bare-column reducer-argument rule, phi extraction, the ambient AccuracyTarget). Also generalizes uniqExact's stub-UDAF + FunctionRewrite mechanism (#221) so a new ClickHouse-only builtin DataFusion doesn't know at all is a catalog data entry (name, arity, RewriteKind) rather than a new one-off Rust type and a new one-off stub-UDAF function. uniqExact and the new countIf entry both go through the single ClickHouseBuiltinRewrite + clickhouse_builtin_stub_udaf mechanism now. countIf(cond) rewrites to sum(CASE WHEN cond THEN 1 ELSE 0 END) rather than count(...) FILTER (WHERE cond), since AggIntent::Count never consults its argument (it always means "row count") and would silently drop the filter; summing a 0/1 indicator keeps cond observable through the existing Sum path, including the general non-column-argument materialization lower_aggregate already does (#110). uniqExact's own behavior is unchanged (same signature, same rewrite target) and its existing corpus tests (bgp_analytics, #221) pass unmodified. countIf support moves 13 bgp_jan2024_workload corpus queries out of Category::Plan (12 now lower end to end, one hits a second, pre-existing NotImplemented gap) -- the pinned tally is updated accordingly. New direct unit tests cover countIf's derived-column shape and composition with GROUP BY. Deliberately out of scope (per the issue): generating the catalog from a live ClickHouse/DataFusion introspection source (item 3 -- flagged in the issue as needing a decision on where such tooling would run). Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
uniqExact(x)has no DataFusion planner equivalent and always failed at "unknown function". Register a stub UDAF plus aFunctionRewritethat turns every call intocount(x) DISTINCTbeforelower_planruns —lower_agg_intentalready maps that toAggIntent::Cardinality, so it needs no ClickHouse-specific name.ApplyFunctionRewritesrule becauseAnalyzer::execute_and_checkwill call other rules that are currently not needed.Coverage
bgp_jan2024_workload(200-query corpus):Lowered64 → 85,Plan105 (was 127).bgp_analyticsqueries 6 and 12 now lower end to end.Known limitations (not fixed here)
uniqExact(a, b, c)isn't handled — the stub UDAF's signature accepts exactly one arg.uniqExactand SQLCOUNT(DISTINCT x)wasn't verified.Test plan
cargo test -p asap-frontend-sql(all suites green, zero test changes needed beyond the pinned-coverage corpus tallies)cargo test --workspace(green)cargo clippy -p asap-frontend-sql --all-targets(clean)cargo fmt -p asap-frontend-sql -- --check(clean)🤖 Generated with Claude Code