diff --git a/crates/frontend-sql/src/sql/mod.rs b/crates/frontend-sql/src/sql/mod.rs index b545d26..d6f1d97 100644 --- a/crates/frontend-sql/src/sql/mod.rs +++ b/crates/frontend-sql/src/sql/mod.rs @@ -37,8 +37,8 @@ use datafusion::functions_aggregate::sum::sum_udaf; use datafusion::logical_expr::expr::AggregateFunction; use datafusion::logical_expr::expr_rewriter::FunctionRewrite; use datafusion::logical_expr::{ - self, lit, AggregateUDF, Case, Distinct, Expr, JoinType, LogicalPlan, Signature, - SimpleAggregateUDF, TypeSignature, Volatility, WindowFunctionDefinition, + self, lit, AggregateUDF, Case, Distinct, Expr, JoinType, LogicalPlan, ScalarUDF, ScalarUDFImpl, + Signature, SimpleAggregateUDF, TypeSignature, Volatility, WindowFunctionDefinition, }; use datafusion::optimizer::analyzer::function_rewrite::ApplyFunctionRewrites; use datafusion::optimizer::{AnalyzerRule, OptimizerConfig}; @@ -208,6 +208,19 @@ impl<'a> SqlLowerer<'a> { for builtin in asap_sql_function_catalog::CLICKHOUSE_BUILTINS { ctx.register_udaf(clickhouse_builtin_stub_udaf(builtin.name, builtin.arity)); } + // Register a stub `ScalarUDF` for every catalog-listed ClickHouse-only + // *scalar* builtin — same reason as the `AggregateUDF` loop above + // (DataFusion otherwise rejects the call as an unknown function + // during `SqlToRel` conversion), but with no rewrite step to follow: + // `df_expr_to_unresolved`'s `Expr::ScalarFunction` arm already lowers + // any scalar call generically to `Unresolved::FunctionCall { name, + // args }`, so registering the stub is the entire fix (issue #230). + for builtin in asap_sql_function_catalog::CLICKHOUSE_SCALAR_BUILTINS { + ctx.register_udf(clickhouse_scalar_builtin_stub_udf( + builtin.name, + builtin.arity, + )); + } Ok(ctx) } @@ -911,8 +924,10 @@ fn clickhouse_builtin_stub_udaf(name: &'static str, arity: Arity) -> AggregateUD )) } -/// A catalog [`Arity`] as the DataFusion `Signature` a stub UDAF is -/// registered with. +/// A catalog [`Arity`] as the DataFusion `Signature` a stub UDAF/UDF is +/// registered with — shared by the aggregate stub above and the scalar stub +/// below, since neither wants to model per-argument types, only how many +/// arguments a call may take. fn arity_to_signature(arity: Arity) -> Signature { match arity { Arity::Exact(n) => Signature::any(n, Volatility::Immutable), @@ -923,6 +938,104 @@ fn arity_to_signature(arity: Arity) -> Signature { } } +// ── ClickHouse scalar-builtin compatibility ───────────────────────────────── +// +// The scalar counterpart of the aggregate mechanism above, but simpler: +// `asap_sql_function_catalog::CLICKHOUSE_SCALAR_BUILTINS` carries no +// `RewriteKind`, because a scalar call needs none. Unlike an aggregate call +// (which must become a real `AggIntent`, hence the rewrite to a native +// DataFusion aggregate shape `lower_agg_intent` can classify), a scalar +// function call in this IR is already deliberately opaque — +// `expr::df_expr_to_unresolved`'s `Expr::ScalarFunction` arm lowers *any* +// scalar call generically to `Unresolved::FunctionCall { name, args }`, with +// zero name-specific logic. So teaching DataFusion's planner to accept a +// ClickHouse scalar builtin's name — a stub `ScalarUDF`, registered below — +// is the entire fix; the existing generic lowering already does the rest. + +/// A stub `ScalarUDF` for one `CLICKHOUSE_SCALAR_BUILTINS` entry, registered +/// purely so DataFusion's planner can resolve the function name during +/// `SqlToRel` conversion (it errors on an unknown function otherwise), and so +/// it can keep building the surrounding expression's type from a plausible +/// return type. Unlike `clickhouse_builtin_stub_udaf`, no `FunctionRewrite` +/// ever fires for these — the call survives to `lower_plan` as-is and lowers +/// through the generic `Expr::ScalarFunction` arm — so `invoke`/`invoke_batch` +/// (left at their default, which returns a `NotImplemented` `DataFusionError`) +/// are unreachable for every catalog entry: this front end only ever uses +/// DataFusion for planning/type-checking, never physical execution. +fn clickhouse_scalar_builtin_stub_udf(name: &'static str, arity: Arity) -> ScalarUDF { + ScalarUDF::from(ClickHouseScalarBuiltinStub { + name, + signature: arity_to_signature(arity), + return_type: clickhouse_scalar_builtin_return_type(name), + }) +} + +/// A plausible Arrow return type for one `CLICKHOUSE_SCALAR_BUILTINS` entry — +/// just precise enough that DataFusion's planner can keep building the type +/// of whatever expression the call sits inside (e.g. a `WHERE` predicate +/// wants `Boolean`), not a claim about ClickHouse's actual return type. +/// Real function typing happens downstream, at post-ASAP binding. +fn clickhouse_scalar_builtin_return_type(name: &str) -> ArrowDataType { + match name { + // Array(String) in ClickHouse; a plain `Utf8` element list is close + // enough for planning purposes here. + "splitbychar" => ArrowDataType::List(Arc::new(datafusion::arrow::datatypes::Field::new( + "item", + ArrowDataType::Utf8, + true, + ))), + "todate" => ArrowDataType::Date32, + // ClickHouse returns UInt8 (0/1), but every corpus use is a boolean + // predicate — `Boolean` keeps that context type-checking. + "match" | "startswith" => ArrowDataType::Boolean, + "tostartofhour" + | "tostartofweek" + | "tostartofminute" + | "tostartoffiveminutes" + | "tostartofinterval" => { + ArrowDataType::Timestamp(datafusion::arrow::datatypes::TimeUnit::Millisecond, None) + } + // 1-based match position, 0 if not found. + "positioncaseinsensitive" => ArrowDataType::UInt64, + other => unreachable!( + "{other}: every CLICKHOUSE_SCALAR_BUILTINS entry must have a return type listed here" + ), + } +} + +/// A stub `ScalarUDFImpl` carrying only what DataFusion's planner needs: +/// name, arity-only [`Signature`], and a fixed return type. `invoke`/ +/// `invoke_batch` are left at their trait defaults (a `NotImplemented` +/// `DataFusionError`) — see [`clickhouse_scalar_builtin_stub_udf`]'s doc for +/// why that is unreachable in practice. +#[derive(Debug)] +struct ClickHouseScalarBuiltinStub { + name: &'static str, + signature: Signature, + return_type: ArrowDataType, +} + +impl ScalarUDFImpl for ClickHouseScalarBuiltinStub { + fn as_any(&self) -> &dyn std::any::Any { + self + } + + fn name(&self) -> &str { + self.name + } + + fn signature(&self) -> &Signature { + &self.signature + } + + fn return_type( + &self, + _arg_types: &[ArrowDataType], + ) -> datafusion::common::Result { + Ok(self.return_type.clone()) + } +} + /// Rewrites every `asap_sql_function_catalog::CLICKHOUSE_BUILTINS` call to /// the native DataFusion aggregate shape its entry's `RewriteKind` names — /// so a ClickHouse-only builtin DataFusion doesn't know at all becomes an diff --git a/crates/frontend-sql/tests/bgp_analytics/bgp_analytics.rs b/crates/frontend-sql/tests/bgp_analytics/bgp_analytics.rs index 4cab365..0a81748 100644 --- a/crates/frontend-sql/tests/bgp_analytics/bgp_analytics.rs +++ b/crates/frontend-sql/tests/bgp_analytics/bgp_analytics.rs @@ -18,10 +18,12 @@ //! getting "fixed" into an unknown-function error, or vice versa) fails the //! test even though the aggregate 4/9/2 split wouldn't otherwise move: //! - 9 queries hit `DataFusionError::Plan` ("unknown function"): they use -//! ClickHouse-only builtins (`countIf`, `toStartOfInterval`, -//! `lagInFrame`, `isIPAddressInRange`, `arrayJoin`, `arrayFilter`, ...) -//! that parse fine under the ClickHouse dialect but have no DataFusion -//! planner equivalent registered. +//! ClickHouse-only builtins (`toIntervalMinute`, `lagInFrame`, +//! `isIPAddressInRange`, `arrayJoin`, `arrayFilter`, ...) that parse fine +//! under the ClickHouse dialect but have no DataFusion planner equivalent +//! registered. (`toStartOfInterval` itself is registered -- issue #230 -- +//! but query 7 nests an unregistered `toIntervalMinute(...)` call inside +//! it, so it still lands here, just one function name deeper.) //! - 2 queries (14, 15) hit `DataFusionError::SQL` (a `ParserError`): they //! use ClickHouse grammar the vendored sqlparser doesn't implement at //! all -- a scalar/tuple `WITH AS ` binding, and the @@ -116,20 +118,24 @@ enum Expected { /// Expected outcome for corpus queries 1-15, in order. See the module doc /// comment for why each query lands where it does. const EXPECTED: &[Expected] = &[ - Expected::Lowered, // 1 - Expected::Lowered, // 2 - Expected::UnknownFunction("arrayfilter"), // 3 - Expected::UnknownFunction("arrayfilter"), // 4 - Expected::UnknownFunction("arrayfilter"), // 5 - Expected::Lowered, // 6 - Expected::UnknownFunction("tostartofinterval"), // 7 - Expected::UnknownFunction("arrayfilter"), // 8 - Expected::UnknownFunction("isipaddressinrange"), // 9 - Expected::UnknownFunction("arrayfilter"), // 10 - Expected::UnknownFunction("laginframe"), // 11 - Expected::Lowered, // 12 - Expected::UnknownFunction("arrayjoin"), // 13 - Expected::UnsupportedGrammar("Expected: identifier, found: ("), // 14 + Expected::Lowered, // 1 + Expected::Lowered, // 2 + Expected::UnknownFunction("arrayfilter"), // 3 + Expected::UnknownFunction("arrayfilter"), // 4 + Expected::UnknownFunction("arrayfilter"), // 5 + Expected::Lowered, // 6 + // `toStartOfInterval` itself is registered (issue #230), so planning + // proceeds into its nested `toIntervalMinute(5)` argument -- an + // unregistered ClickHouse builtin outside this issue's 10-function + // scope -- and fails there instead. + Expected::UnknownFunction("tointervalminute"), // 7 + Expected::UnknownFunction("arrayfilter"), // 8 + Expected::UnknownFunction("isipaddressinrange"), // 9 + Expected::UnknownFunction("arrayfilter"), // 10 + Expected::UnknownFunction("laginframe"), // 11 + Expected::Lowered, // 12 + Expected::UnknownFunction("arrayjoin"), // 13 + Expected::UnsupportedGrammar("Expected: identifier, found: ("), // 14 Expected::UnsupportedGrammar("Expected: a list of columns in parentheses"), // 15 ]; diff --git a/crates/frontend-sql/tests/bgp_jan2024_workload/bgp_jan2024_workload.rs b/crates/frontend-sql/tests/bgp_jan2024_workload/bgp_jan2024_workload.rs index c3e73fe..7e122b1 100644 --- a/crates/frontend-sql/tests/bgp_jan2024_workload/bgp_jan2024_workload.rs +++ b/crates/frontend-sql/tests/bgp_jan2024_workload/bgp_jan2024_workload.rs @@ -165,15 +165,37 @@ async fn corpus_lowering_matches_the_pinned_aggregate_tally() { // "unknown function: countif" `Plan` failure for 13 queries: 12 now // lower end to end, and a 13th plans far enough to hit a second, // pre-existing gap (map/array index access, `NotImplemented`). - expect(Category::Lowered, 97); - expect(Category::Plan, 92); + // + // Stub-`ScalarUDF` registration for `CLICKHOUSE_SCALAR_BUILTINS` (issue + // #230 -- `splitByChar`, `toDate`, `match`, the `toStartOf*` family, + // `startsWith`, `positionCaseInsensitive`) clears the "unknown function" + // `Plan` failure for the rest of the 92: most now lower end to end, but + // not all -- as the issue itself flags, `splitByChar(...)[-1]`-style + // calls (and a couple of other array/map-index uses) now plan far enough + // to hit the same pre-existing map/array-index `NotImplemented` gap, and + // two `toStartOfInterval(...)` queries plan far enough to hit a + // different pre-existing gap: `types::scalar_value_to_asap` doesn't yet + // convert an `INTERVAL x unit` literal (`DfScalarValue:: + // IntervalMonthDayNano`), so those two land in `Other` via + // `LoweringError::InvalidExpression` instead. Both are companion gaps + // this issue's scope explicitly doesn't chase down (see its "known + // caveat" section) -- getting these functions' *names* to lower to a + // structurally correct `FunctionCall` node is what's in scope here, not + // array/map indexing or interval-literal conversion. + expect(Category::Lowered, 145); + expect(Category::Plan, 40); expect(Category::Schema, 0); expect(Category::Parse, 0); // One query that used to fail at `uniqExact` (`Plan`) now clears that // hurdle -- `ClickHouseBuiltinRewrite` in `sql/mod.rs` rewrites it to // `COUNT(DISTINCT ...)` before `lower_plan` runs -- and plans far enough - // to hit a second, pre-existing gap: map/array index access. - expect(Category::NotImplemented, 5); + // to hit a second, pre-existing gap: map/array index access. Plus the + // `splitByChar`/other array-index companion gap noted above (issue + // #230). + expect(Category::NotImplemented, 7); expect(Category::UnsupportedFeature, 6); - expect(Category::Other, 0); + // Two `toStartOfInterval(...)` queries -- see the `toStartOfInterval` + // note above; a pre-existing `INTERVAL`-literal conversion gap, not a + // ClickHouse scalar-builtin catalog gap. + expect(Category::Other, 2); } diff --git a/crates/sql-function-catalog/src/lib.rs b/crates/sql-function-catalog/src/lib.rs index cff8936..5cee2e8 100644 --- a/crates/sql-function-catalog/src/lib.rs +++ b/crates/sql-function-catalog/src/lib.rs @@ -16,24 +16,31 @@ //! outright (`reducer_col` in `asap-frontend-sql`) rather than trying to //! typecheck it. //! -//! Two tables, matching the two problems this replaces: +//! Three tables, matching the three problems this replaces: //! -//! - [`NATIVE_FUNCTIONS`] -- names DataFusion's own planner already resolves -//! (`sum`, `avg`, `approx_percentile_cont`, ...). [`lookup_native`] maps -//! one to the [`AggSemantic`] `lower_agg_intent` builds an `AggIntent` +//! - [`NATIVE_FUNCTIONS`] -- aggregate names DataFusion's own planner already +//! resolves (`sum`, `avg`, `approx_percentile_cont`, ...). [`lookup_native`] +//! maps one to the [`AggSemantic`] `lower_agg_intent` builds an `AggIntent` //! from. The DISTINCT-modifier rule ("`COUNT DISTINCT` alone maps, to //! `Cardinality`; reject DISTINCT elsewhere") and the "reducer argument //! must be a bare column" rule are call-site logic, not per-function data, //! and stay in `asap-frontend-sql`. -//! - [`CLICKHOUSE_BUILTINS`] -- ClickHouse-only names DataFusion doesn't -//! know at all (`uniqExact`, `countIf`). Each entry additionally carries a -//! [`RewriteKind`]: the native DataFusion aggregate shape the call -//! rewrites to before `lower_agg_intent` (or DataFusion's own physical +//! - [`CLICKHOUSE_BUILTINS`] -- ClickHouse-only *aggregate* names DataFusion +//! doesn't know at all (`uniqExact`, `countIf`). Each entry additionally +//! carries a [`RewriteKind`]: the native DataFusion aggregate shape the +//! call rewrites to before `lower_agg_intent` (or DataFusion's own physical //! planner) ever has to understand the ClickHouse name itself. This is //! what generalizes `uniqExact`'s old bespoke `UniqExactRewrite` + //! `uniq_exact_udaf` pair (issue #221): a new builtin that rewrites to an //! already-handled shape is a new entry in this table, not a new //! `FunctionRewrite` impl and a new stub-`AggregateUDF` constructor. +//! - [`CLICKHOUSE_SCALAR_BUILTINS`] -- ClickHouse-only *scalar* names +//! DataFusion doesn't know at all (`splitByChar`, `toDate`, `match`, +//! the `toStartOf*` family, `startsWith`, `positionCaseInsensitive`). No +//! [`RewriteKind`] here: unlike an aggregate call, a scalar call already +//! lowers generically (`asap-frontend-sql::sql::expr`'s +//! `Expr::ScalarFunction` arm), so a stub `ScalarUDF` registered for the +//! name is the whole fix (issue #230). //! //! Generating these tables from a live introspectable source -- ClickHouse's //! `system.functions`, DataFusion's own in-process UDF/UDAF registry -- the @@ -342,6 +349,97 @@ pub fn lookup_clickhouse_builtin(name: &str) -> Option<&'static ClickHouseBuilti CLICKHOUSE_BUILTINS.iter().find(|b| b.name == name) } +/// One [`CLICKHOUSE_SCALAR_BUILTINS`] entry -- just `{ name, arity }`, no +/// [`RewriteKind`]. Unlike an aggregate call, a scalar function call in the +/// canonical IR is already deliberately opaque +/// (`asap-frontend-sql::sql::expr::df_expr_to_unresolved`'s +/// `Expr::ScalarFunction` arm lowers *any* scalar call generically to +/// `Unresolved::FunctionCall { name, args }`), so once DataFusion's planner +/// accepts the name at all -- via a stub `ScalarUDF`, see +/// `asap-frontend-sql::sql::clickhouse_scalar_builtin_stub_udf` -- the +/// existing generic lowering already produces a structurally correct node. +/// No rewrite/semantic classification is needed (issue #230). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ClickHouseScalarBuiltin { + /// Lowercase function name, matching the name a stub `ScalarUDF` is + /// registered under (DataFusion resolves a SQL call to it + /// case-insensitively, but reports it back lowercase). + pub name: &'static str, + pub arity: Arity, +} + +/// ClickHouse-only *scalar* builtin names DataFusion's planner has no native +/// equivalent for at all -- each needs a stub `ScalarUDF` registered so the +/// planner accepts the call. Arities follow ClickHouse's documented +/// signatures for each function (optional trailing arguments -- a timezone, +/// a start position, a max-substrings cap -- become an `Arity::Range`). +/// +/// No return-type modeling here: the stub's Arrow return type (a single, +/// per-entry plausible choice, not modeled in this arity-only table) only +/// needs to let DataFusion's planner keep building the surrounding +/// expression's type -- see `clickhouse_scalar_builtin_stub_udf`'s call +/// sites in `SqlLowerer::build_context`. +pub const CLICKHOUSE_SCALAR_BUILTINS: &[ClickHouseScalarBuiltin] = &[ + // splitByChar(separator, s[, max_substrings]) -> Array(String). + ClickHouseScalarBuiltin { + name: "splitbychar", + arity: Arity::Range { min: 2, max: 3 }, + }, + // toDate(expr) -> Date. + ClickHouseScalarBuiltin { + name: "todate", + arity: Arity::Exact(1), + }, + // match(haystack, pattern) -> UInt8 (0/1), used as a boolean predicate. + ClickHouseScalarBuiltin { + name: "match", + arity: Arity::Exact(2), + }, + // toStartOfHour(datetime[, timezone]) -> DateTime. + ClickHouseScalarBuiltin { + name: "tostartofhour", + arity: Arity::Range { min: 1, max: 2 }, + }, + // toStartOfWeek(datetime[, mode[, timezone]]) -> Date. + ClickHouseScalarBuiltin { + name: "tostartofweek", + arity: Arity::Range { min: 1, max: 3 }, + }, + // toStartOfMinute(datetime[, timezone]) -> DateTime. + ClickHouseScalarBuiltin { + name: "tostartofminute", + arity: Arity::Range { min: 1, max: 2 }, + }, + // toStartOfFiveMinutes(datetime[, timezone]) -> DateTime. + ClickHouseScalarBuiltin { + name: "tostartoffiveminutes", + arity: Arity::Range { min: 1, max: 2 }, + }, + // toStartOfInterval(datetime, INTERVAL x unit[, timezone]) -> DateTime. + // The `INTERVAL x unit` clause parses as a single expression argument. + ClickHouseScalarBuiltin { + name: "tostartofinterval", + arity: Arity::Range { min: 2, max: 3 }, + }, + // startsWith(s, prefix) -> UInt8 (0/1), used as a boolean predicate. + ClickHouseScalarBuiltin { + name: "startswith", + arity: Arity::Exact(2), + }, + // positionCaseInsensitive(haystack, needle[, start_pos]) -> UInt64 + // (1-based position, 0 if not found). + ClickHouseScalarBuiltin { + name: "positioncaseinsensitive", + arity: Arity::Range { min: 2, max: 3 }, + }, +]; + +/// Look up a ClickHouse-only scalar builtin by name (case-sensitive, see +/// [`lookup_native`]). +pub fn lookup_clickhouse_scalar_builtin(name: &str) -> Option<&'static ClickHouseScalarBuiltin> { + CLICKHOUSE_SCALAR_BUILTINS.iter().find(|b| b.name == name) +} + #[cfg(test)] mod tests { use super::*; @@ -422,4 +520,46 @@ mod tests { assert_eq!(*name, name.to_lowercase(), "not lowercase: {name}"); } } + + #[test] + fn clickhouse_scalar_builtin_lookup_finds_every_listed_name() { + for b in CLICKHOUSE_SCALAR_BUILTINS { + let found = + lookup_clickhouse_scalar_builtin(b.name).expect("listed name must be found"); + assert_eq!(found.name, b.name); + assert_eq!(found.arity, b.arity); + } + assert_eq!( + lookup_clickhouse_scalar_builtin("not_a_real_function"), + None + ); + } + + #[test] + fn clickhouse_scalar_builtin_names_are_lowercase() { + for b in CLICKHOUSE_SCALAR_BUILTINS { + assert_eq!(b.name, b.name.to_lowercase(), "not lowercase: {}", b.name); + } + } + + /// Scalar builtins live in their own namespace from the aggregate + /// tables: a scalar and an aggregate function can share a bare SQL name + /// in general, but none of this catalog's entries happen to collide, so + /// this documents that rather than asserting a real invariant this crate + /// enforces elsewhere. + #[test] + fn clickhouse_scalar_builtins_do_not_shadow_native_or_aggregate_names() { + for b in CLICKHOUSE_SCALAR_BUILTINS { + assert!( + lookup_native(b.name).is_none(), + "{} listed as both a native aggregate and a ClickHouse scalar builtin", + b.name + ); + assert!( + lookup_clickhouse_builtin(b.name).is_none(), + "{} listed as both a ClickHouse aggregate and scalar builtin", + b.name + ); + } + } }