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

.gitignore

@@ -5,3 +5,7 @@
 
 # Regenerated by tools/dag-viewer/generate-sample.sh; dag.example.json is the committed sample
 /tools/dag-viewer/dag.json
+
+# Python bytecode cache (tools/clickhouse/extract_functions.py)
+__pycache__/
+*.pyc

crates/frontend-sql/src/sql/mod.rs

@@ -1481,3 +1481,87 @@ fn lower_window_func_kind(fun: &WindowFunctionDefinition) -> Result<WindowFuncKi
         }
     }
 }
+
+// ── Issue #225, item 3: DataFusion registry drift detection ────────────────
+//
+// `asap_sql_function_catalog::NATIVE_FUNCTIONS` is hand-maintained data
+// mirroring what DataFusion's own aggregate-function registry resolves. That
+// mirror can only silently drift out of sync — a DataFusion version bump
+// that adds, renames, or removes a builtin aggregate leaves the catalog
+// looking fine while `lower_agg_intent` quietly gains or loses coverage. Of
+// the two introspectable sources the issue names, DataFusion's own registry
+// is the one with no external dependency: `SessionContext` already lists its
+// aggregate UDFs in-process, so the check below builds a real context the
+// same way `build_context` does and walks it directly — no live database,
+// no new CI infra, just `cargo test`. (ClickHouse's `system.functions` is
+// the other source; it needs a live ClickHouse instance, which is handled
+// separately by the dev-only `tools/clickhouse/extract_functions.py` script,
+// deliberately not wired into this test or into CI.)
+#[cfg(test)]
+mod catalog_drift {
+    use super::*;
+
+    /// Every aggregate function name DataFusion's planner resolves inside a
+    /// context built the same way `build_context` builds one must be
+    /// *accounted for* by the catalog: either `lookup_native` maps it to a
+    /// canonical semantic, it is one of our own `CLICKHOUSE_BUILTINS` stub
+    /// registrations (`build_context` registers those into the very same
+    /// context, so they show up here too), or it is explicitly listed in
+    /// `KNOWN_UNMAPPED_NATIVE_FUNCTIONS` with a reason.
+    ///
+    /// This does *not* assert the reverse (that every `NATIVE_FUNCTIONS`
+    /// entry is resolvable) — a name that stops resolving after a DataFusion
+    /// bump just becomes permanently unreachable dead data, not a lowering
+    /// hazard, so it's out of scope for a regression gate. It also does not
+    /// try to derive `AggSemantic` from anything DataFusion reports — that
+    /// judgment call stays with whoever adds the catalog entry.
+    #[test]
+    fn every_datafusion_aggregate_name_is_covered_by_the_catalog() {
+        let catalog = SqlCatalog::new();
+        let ctx = SqlLowerer::new(&catalog)
+            .build_context()
+            .expect("build_context with an empty table catalog cannot fail");
+        let state = ctx.state();
+        let mut uncovered: Vec<&str> = state
+            .aggregate_functions()
+            .keys()
+            .map(String::as_str)
+            .filter(|name| {
+                asap_sql_function_catalog::lookup_native(name).is_none()
+                    && asap_sql_function_catalog::lookup_clickhouse_builtin(name).is_none()
+                    && !asap_sql_function_catalog::KNOWN_UNMAPPED_NATIVE_FUNCTIONS.contains(name)
+            })
+            .collect();
+        uncovered.sort_unstable();
+        assert!(
+            uncovered.is_empty(),
+            "DataFusion resolves these aggregate names but the catalog doesn't know about them \
+             (crates/sql-function-catalog/src/lib.rs): {uncovered:?}\n\
+             Either add a `NativeFunction` entry mapping each to its `AggSemantic`, or -- if it's \
+             a deliberate non-goal (no `AggIntent` shape for it, or it's rejected elsewhere) -- \
+             add it to `KNOWN_UNMAPPED_NATIVE_FUNCTIONS` with a reason. This usually means a \
+             DataFusion version bump added or renamed a builtin aggregate."
+        );
+    }
+
+    /// Every `KNOWN_UNMAPPED_NATIVE_FUNCTIONS` entry earns its place by
+    /// actually being a name DataFusion resolves today — otherwise it is
+    /// stale documentation for a name that no longer exists (e.g. a prior
+    /// DataFusion version renamed it), not a real "deliberately not mapped"
+    /// decision, and should be removed.
+    #[test]
+    fn known_unmapped_entries_are_all_real_datafusion_names() {
+        let catalog = SqlCatalog::new();
+        let ctx = SqlLowerer::new(&catalog)
+            .build_context()
+            .expect("build_context with an empty table catalog cannot fail");
+        let resolved = ctx.state().aggregate_functions().clone();
+        for name in asap_sql_function_catalog::KNOWN_UNMAPPED_NATIVE_FUNCTIONS {
+            assert!(
+                resolved.contains_key(*name),
+                "`{name}` is listed in KNOWN_UNMAPPED_NATIVE_FUNCTIONS but DataFusion no longer \
+                 resolves it -- remove the stale entry"
+            );
+        }
+    }
+}

