Skip to content

SQL: extract dialect builtin-function catalog into independent, generatable data (inspired by polyglot-sql-function-catalogs) #225

Description

@zzylol

Summary

Pull the "SQL dialect builtin function name (+ arity) → canonical AggIntent"
mapping out of the hand-written match in frontend-sql::lower_agg_intent
and into its own small, independently-maintained (ideally generated) catalog
— inspired by tobilg/polyglot's
polyglot-sql-function-catalogs crate, which does exactly this for its
30+ SQL dialects: per-dialect data (function name, arity/overloads, casing),
deliberately not depending on the core parser crate (exposed instead via a
small CatalogSink trait the consumer implements), populated by extraction
tooling that introspects real databases rather than hand-maintained.

Where the pain shows up today

lower_agg_intent (crates/frontend-sql/src/sql/mod.rs) is a single
hand-written match from a DataFusion-resolved function name to an
AggIntent:

Ok(match name.as_str() {
    "count" if agg_fn.distinct => AggIntent::Cardinality { .. },
    "count" => AggIntent::Count { .. },
    "sum" => AggIntent::Sum { .. },
    ...
    "approx_percentile_cont" | "percentile_cont" => AggIntent::Quantile { .. },
    "median" | "approx_median" => AggIntent::Quantile { q: 0.5, .. },
    "approx_distinct" => AggIntent::Cardinality { .. },
    _ => return Err(LoweringError::UnsupportedAggregate(name)),
})

This covers names DataFusion itself resolves. A dialect builtin DataFusion
doesn't know at all is a strictly bigger problem — the uniqExact case
(#221) needed, per its own doc comments:

  1. A stub AggregateUDF registered into the SessionContext purely so
    DataFusion's planner doesn't reject the unknown function name while
    parsing (uniq_exact_udaf() — its accumulator is literally
    unimplemented!(), dead code by construction).
  2. A custom FunctionRewrite (UniqExactRewrite) rewriting every
    uniqexact(x) call to DataFusion's own count(x) DISTINCT before
    lower_agg_intent ever sees it, wired in by calling DataFusion's
    ApplyFunctionRewrites analyzer rule directly (not through
    Analyzer::execute_and_check, which runs an unwanted post-check) — a
    dozen lines of doc comment just to justify how it's invoked.

That's a meaningful amount of DataFusion-internals engineering for one
ClickHouse builtin. with_dialect's own doc comment already names another
still-unhandled one (countIf), and there's no reason to expect the list
stops there — ClickHouse's builtin function surface is large.

Proposed direction

  1. Separate the data from the code that consumes it. A catalog module
    (or crate, if it grows) mapping (dialect, function_name) → (arity, canonical semantic), structured close to polyglot-sql-function-catalogs's
    shape: existence + arity/overloads + casing is enough — no need to model
    full type signatures, matching that project's own deliberately shallow
    scope. It should not depend on asap-frontend-sql; asap-frontend-sql
    depends on it (or reads it through a small sink-style trait), keeping the
    dependency arrow pointing the way the rest of the workspace already does
    (asap-types "depends on nothing").
  2. Generalize the stub-UDAF-plus-FunctionRewrite mechanism so adding a
    new ClickHouse-only builtin is a catalog entry, not a new Rust type
    implementing FunctionRewrite plus a new stub-UDAF function. uniqExact
    is the shape every future one of these will take: register a stub so
    DataFusion's planner accepts the call, rewrite it to something DataFusion
    natively understands before lower_agg_intent runs.
  3. Where feasible, generate rather than hand-maintain, the way
    tools/clickhouse/extract_functions.py /
    tools/duckdb/extract_functions.py do in the reference project —
    ClickHouse exposes system.functions; DataFusion's own function
    registry is introspectable in-process (SessionContext's UDF/UDAF
    lists). Needs a decision on where such extraction tooling would actually
    run (a live ClickHouse instance in CI/dev is a real requirement, not a
    given) — flagging as an open question rather than assuming it's free.

Explicitly out of scope

  • ElasticSQL having no vendored parser at all — a different, bigger
    gap (no parser, not just missing functions); polyglot doesn't cover
    Elasticsearch SQL either, so it offers no shortcut there. Separate issue
    if/when that gets picked up.
  • Modeling argument types / return types / coercion — matching
    polyglot-sql-function-catalogs's own choice to leave these out; our
    AggIntent construction doesn't need them either (it already rejects a
    non-column argument explicitly, reducer_col).

Related

Activity

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

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions