Skip to content

feat(sql): generate-vs-hand-maintain follow-up for the function catalog (#225) - #233

Merged
zzylol merged 1 commit into
mainfrom
sql-function-catalog-generation
Aug 22, 2026
Merged

zzylol merged 1 commit into
mainfrom
sql-function-catalog-generation

Conversation

@zzylol

@zzylol zzylol commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Part of #225 -- completes item 3 of its "Proposed direction" ("Where
feasible, generate rather than hand-maintain"), which #227 deliberately left
undone. Stacked on top of #227 (feat/sql-function-catalog-225): this PR's
base is that branch, not main, since asap-sql-function-catalog doesn't
exist on main yet.

Issue #225 names two introspectable sources and treats them differently:

  1. DataFusion's own aggregate-function registry -- introspectable
    in-process, no external dependency, no infra decision needed.
  2. ClickHouse's system.functions -- needs a live/embedded ClickHouse,
    which this repo has no CI infra for.

1. DataFusion registry drift detection (real, CI-exercised Rust)

crates/frontend-sql/src/sql/mod.rs gets two new tests (mod catalog_drift)
that build a real SessionContext the same way SqlLowerer::build_context()
does and:

  • assert every aggregate name it resolves is covered by
    asap_sql_function_catalog::NATIVE_FUNCTIONS, is one of the catalog's own
    CLICKHOUSE_BUILTINS stub registrations, or is on a new, documented
    KNOWN_UNMAPPED_NATIVE_FUNCTIONS allow-list (each entry has a reason:
    no AggIntent shape yet, or explicitly rejected elsewhere);
  • assert every KNOWN_UNMAPPED_NATIVE_FUNCTIONS entry is still a real
    DataFusion name (so the allow-list itself can't quietly rot).

This turns a future DataFusion version bump that adds/renames/removes a
builtin aggregate into a cargo test failure instead of silent catalog
drift -- no new CI infra, it's part of the existing cargo test --workspace.
I verified the test actually catches drift by temporarily removing an entry
from the allow-list and confirming it fails with a clear message, then
restored it.

Along the way this surfaced two small, pre-existing gaps that the old
hand-written match never had a way to reveal: DataFusion resolves
var_sample / var_population (its own alias spellings of var_samp /
var_pop) but NATIVE_FUNCTIONS doesn't list those spellings. Rather than
silently "fixing" that in this PR, they're recorded on
KNOWN_UNMAPPED_NATIVE_FUNCTIONS with a comment -- accepting a new spelling
is a maintainer's call, and surfacing it rather than deciding it is the
point of this tooling.

2. ClickHouse builtin discovery (dev-only, not CI-wired)

tools/clickhouse/extract_functions.py diffs ClickHouse's own
system.functions (aggregate functions only) against
CLICKHOUSE_BUILTINS and reports:

  • candidates -- ClickHouse aggregate names not yet in the catalog,
  • possibly stale -- catalog entries ClickHouse's list no longer has,
  • combinator-derived -- catalog entries like countif that are really
    a base function (count) plus one of ClickHouse's aggregate combinators
    (-If, -Distinct, -Array, ...), which ClickHouse doesn't enumerate as
    its own system.functions row -- so a naive diff would misreport these as
    stale every run; they're called out separately instead.

It uses chdb (ClickHouse embedded
in-process) rather than a live server -- pip install chdb is the only
setup step, no Docker, no network service. tools/clickhouse/README.md
covers how to run it, and explicitly documents why it's not wired into
CI (no ClickHouse service anywhere in this repo's CI, and that's a
deliberate decision, not an oversight).

It never touches crates/sql-function-catalog/src/lib.rs -- it's a
reporting tool for a human to act on, matching the issue's own scope
(existence + arity only; RewriteKind/semantic is a human judgment call).

Explicitly out of scope (per the issue and this follow-up's own scope)

Test plan

  • cargo build --workspace --all-targets
  • cargo test --workspace (all green, including the two new
    catalog_drift tests)
  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • Manually verified the drift test fails with a clear message when a
    catalog entry is removed, confirming it actually detects drift
  • tools/clickhouse/extract_functions.py run end-to-end against real
    embedded ClickHouse via chdb (installed successfully in this
    sandbox) -- produced a correct, sensible diff report, including
    correctly classifying countif as combinator-derived rather than
    falsely "stale"
  • python3 -m unittest tools/clickhouse/test_extract_functions.py
    (16 tests, all passing) -- covers the catalog-parsing and diff logic
    without needing ClickHouse at all

🤖 Generated with Claude Code

Base automatically changed from feat/sql-function-catalog-225 to main August 22, 2026 20:51
…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>
@zzylol
zzylol force-pushed the sql-function-catalog-generation branch from 564462d to e989bd7 Compare August 22, 2026 20:56
@zzylol
zzylol merged commit b5d58d5 into main Aug 22, 2026
3 checks passed
@zzylol
zzylol deleted the sql-function-catalog-generation branch August 22, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant