feat(sql): generalize ClickHouse-builtin stub registration to ScalarUDF

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<ArrowDataType> {
+        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

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 <expr> AS <alias>` 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
 ];
 

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);
 }