feat(sql): extract function-name catalog, generalize uniqExact/countIf rewrite - #227
Merged
Merged
Conversation
…f rewrite 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 was referenced Aug 22, 2026
zzylol
added a commit
that referenced
this pull request
Aug 22, 2026
…og (#225) Rebased onto main now that #227 (the catalog crate + generalized ClickHouse-builtin mechanism) and #226 (the QueryExpr rename) have landed there — this PR's own content is unchanged, just reapplied cleanly on top of current main instead of the now-deleted feat/sql-function-catalog-225 branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
zzylol
added a commit
that referenced
this pull request
Aug 22, 2026
Rebased onto main now that #226 (the QueryExpr rename) and #227 (the SQL function catalog) have landed there. This PR's own design and content are unchanged; adapted the new cse.rs module's exhaustive QueryExpr match to the current post-rename variant names (Merge -> Concat, Distinct -> Dedup, Scalar -> PromqlScalar, EvalTime -> QueryTimestamp, WindowFunc -> SQLWindowFunc, Arith -> Arithmetic, VectorFromScalar/ScalarFromVector/Relabel/InfoJoin/Sample/Subquery -> their Promql*-prefixed names, ArithOp/CompareOp -> ArithmeticOpKind/ CompareOpKind) and reapplied mod.rs's doc/pub-use additions around the renamed expr_ir re-export. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Aug 22, 2026
#235) Rebased onto main now that #226 (the QueryExpr rename) and #227 (the SQL function catalog) have landed there. This PR's own design and content are unchanged; adapted the new cse.rs module's exhaustive QueryExpr match to the current post-rename variant names (Merge -> Concat, Distinct -> Dedup, Scalar -> PromqlScalar, EvalTime -> QueryTimestamp, WindowFunc -> SQLWindowFunc, Arith -> Arithmetic, VectorFromScalar/ScalarFromVector/Relabel/InfoJoin/Sample/Subquery -> their Promql*-prefixed names, ArithOp/CompareOp -> ArithmeticOpKind/ CompareOpKind) and reapplied mod.rs's doc/pub-use additions around the renamed expr_ir re-export. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Aug 22, 2026
…og (#225) (#233) Rebased onto main now that #227 (the catalog crate + generalized ClickHouse-builtin mechanism) and #226 (the QueryExpr rename) have landed there — this PR's own content is unchanged, just reapplied cleanly on top of current main instead of the now-deleted feat/sql-function-catalog-225 branch. 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
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
crates/sql-function-catalog(packageasap-sql-function-catalog), holding the SQL aggregate-function name → canonical semantic mappingfrontend-sql::lower_agg_intentused to hand-match. Design lifted fromtobilg/polyglot'spolyglot-sql-function-catalogs: flat per-function data (name, arity, canonicalAggSemantic), with no dependency onasap-frontend-sqlor DataFusion —asap-frontend-sqldepends on it, not the other way around.lower_agg_intentkeeps only the call-site logic that isn't a function of the name alone: the DISTINCT modifier rule, the "reducer argument must be a bare column" rule (reducer_col), φ extraction from a literal argument, and the ambientAccuracyTarget.uniqExact's stub-AggregateUDF+FunctionRewritemechanism (feat: rewrite uniqExact to COUNT(DISTINCT ...) #221) so a new ClickHouse-only builtin DataFusion doesn't know at all is a catalog data entry (name, arity,RewriteKind), not a new one-off Rust type plus a new one-off stub-UDAF function.uniqExactand a newcountIfentry both go through oneClickHouseBuiltinRewrite+clickhouse_builtin_stub_udafmechanism now.countIf(cond)rewrites tosum(CASE WHEN cond THEN 1 ELSE 0 END), notcount(...) FILTER (WHERE cond):AggIntent::Countnever consults its argument (it always means "row count"), so a plain filtered count would silently drop the filter. Summing a 0/1 indicator keepscondobservable through the existingSumpath, including the general non-column-argument materializationlower_aggregatealready does (SQL: expression GROUP BY (date_trunc time bucketing), aggregates over expressions, and GROUPING SETS/ROLLUP/CUBE are rejected #110) — no newAggIntentvariant or lowering path needed.Design choices
asap-sql-function-catalog, two flat const tables —NATIVE_FUNCTIONS(names DataFusion's planner already resolves, mapped toAggSemantic) andCLICKHOUSE_BUILTINS(ClickHouse-only names, each carrying aRewriteKind). Hand-maintained Rust consts; the module doc notes that generating them from a live ClickHouse/DataFusion introspection source is a documented follow-up (issue SQL: extract dialect builtin-function catalog into independent, generatable data (inspired by polyglot-sql-function-catalogs) #225, item 3 — explicitly flagged in the issue as needing a decision on where such tooling would run, not built here).AggSemanticis deliberately notasap_types::pre_asap::agg_intent::AggIntentitself — mostAggIntentvariants carry call-site-only state (ambient accuracy, φ from a literal arg, DISTINCT) that isn't a function of the name alone.AggSemanticis just the per-name discriminant those call sites key off.countIfproves the generalization: it reusesClickHouseBuiltinRewrite/clickhouse_builtin_stub_udafwith zero new Rust types, adding only a catalog entry and one newRewriteKind::CountIfToSummatch arm.Behavior / test impact
uniqExact's behavior is unchanged (same stub signature, same rewrite target); its existing corpus tests (bgp_analytics, from feat: rewrite uniqExact to COUNT(DISTINCT ...) #221) pass unmodified.countIfsupport moves 13bgp_jan2024_workloadcorpus queries out ofCategory::Plan(12 now lower end to end, one hits a second, pre-existingNotImplementedgap — map/array index access) — the pinned tally in that test is updated accordingly, per its own documented ratchet convention.sql_lowering.rscovercountIf's derived-indicator-column shape, multiple distinctcountIfcalls in one aggregate, and composition withGROUP BY.Out of scope (per the issue)
system.functions/ DataFusion UDF registry introspection (issue SQL: extract dialect builtin-function catalog into independent, generatable data (inspired by polyglot-sql-function-catalogs) #225, item 3).ElasticSQLhaving no vendored parser.Test plan
cargo build --workspace --all-targetscargo test --workspace(all green)cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningsCloses #225