Skip to content

Commit e989bd7

Browse files
zzylolclaude
andcommitted
feat(sql): generate-vs-hand-maintain follow-up for the function catalog (#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>
1 parent c1de25b commit e989bd7

6 files changed

Lines changed: 646 additions & 0 deletions

File tree

‎.gitignore‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,7 @@
55

66
# Regenerated by tools/dag-viewer/generate-sample.sh; dag.example.json is the committed sample
77
/tools/dag-viewer/dag.json
8+
9+
# Python bytecode cache (tools/clickhouse/extract_functions.py)
10+
__pycache__/
11+
*.pyc

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

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1481,3 +1481,87 @@ fn lower_window_func_kind(fun: &WindowFunctionDefinition) -> Result<WindowFuncKi
14811481
}
14821482
}
14831483
}
1484+
1485+
// ── Issue #225, item 3: DataFusion registry drift detection ────────────────
1486+
//
1487+
// `asap_sql_function_catalog::NATIVE_FUNCTIONS` is hand-maintained data
1488+
// mirroring what DataFusion's own aggregate-function registry resolves. That
1489+
// mirror can only silently drift out of sync — a DataFusion version bump
1490+
// that adds, renames, or removes a builtin aggregate leaves the catalog
1491+
// looking fine while `lower_agg_intent` quietly gains or loses coverage. Of
1492+
// the two introspectable sources the issue names, DataFusion's own registry
1493+
// is the one with no external dependency: `SessionContext` already lists its
1494+
// aggregate UDFs in-process, so the check below builds a real context the
1495+
// same way `build_context` does and walks it directly — no live database,
1496+
// no new CI infra, just `cargo test`. (ClickHouse's `system.functions` is
1497+
// the other source; it needs a live ClickHouse instance, which is handled
1498+
// separately by the dev-only `tools/clickhouse/extract_functions.py` script,
1499+
// deliberately not wired into this test or into CI.)
1500+
#[cfg(test)]
1501+
mod catalog_drift {
1502+
use super::*;
1503+
1504+
/// Every aggregate function name DataFusion's planner resolves inside a
1505+
/// context built the same way `build_context` builds one must be
1506+
/// *accounted for* by the catalog: either `lookup_native` maps it to a
1507+
/// canonical semantic, it is one of our own `CLICKHOUSE_BUILTINS` stub
1508+
/// registrations (`build_context` registers those into the very same
1509+
/// context, so they show up here too), or it is explicitly listed in
1510+
/// `KNOWN_UNMAPPED_NATIVE_FUNCTIONS` with a reason.
1511+
///
1512+
/// This does *not* assert the reverse (that every `NATIVE_FUNCTIONS`
1513+
/// entry is resolvable) — a name that stops resolving after a DataFusion
1514+
/// bump just becomes permanently unreachable dead data, not a lowering
1515+
/// hazard, so it's out of scope for a regression gate. It also does not
1516+
/// try to derive `AggSemantic` from anything DataFusion reports — that
1517+
/// judgment call stays with whoever adds the catalog entry.
1518+
#[test]
1519+
fn every_datafusion_aggregate_name_is_covered_by_the_catalog() {
1520+
let catalog = SqlCatalog::new();
1521+
let ctx = SqlLowerer::new(&catalog)
1522+
.build_context()
1523+
.expect("build_context with an empty table catalog cannot fail");
1524+
let state = ctx.state();
1525+
let mut uncovered: Vec<&str> = state
1526+
.aggregate_functions()
1527+
.keys()
1528+
.map(String::as_str)
1529+
.filter(|name| {
1530+
asap_sql_function_catalog::lookup_native(name).is_none()
1531+
&& asap_sql_function_catalog::lookup_clickhouse_builtin(name).is_none()
1532+
&& !asap_sql_function_catalog::KNOWN_UNMAPPED_NATIVE_FUNCTIONS.contains(name)
1533+
})
1534+
.collect();
1535+
uncovered.sort_unstable();
1536+
assert!(
1537+
uncovered.is_empty(),
1538+
"DataFusion resolves these aggregate names but the catalog doesn't know about them \
1539+
(crates/sql-function-catalog/src/lib.rs): {uncovered:?}\n\
1540+
Either add a `NativeFunction` entry mapping each to its `AggSemantic`, or -- if it's \
1541+
a deliberate non-goal (no `AggIntent` shape for it, or it's rejected elsewhere) -- \
1542+
add it to `KNOWN_UNMAPPED_NATIVE_FUNCTIONS` with a reason. This usually means a \
1543+
DataFusion version bump added or renamed a builtin aggregate."
1544+
);
1545+
}
1546+
1547+
/// Every `KNOWN_UNMAPPED_NATIVE_FUNCTIONS` entry earns its place by
1548+
/// actually being a name DataFusion resolves today — otherwise it is
1549+
/// stale documentation for a name that no longer exists (e.g. a prior
1550+
/// DataFusion version renamed it), not a real "deliberately not mapped"
1551+
/// decision, and should be removed.
1552+
#[test]
1553+
fn known_unmapped_entries_are_all_real_datafusion_names() {
1554+
let catalog = SqlCatalog::new();
1555+
let ctx = SqlLowerer::new(&catalog)
1556+
.build_context()
1557+
.expect("build_context with an empty table catalog cannot fail");
1558+
let resolved = ctx.state().aggregate_functions().clone();
1559+
for name in asap_sql_function_catalog::KNOWN_UNMAPPED_NATIVE_FUNCTIONS {
1560+
assert!(
1561+
resolved.contains_key(*name),
1562+
"`{name}` is listed in KNOWN_UNMAPPED_NATIVE_FUNCTIONS but DataFusion no longer \
1563+
resolves it -- remove the stale entry"
1564+
);
1565+
}
1566+
}
1567+
}

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

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,78 @@ pub const CLICKHOUSE_BUILTINS: &[ClickHouseBuiltin] = &[
254254
},
255255
];
256256