crates/sql-function-catalog/src/lib.rs

@@ -254,6 +254,78 @@ pub const CLICKHOUSE_BUILTINS: &[ClickHouseBuiltin] = &[
     },
 ];
 
+/// Aggregate function names DataFusion's own planner resolves out of the box
+/// that this catalog deliberately does *not* map to a canonical
+/// [`AggSemantic`] -- either because `AggIntent` has no shape for them
+/// (a multi-column correlation/regression aggregate, a bitwise/boolean
+/// aggregate, a string concatenation aggregate, ...) or because this front
+/// end already rejects them explicitly elsewhere (`array_agg`, `grouping`).
+///
+/// This list exists for one reason: `asap-frontend-sql`'s DataFusion-registry
+/// drift test (issue #225, item 3 -- see
+/// `asap_frontend_sql::sql::catalog_drift`) walks a real `SessionContext`'s
+/// resolved aggregate names and requires every one to be either in
+/// [`NATIVE_FUNCTIONS`], a [`CLICKHOUSE_BUILTINS`] name, or listed here. A
+/// name landing here is a *recorded decision*, not a silenced test failure --
+/// each group below says why. Do not add an entry just to make the test
+/// pass; add a `NativeFunction` instead if the name should actually lower.
+///
+/// Two known, narrow gaps rather than a deliberate non-goal: `var_sample` and
+/// `var_population` are DataFusion's own alias spellings of `var_samp` /
+/// `var_pop` (see `variance.rs`'s `aliases()` in `datafusion-functions-
+/// aggregate`) that [`NATIVE_FUNCTIONS`] doesn't also list under those
+/// spellings. Surfaced here rather than silently added to
+/// [`NATIVE_FUNCTIONS`], since accepting a new spelling is a maintainer's
+/// call, not something this catalog should do on its own.
+pub const KNOWN_UNMAPPED_NATIVE_FUNCTIONS: &[&str] = &[
+    // Selector aggregates: this front end only supports these as window
+    // functions (`WindowFuncKind::FirstValue`/`LastValue`/`NthValue`, via
+    // `OVER (...)`), not as plain `GROUP BY` aggregates -- no `AggIntent`
+    // variant models "the value from a particular row" as a reduction.
+    "first_value",
+    "last_value",
+    "nth_value",
+    // Bitwise / boolean aggregates -- no corresponding `AggIntent` variant.
+    "bit_and",
+    "bit_or",
+    "bit_xor",
+    "bool_and",
+    "bool_or",
+    // Two-column correlation / linear-regression aggregates -- every
+    // `AggIntent` value reducer takes one input column (`reducer_col` in
+    // `asap-frontend-sql`), so these have no home yet.
+    "corr",
+    "covar",
+    "covar_pop",
+    "covar_samp",
+    "regr_avgx",
+    "regr_avgy",
+    "regr_count",
+    "regr_intercept",
+    "regr_r2",
+    "regr_slope",
+    "regr_sxx",
+    "regr_sxy",
+    "regr_syy",
+    // String concatenation -- no `AggIntent` equivalent.
+    "string_agg",
+    // The weighted-percentile variant of `approx_percentile_cont` (an extra
+    // weight-column argument); only the unweighted form is in
+    // `NATIVE_FUNCTIONS`.
+    "approx_percentile_cont_with_weight",
+    // Explicitly rejected elsewhere, not merely unmapped:
+    // `array_agg_is_deliberately_rejected` (asap-frontend-sql's
+    // sql_lowering tests) covers `array_agg`; `lower_grouping_sets`'s own
+    // doc comment covers `grouping` (`GROUPING(col)` -- observable only via
+    // the `__grouping_id` discriminator this front end drops).
+    "array_agg",
+    "grouping",
+    // Known narrow gaps (see doc comment above) -- alias spellings of
+    // `var_samp` / `var_pop` this catalog doesn't accept yet.
+    "var_sample",
+    "var_population",
+];
+
 /// Look up a native function name's canonical semantic (case-sensitive --
 /// callers normalize case first, as `asap-frontend-sql` already does via
 /// `.to_lowercase()`).
@@ -316,4 +388,38 @@ mod tests {
             );
         }
     }
