Repository navigation
[EPIC] Implement expressions as ScalarUDFImpl #1819
Description
What is the problem the feature request solves?
Many of Comet's Spark-compatible DataFusion expressions are currently implemented as PhysicalExpr. We would like to update them to implement ScalarUDFImpl instead as a first step to contributing these expressions upstream to the datafusion-spark crate.
We should create one issue/PR per expression.
Here is a list of expressions based on a quick grep - it may not be complete, and it may not make sense to try and upstream some expressions.
$ find . -name *.rs -exec grep "impl PhysicalExpr" {} \;
impl PhysicalExpr for IfExpr {
impl PhysicalExpr for Cast {
impl PhysicalExpr for ToJson {
impl PhysicalExpr for ListExtract {
impl PhysicalExpr for ArrayInsert {
impl PhysicalExpr for GetArrayStructFields {
impl PhysicalExpr for RLike {
impl PhysicalExpr for NegativeExpr {
impl PhysicalExpr for CheckOverflow {
impl PhysicalExpr for NormalizeNaNAndZero {
impl PhysicalExpr for TimestampTruncExpr {
impl PhysicalExpr for DateTruncExpr {
impl PhysicalExpr for HourExpr {
impl PhysicalExpr for SecondExpr {
impl PhysicalExpr for MinuteExpr {
impl PhysicalExpr for StringSpaceExpr {
impl PhysicalExpr for SubstringExpr {
impl PhysicalExpr for $name {
impl PhysicalExpr for UnboundColumn {
impl PhysicalExpr for GetStructField {
impl PhysicalExpr for CreateNamedStruct {
impl PhysicalExpr for BitwiseNotExpr {
impl PhysicalExpr for BloomFilterMightContain {
impl PhysicalExpr for Subquery {
$ find . -name *.rs -exec grep "make_predicate_function" {} \;
macro_rules! make_predicate_function {
make_predicate_function!(Like, like_dyn, like_utf8_scalar_dyn);
make_predicate_function!(StartsWith, starts_with_dyn, starts_with_utf8_scalar_dyn);
make_predicate_function!(EndsWith, ends_with_dyn, ends_with_utf8_scalar_dyn);
make_predicate_function!(Contains, contains_dyn, contains_utf8_scalar_dyn);
Describe the potential solution
No response
Additional context
No response
Reacted by Shehab Amin
Reacted by Bhargava Vadlamani
Activity
Kimahriman commentedon Jun 29, 2025
Wondering if it makes sense to convert some of these. Specifically the two I added:
GetStructField: DataFusion has a similarget_fieldalready we could use, but it operates by field name instead of index, so it has to find the right field by name for each batch which adds some overhead, which is some of the downside it seems to the UDFs over logical and physical expressions which can optimize these cases at analysis time. Implementing a custom ScalarUDF also doesn't really make sense because as a user you would never select a nested field by index, always by name.GetArrayStructFields: There's no equivalent DataFusion expression AFAIK, but the same thing above applies where there's not really a use cases for using this function directly, Spark just creates this at analysis time from anExtractValueoperationparthchandra commentedon Jul 1, 2025
@Kimahriman what you say makes sense. For
GetStructFieldandGetArrayStructFieldsit may not be useful to convert to ScalarUDFImplakupchinskiy commentedon Jul 17, 2025
One limitation of trying to switch from PhysicalExpr to ScalarUDFImpl I faced - is lack of capability extract the batch size. That is why it won't work for non-determenistic functions. Though, those functions are tightly coupled with spark runtime, so I doubt they are worth being included into the datafusion-spark crate.
andygrove commentedon Sep 28, 2026
Closing this. The conversions that made sense landed in 2025 (#1825, #1826, #1874, #1880, #1887, #1954, #2041), and bit_not, bit_count, space and substring now come straight from datafusion-spark, which was the point of this epic. Comet uses datafusion-spark directly now, so converting our own expressions as a first step toward upstreaming isn't how this work gets done.
Most of what's left shouldn't be converted. IfExpr wraps CaseExpr so each branch is evaluated only for the rows that take it. A ScalarUDF receives every argument already evaluated, so IF(b = 0, NULL, a / b) would fail under ANSI, and datafusion-spark's
ifonly works throughsimplify, which our planner never calls. GetStructField and GetArrayStructFields were ruled out above, and the nondeterministic expressions need per-partition state. Subquery, UnboundColumn, CheckOverflow and NormalizeNaNAndZero are planner and analyzer plumbing rather than SQL functions. Cast would be a project of its own, and DataFusion already has named_struct. Timestamp truncation is covered by #5103. The user-facing ones that remain (to_json, element_at, array_insert, rlike) are better as separate issues for upstreaming each one, if someone wants to take them on.