257+
/// Aggregate function names DataFusion's own planner resolves out of the box
258+
/// that this catalog deliberately does *not* map to a canonical
259+
/// [`AggSemantic`] -- either because `AggIntent` has no shape for them
260+
/// (a multi-column correlation/regression aggregate, a bitwise/boolean
261+
/// aggregate, a string concatenation aggregate, ...) or because this front
262+
/// end already rejects them explicitly elsewhere (`array_agg`, `grouping`).
263+
///
264+
/// This list exists for one reason: `asap-frontend-sql`'s DataFusion-registry
265+
/// drift test (issue #225, item 3 -- see
266+
/// `asap_frontend_sql::sql::catalog_drift`) walks a real `SessionContext`'s
267+
/// resolved aggregate names and requires every one to be either in
268+
/// [`NATIVE_FUNCTIONS`], a [`CLICKHOUSE_BUILTINS`] name, or listed here. A
269+
/// name landing here is a *recorded decision*, not a silenced test failure --
270+
/// each group below says why. Do not add an entry just to make the test
271+
/// pass; add a `NativeFunction` instead if the name should actually lower.
272+
///
273+
/// Two known, narrow gaps rather than a deliberate non-goal: `var_sample` and
274+
/// `var_population` are DataFusion's own alias spellings of `var_samp` /
275+
/// `var_pop` (see `variance.rs`'s `aliases()` in `datafusion-functions-
276+
/// aggregate`) that [`NATIVE_FUNCTIONS`] doesn't also list under those
277+
/// spellings. Surfaced here rather than silently added to
278+
/// [`NATIVE_FUNCTIONS`], since accepting a new spelling is a maintainer's
279+
/// call, not something this catalog should do on its own.
280+
pub const KNOWN_UNMAPPED_NATIVE_FUNCTIONS: &[&str] = &[
281+
// Selector aggregates: this front end only supports these as window
282+
// functions (`WindowFuncKind::FirstValue`/`LastValue`/`NthValue`, via
283+
// `OVER (...)`), not as plain `GROUP BY` aggregates -- no `AggIntent`
284+
// variant models "the value from a particular row" as a reduction.
285+
"first_value",
286+
"last_value",
287+
"nth_value",
288+
// Bitwise / boolean aggregates -- no corresponding `AggIntent` variant.
289+
"bit_and",
290+
"bit_or",
291+
"bit_xor",
292+
"bool_and",
293+
"bool_or",
294+
// Two-column correlation / linear-regression aggregates -- every
295+
// `AggIntent` value reducer takes one input column (`reducer_col` in
296+
// `asap-frontend-sql`), so these have no home yet.
297+
"corr",
298+
"covar",
299+
"covar_pop",
300+
"covar_samp",
301+
"regr_avgx",
302+
"regr_avgy",
303+
"regr_count",
304+
"regr_intercept",
305+
"regr_r2",
306+
"regr_slope",
307+
"regr_sxx",
308+
"regr_sxy",
309+
"regr_syy",
310+
// String concatenation -- no `AggIntent` equivalent.
311+
"string_agg",
312+
// The weighted-percentile variant of `approx_percentile_cont` (an extra
313+
// weight-column argument); only the unweighted form is in
314+
// `NATIVE_FUNCTIONS`.
315+
"approx_percentile_cont_with_weight",
316+
// Explicitly rejected elsewhere, not merely unmapped:
317+
// `array_agg_is_deliberately_rejected` (asap-frontend-sql's
318+
// sql_lowering tests) covers `array_agg`; `lower_grouping_sets`'s own
319+
// doc comment covers `grouping` (`GROUPING(col)` -- observable only via
320+
// the `__grouping_id` discriminator this front end drops).
321+
"array_agg",
322+
"grouping",
323+
// Known narrow gaps (see doc comment above) -- alias spellings of
324+
// `var_samp` / `var_pop` this catalog doesn't accept yet.
325+
"var_sample",
326+
"var_population",
327+
];
328+
257329
/// Look up a native function name's canonical semantic (case-sensitive --
258330
/// callers normalize case first, as `asap-frontend-sql` already does via
259331
/// `.to_lowercase()`).
@@ -316,4 +388,38 @@ mod tests {
316388
);
317389
}
318390
}
391+
392+
/// `KNOWN_UNMAPPED_NATIVE_FUNCTIONS` documents names this catalog
393+
/// deliberately does *not* map -- it should never overlap with a table
394+
/// that *does* map the same name (that would be a contradiction: mapped
395+
/// and "known unmapped" at once), and shouldn't repeat itself either
396+
/// (each name is a one-time recorded decision).
397+
#[test]
398+
fn known_unmapped_list_is_disjoint_from_the_mapped_tables_and_has_no_duplicates() {
399+
for name in KNOWN_UNMAPPED_NATIVE_FUNCTIONS {
400+
assert!(
401+
lookup_native(name).is_none(),
402+
"{name} is both in NATIVE_FUNCTIONS and KNOWN_UNMAPPED_NATIVE_FUNCTIONS"
403+
);
404+
assert!(
405+
lookup_clickhouse_builtin(name).is_none(),
406+
"{name} is both in CLICKHOUSE_BUILTINS and KNOWN_UNMAPPED_NATIVE_FUNCTIONS"
407+
);
408+
}
409+
let mut sorted = KNOWN_UNMAPPED_NATIVE_FUNCTIONS.to_vec();
410+
sorted.sort_unstable();
411+
sorted.dedup();
412+
assert_eq!(
413+
sorted.len(),
414+
KNOWN_UNMAPPED_NATIVE_FUNCTIONS.len(),
415+
"KNOWN_UNMAPPED_NATIVE_FUNCTIONS has a duplicate entry"
416+
);
417+
}
418+
419+
#[test]
420+
fn known_unmapped_names_are_lowercase() {
421+
for name in KNOWN_UNMAPPED_NATIVE_FUNCTIONS {
422+
assert_eq!(*name, name.to_lowercase(), "not lowercase: {name}");
423+
}
424+
}
319425
}

