Summary
#225/#227 built a catalog-driven mechanism so a ClickHouse-only aggregate builtin DataFusion doesn't know at all (uniqExact, countIf) gets a stub AggregateUDF registered (so DataFusion's planner accepts the call) plus a FunctionRewrite to something DataFusion natively understands, driven by data in crates/sql-function-catalog. The bgp_jan2024_workload 200-query corpus (#219) surfaced the same problem for scalar functions, and it's a strictly cheaper fix than the aggregate case was.
Where the pain shows up today
Milind's analysis of the 200-query corpus's Category::Plan ("unknown function") failures, by function name:
| Function |
Occurrences |
Kind |
splitByChar |
11 |
string split (usually combined with [-1]/[n] indexing) |
toStartOfHour/toStartOfWeek/toStartOfMinute/toStartOfFiveMinutes/toStartOfInterval |
19 combined |
time-bucketing family |
toDate |
10 |
date cast |
match |
7 |
regex match |
startsWith |
5 |
string prefix |
positionCaseInsensitive |
5 |
case-insensitive substring search |
All of these fail today the same way uniqExact/countIf used to: DataFusion's own planner rejects the unregistered function name during SqlToRel conversion, before the front end's own expression-lowering code ever runs.
Why this is cheaper than the aggregate case
Unlike aggregate calls (which need to become a real AggIntent — a first-class canonical-IR operator with defined semantics, hence #225's AggSemantic/RewriteKind classification and countIf's CASE-based rewrite), a scalar function call in this IR is already deliberately opaque: crates/frontend-sql/src/sql/expr.rs's Expr::ScalarFunction arm (~line 189) lowers any scalar call generically to QueryExpr::FunctionCall { name, args }, with zero further code needed — infer_expr_type's own doc comment (crates/types/src/pre_asap/query_expr.rs) already says scalar function calls get "no function/type registry here — default permissive (post-ASAP binding refines with a real function/type registry)." So once DataFusion's planner is taught to accept a ClickHouse scalar builtin's name at all (a stub ScalarUDF, mirroring clickhouse_builtin_stub_udaf for AggregateUDF), the existing generic lowering already produces a structurally correct FunctionCall node — no rewrite/semantic-classification step is needed, unlike the aggregate case.
Proposed direction
- Add a scalar-function table to (or alongside)
crates/sql-function-catalog — name + arity is enough (no return-type modeling needed; the stub only needs a plausible Arrow return type so DataFusion's planner can keep building the rest of the logical plan around the call — the stub's invoke is never actually reached, same "dead code by construction" reasoning clickhouse_builtin_stub_udaf's doc comment already gives, since this front end only uses DataFusion for planning/type-checking, never executes the physical plan).
- Register a stub
ScalarUDF per catalog entry in SqlLowerer::build_context(), alongside the existing per-entry AggregateUDF stub registration loop.
- No
FunctionRewrite needed for these — they should pass straight through as FunctionCall { name, args }.
Known caveat — don't expect 100% of the listed occurrences to flip to Lowered
splitByChar(...)[-1]-style calls combine the function-name gap with array/map index access, which is a separate, already-known DataFusionError::NotImplemented gap (see bgp_jan2024_workload.rs's own Category::NotImplemented doc comment). Registering splitByChar's stub will likely move some of its 11 occurrences from Category::Plan to Category::NotImplemented rather than straight to Lowered — that's expected, not a bug in this fix.
Explicitly out of scope
lagInFrame (window function) — tracked separately, needs a window-frame-modeling design decision first (see follow-up issue).
argMax (new aggregate semantic) — tracked separately, needs an AggIntent design decision (see follow-up issue).
- Any actual runtime/semantic implementation of what these functions do — this issue is only about getting them structurally represented as
FunctionCall nodes, matching how every other scalar function (LOWER, ABS, ...) is already handled opaquely at this layer.
Related
Summary
#225/#227 built a catalog-driven mechanism so a ClickHouse-only aggregate builtin DataFusion doesn't know at all (
uniqExact,countIf) gets a stubAggregateUDFregistered (so DataFusion's planner accepts the call) plus aFunctionRewriteto something DataFusion natively understands, driven by data incrates/sql-function-catalog. Thebgp_jan2024_workload200-query corpus (#219) surfaced the same problem for scalar functions, and it's a strictly cheaper fix than the aggregate case was.Where the pain shows up today
Milind's analysis of the 200-query corpus's
Category::Plan("unknown function") failures, by function name:splitByChar[-1]/[n]indexing)toStartOfHour/toStartOfWeek/toStartOfMinute/toStartOfFiveMinutes/toStartOfIntervaltoDatematchstartsWithpositionCaseInsensitiveAll of these fail today the same way
uniqExact/countIfused to: DataFusion's own planner rejects the unregistered function name duringSqlToRelconversion, before the front end's own expression-lowering code ever runs.Why this is cheaper than the aggregate case
Unlike aggregate calls (which need to become a real
AggIntent— a first-class canonical-IR operator with defined semantics, hence #225'sAggSemantic/RewriteKindclassification andcountIf'sCASE-based rewrite), a scalar function call in this IR is already deliberately opaque:crates/frontend-sql/src/sql/expr.rs'sExpr::ScalarFunctionarm (~line 189) lowers any scalar call generically toQueryExpr::FunctionCall { name, args }, with zero further code needed —infer_expr_type's own doc comment (crates/types/src/pre_asap/query_expr.rs) already says scalar function calls get "no function/type registry here — default permissive (post-ASAP binding refines with a real function/type registry)." So once DataFusion's planner is taught to accept a ClickHouse scalar builtin's name at all (a stubScalarUDF, mirroringclickhouse_builtin_stub_udafforAggregateUDF), the existing generic lowering already produces a structurally correctFunctionCallnode — no rewrite/semantic-classification step is needed, unlike the aggregate case.Proposed direction
crates/sql-function-catalog— name + arity is enough (no return-type modeling needed; the stub only needs a plausible Arrow return type so DataFusion's planner can keep building the rest of the logical plan around the call — the stub'sinvokeis never actually reached, same "dead code by construction" reasoningclickhouse_builtin_stub_udaf's doc comment already gives, since this front end only uses DataFusion for planning/type-checking, never executes the physical plan).ScalarUDFper catalog entry inSqlLowerer::build_context(), alongside the existing per-entryAggregateUDFstub registration loop.FunctionRewriteneeded for these — they should pass straight through asFunctionCall { name, args }.Known caveat — don't expect 100% of the listed occurrences to flip to
LoweredsplitByChar(...)[-1]-style calls combine the function-name gap with array/map index access, which is a separate, already-knownDataFusionError::NotImplementedgap (seebgp_jan2024_workload.rs's ownCategory::NotImplementeddoc comment). RegisteringsplitByChar's stub will likely move some of its 11 occurrences fromCategory::PlantoCategory::NotImplementedrather than straight toLowered— that's expected, not a bug in this fix.Explicitly out of scope
lagInFrame(window function) — tracked separately, needs a window-frame-modeling design decision first (see follow-up issue).argMax(new aggregate semantic) — tracked separately, needs anAggIntentdesign decision (see follow-up issue).FunctionCallnodes, matching how every other scalar function (LOWER,ABS, ...) is already handled opaquely at this layer.Related
bgp_jan2024_workloadcorpus test, where this gap was foundcrates/frontend-sql/src/sql/expr.rs—df_expr_to_unresolved'sExpr::ScalarFunctionarmcrates/frontend-sql/src/sql/mod.rs—SqlLowerer::build_context,clickhouse_builtin_stub_udaf