Fix nullability report for spark expression #3362

Activity

1. hsiang-c

   added

   bugSomething isn't workingSomething isn't working

   on Feb 1, 2026
2. andygrove

   added

   priority:mediumFunctional bugs, performance regressions, broken featuresFunctional bugs, performance regressions, broken features

   area:expressionsExpression evaluationExpression evaluation

   on Mar 27, 2026
3. andygrove commented on Jun 30, 2026

   

   andygrove

   on Jun 30, 2026

   Member

   More actions

   Analysis: how this applies to Comet

   Worth noting that Comet's situation is the inverse of the DataFusion EPIC (apache/datafusion#19144), which makes the fix here cheaper.

   DataFusion has to reimplement Spark's per-expression nullability rules in Rust (via return_field_from_args) because it has no Spark to ask. Comet does have Spark: expr.nullable is already computed correctly on the JVM side, including nullIntolerant propagation, before serialization. So the natural fix for Comet is to propagate that value through the proto rather than re-deriving it natively.

   Current state

   • Most native expressions conservatively report nullable() -> true.
   • The only expression that propagates Spark's real nullability today is JvmScalarUdf, via the return_nullable field on its proto message (expr.proto), consumed in jvm_udf/mod.rs.
   • A version-agnostic isNullIntolerant shim already exists (CometExprTraitShim, handling the 3.x trait and the 4.x boolean method), but it is currently only used by the codegen dispatcher's null short-circuit, not for reporting native output nullability.

   Why it matters

   The risk is directional:

   • Over-reporting (nullable=true when actually non-null) is safe. It only misses optimizations. This is Comet's current behavior.
   • Under-reporting (nullable=false, then producing a null) is a hard failure: Arrow RecordBatch validation throws "declared as non-nullable but contains null values" downstream in shuffle/sort.

   That second failure mode has already been fixed one expression at a time, for example #4523 (GetStructField), #4533 (CreateArray), #4237 (array_except), and #4554 (codegen dispatcher honoring ev.isNull for nullable NullIntolerant roots). A systematic mechanism would retire this recurring class of bugs.

   Suggested approach

   1. Generalize the JvmScalarUdf pattern into a return_nullable channel for general expressions (cleanest as a generic wrapper on the Expr proto message so every serde benefits, rather than per-message fields).
   2. Set it from expr.nullable in the serde layer.
   3. Consume it on the Rust side in nullable() and when building output Fields in planner.rs.

   One caveat

   Propagating nullable=false is only safe where Comet's native implementation provably matches Spark, otherwise a latent value divergence becomes a hard Arrow validation error the moment the field is marked non-nullable. Comet's current nullable=true default is the safe floor, so this likely wants to roll out incrementally and per-expression rather than as a global flip.

   👍React with 👍1Reacted by hsiang-c