‎tools/clickhouse/README.md‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
# ClickHouse function-catalog extraction (issue #225, item 3)
2+
3+
`extract_functions.py` is a **dev-only, manual discovery tool**. It compares
4+
ClickHouse's own aggregate-function list (`system.functions`) against
5+
[`asap_sql_function_catalog::CLICKHOUSE_BUILTINS`](../../crates/sql-function-catalog/src/lib.rs)
6+
and reports:
7+
8+
- **candidates** -- ClickHouse aggregate names not yet in `CLICKHOUSE_BUILTINS`
9+
(things worth considering adding),
10+
- **possibly stale** -- `CLICKHOUSE_BUILTINS` entries that no longer appear in
11+
ClickHouse's own list (e.g. renamed or removed upstream), and
12+
- **combinator-derived** -- catalog entries like `countif` that are really a
13+
base function (`count`) plus one of ClickHouse's
14+
[aggregate combinators](https://clickhouse.com/docs/en/sql-reference/aggregate-functions/combinators)
15+
(`-If`, `-Distinct`, `-Array`, ...), which ClickHouse doesn't enumerate as
16+
its own `system.functions` row -- reported separately so it isn't
17+
misreported as stale.
18+
19+
It **only reports**. It never edits `crates/sql-function-catalog/src/lib.rs` --
20+
existence (+ arity) is all issue #225 asks the catalog to track, and deciding
21+
each new entry's `RewriteKind` / canonical semantic is a human judgment call,
22+
not something this script attempts.
23+
24+
## Why this isn't wired into CI
25+
26+
ClickHouse's function surface can only be introspected against an actual
27+
ClickHouse (`system.functions` isn't documented data, it's a live catalog
28+
table). This repo has no ClickHouse service anywhere in CI, and the project
29+
decided deliberately not to add one just for this — see issue #225's own
30+
"needs a decision on where such extraction tooling would actually run" open
31+
question. So: a maintainer runs this locally, by hand, whenever they want to
32+
check for drift or discover new candidates. It's never invoked automatically.
33+
34+
## Running it
35+
36+
This tool uses [`chdb`](https://github.com/chdb-io/chdb) -- ClickHouse
37+
embedded as an in-process Python library. There's no server to start:
38+
39+
```sh
40+
pip install chdb
41+
python3 tools/clickhouse/extract_functions.py
42+
```
43+
44+
Optional flags:
45+
46+
- `--repo-root PATH` -- repo root containing `crates/sql-function-catalog`
47+
(default: inferred from this script's own location).
48+
- `--output PATH` -- write the report to a file instead of stdout.
49+
50+
Because `chdb` runs ClickHouse in-process rather than connecting to a
51+
server, the report reflects whatever ClickHouse version the installed
52+
`chdb` package embeds. `pip install --upgrade chdb` to check against a
53+
newer ClickHouse release.
54+
55+
## Running its tests
56+
57+
The catalog-parsing and diff logic (everything except the actual `chdb`
58+
query) has plain `unittest` coverage that needs no ClickHouse at all:
59+
60+
```sh
61+
cd tools/clickhouse && python3 -m unittest test_extract_functions -v
62+
```

0 commit comments

Comments
 (0)