+
+    /// `KNOWN_UNMAPPED_NATIVE_FUNCTIONS` documents names this catalog
+    /// deliberately does *not* map -- it should never overlap with a table
+    /// that *does* map the same name (that would be a contradiction: mapped
+    /// and "known unmapped" at once), and shouldn't repeat itself either
+    /// (each name is a one-time recorded decision).
+    #[test]
+    fn known_unmapped_list_is_disjoint_from_the_mapped_tables_and_has_no_duplicates() {
+        for name in KNOWN_UNMAPPED_NATIVE_FUNCTIONS {
+            assert!(
+                lookup_native(name).is_none(),
+                "{name} is both in NATIVE_FUNCTIONS and KNOWN_UNMAPPED_NATIVE_FUNCTIONS"
+            );
+            assert!(
+                lookup_clickhouse_builtin(name).is_none(),
+                "{name} is both in CLICKHOUSE_BUILTINS and KNOWN_UNMAPPED_NATIVE_FUNCTIONS"
+            );
+        }
+        let mut sorted = KNOWN_UNMAPPED_NATIVE_FUNCTIONS.to_vec();
+        sorted.sort_unstable();
+        sorted.dedup();
+        assert_eq!(
+            sorted.len(),
+            KNOWN_UNMAPPED_NATIVE_FUNCTIONS.len(),
+            "KNOWN_UNMAPPED_NATIVE_FUNCTIONS has a duplicate entry"
+        );
+    }
+
+    #[test]
+    fn known_unmapped_names_are_lowercase() {
+        for name in KNOWN_UNMAPPED_NATIVE_FUNCTIONS {
+            assert_eq!(*name, name.to_lowercase(), "not lowercase: {name}");
+        }
+    }
 }

tools/clickhouse/README.md

@@ -0,0 +1,62 @@
+# ClickHouse function-catalog extraction (issue #225, item 3)
+
+`extract_functions.py` is a **dev-only, manual discovery tool**. It compares
+ClickHouse's own aggregate-function list (`system.functions`) against
+[`asap_sql_function_catalog::CLICKHOUSE_BUILTINS`](../../crates/sql-function-catalog/src/lib.rs)
+and reports:
+
+- **candidates** -- ClickHouse aggregate names not yet in `CLICKHOUSE_BUILTINS`
+  (things worth considering adding),
+- **possibly stale** -- `CLICKHOUSE_BUILTINS` entries that no longer appear in
+  ClickHouse's own list (e.g. renamed or removed upstream), and
+- **combinator-derived** -- catalog entries like `countif` that are really a
+  base function (`count`) plus one of ClickHouse's
+  [aggregate combinators](https://clickhouse.com/docs/en/sql-reference/aggregate-functions/combinators)
+  (`-If`, `-Distinct`, `-Array`, ...), which ClickHouse doesn't enumerate as
+  its own `system.functions` row -- reported separately so it isn't
+  misreported as stale.
+
+It **only reports**. It never edits `crates/sql-function-catalog/src/lib.rs` --
+existence (+ arity) is all issue #225 asks the catalog to track, and deciding
+each new entry's `RewriteKind` / canonical semantic is a human judgment call,
+not something this script attempts.
+
+## Why this isn't wired into CI
+
+ClickHouse's function surface can only be introspected against an actual
+ClickHouse (`system.functions` isn't documented data, it's a live catalog
+table). This repo has no ClickHouse service anywhere in CI, and the project
+decided deliberately not to add one just for this — see issue #225's own
+"needs a decision on where such extraction tooling would actually run" open
+question. So: a maintainer runs this locally, by hand, whenever they want to
+check for drift or discover new candidates. It's never invoked automatically.
+
+## Running it
+
+This tool uses [`chdb`](https://github.com/chdb-io/chdb) -- ClickHouse
+embedded as an in-process Python library. There's no server to start:
+
+```sh
+pip install chdb
+python3 tools/clickhouse/extract_functions.py
+```
+
+Optional flags:
+
+- `--repo-root PATH` -- repo root containing `crates/sql-function-catalog`
+  (default: inferred from this script's own location).
+- `--output PATH` -- write the report to a file instead of stdout.
+
+Because `chdb` runs ClickHouse in-process rather than connecting to a
+server, the report reflects whatever ClickHouse version the installed
+`chdb` package embeds. `pip install --upgrade chdb` to check against a
+newer ClickHouse release.
+
+## Running its tests
+
+The catalog-parsing and diff logic (everything except the actual `chdb`
+query) has plain `unittest` coverage that needs no ClickHouse at all:
+
+```sh
+cd tools/clickhouse && python3 -m unittest test_extract_functions -v
+```