Repository navigation
Math expression audit follow-ups (from #4486) #4500
Description
Tracking issue for follow-up work surfaced by the math expression audit in #4486. Each item below is either a support-level / serde correctness fix that the audit deliberately deferred, or a coverage gap the audit documented but did not implement.
High priority
1. Lift convert-time withInfo fallbacks in arithmetic.scala into getSupportLevel
CometAdd, CometSubtract, CometMultiply, CometDivide, CometIntegralDivide, CometRemainder, CometUnaryMinus, and CometRound (spark/src/main/scala/org/apache/comet/serde/arithmetic.scala:94-271, 300-321, 355) currently bail out from convert with withInfo(...) + return None for cases that are knowable from the expression alone (unsupported datatype, EvalMode.TRY on Remainder, negative-scale Decimal on Round, Float / Double on Round, etc.).
The audit-comet-expression skill requires expression-shape restrictions to be declared as Unsupported(Some(reason)) / Incompatible(Some(reason)) branches in getSupportLevel so that:
- EXPLAIN surfaces the reason at planning time
- the auto-generated compatibility doc picks them up
- the dispatcher can route around them
This was deferred from #4486 because arithmetic.scala is load-bearing and the fix is mechanical but wide.
2. UnaryPositive has no serde on Spark 3.4 / 3.5
Projections containing +col silently disable Comet for the enclosing projection on Spark 3.4 / 3.5 because there is no CometUnaryPositive registration. On Spark 4.0+ UnaryPositive is RuntimeReplaceable and the optimizer removes it before serde, so the gap is transparent there. Wire a passthrough serde for the 3.4 / 3.5 shim path so the projection stays native.
3. hex / unhex Spark 4.x collation propagation
Spark 4.x widens Hex / Unhex input types to StringTypeWithCollation and preserves collation in dataType. CometHex / CometUnhex pass expr.dataType to the native SparkHex / spark_unhex UDFs, which always return Utf8, so collation does not propagate. Per the audit skill, the support level should be Incompatible(Some(reason)) for non-default collations rather than the current Compatible. Cross-reference #2190 / #4496.
Medium priority (robustness / coverage)
4. greatest / least do not gate input types
Registered as bare CometScalarFunction(\"greatest\") / CometScalarFunction(\"least\") in QueryPlanSerde.scala. Spark accepts interval inputs (and a few Spark-specific orderings) that the native UDF may not handle; there is no explicit fallback path, so a runtime error from the native UDF is the failure mode. Add an input-type guard that falls back to Spark for the unsupported cases.
5. ceil(expr, scale) / floor(expr, scale) (RoundCeil / RoundFloor) not wired
The two-argument forms always fall back to Spark. Wire them via the same path as Round.
6. BRound (HALF_EVEN rounding) not wired
Listed as [ ] bround in the support doc. Banker's rounding is independent of the BigDecimal-via-toString issue that blocks Float / Double Round, so the integer / decimal paths could be supported.
7. signum rejects interval inputs
Spark Signum accepts DayTimeIntervalType / YearMonthIntervalType via inputTypes; Comet only handles DoubleType. Either add an explicit fallback or extend the native path.
Surfaced by the audit-comet-expression skill run in #4486.
Activity
andygrove commentedon Jun 29, 2026
Item #3 (hex/unhex collation) — investigated, no change needed.
Hexoutput is restricted to the fixed ASCII alphabet0-9A-F, so its values are collation-invariant: identical hex strings compare equal and group identically under every collation, and the small uppercase alphabet is not reordered by realistic locale collations.UnhexreturnsBinaryType, which carries no collation at all.The only way collation could matter is a collation-aware operation consuming the hex result (e.g.
WHERE hex(x) = 'ab',ORDER BY hex(x)). Comet already declines to evaluate collation-aware string comparisons/sorts natively (QueryPlanSerde.scala:1051), so those fall back to Spark and produce correct results. The hex projection still runs natively.Net: Comet cannot produce a collation-related wrong answer for
hex/unhextoday, so they correctly stayCompatible. The original audit flag was a blanket-policy concern, not a defect. (Contrastconcat/reverse, which are gated for the same theoretical reason and are arguably over-gated.)andygrove commentedon Jun 29, 2026
Item #2 (UnaryPositive serde on Spark 3.4 / 3.5) — investigated, no change needed.
The premise that
+colreaches serde on Spark 3.4 / 3.5 does not hold. The default optimizer batch includesRemoveDispensableExpressions, which unconditionally rewritesUnaryPositive(child) => child(sql/catalyst/.../optimizer/expressions.scala), soUnaryPositiveis stripped before Comet ever sees the plan. This mirrors the Spark 4.0+ path where it isRuntimeReplaceableand removed by the optimizer.Verified empirically:
SELECT +_1, +_2, +_3, +_4 FROM tblruns fully natively on Spark 3.5 with no code change, even withspark.comet.exec.scalaUDF.codegen.enabled=false. The enclosing projection stays native because the+is already gone by serde time.Consequently the
CometUnaryPositivecodegen-dispatch registration added in #4538 is effectively dead code in normal queries: it is only reachable if a user disablesRemoveDispensableExpressionsviaspark.sql.optimizer.excludedRules, and even then only when the codegen dispatcher is enabled. No correctness gap exists, so no serde change is required.andygrove commentedon Jun 29, 2026
Item #4 (greatest / least input-type gating) — spun out to #4756.
Investigation found no live defect: on Spark 3.5 and 4.0, every Spark-accepted input type either runs natively and matches Spark (numerics incl. NaN, decimal, string, date, timestamp, binary, arrays, structs, null-only) or falls back gracefully (intervals, collated strings). The issue's proposed scalar-only gating would regress the array / struct / decimal cases that work today.
Deferred to #4756 to revisit once the in-flight native interval-support PRs land (intervals currently fall back via non-serializable types; that changes once they serialize), and to add regression tests for the verified behavior.
andygrove commentedon Jun 29, 2026
Items #5 (
ceil/floorwith scale = RoundCeil/RoundFloor) and #6 (BRound) — investigated together; no correctness defect, but native support is blocked on adding rounding modes tospark_round.Verified current behavior on Spark 3.5 (parquet-backed columns so the projection can run natively):
ceil(x, scale)/floor(x, scale)(RoundCeil/RoundFloor)bround(x, scale)(BRound, HALF_EVEN)CometCodegenDispatch(codegen flag on, the default)bround(...)withspark.comet.exec.scalaUDF.codegen.enabled=falseNeither is a correctness defect: RoundCeil/RoundFloor fall back to Spark, and BRound is handled correctly by the codegen dispatcher (which runs Spark's own
doGenCodein a native kernel).Blocker for true-native support. The native
spark_round(native/spark-expr/src/math_funcs/round.rs) implements HALF_UP only (the integer/decimal macros round half-away-from-zero; floats delegate to DataFusion'sRoundFunc, also HALF_UP). So "wire via the same path asRound" is not viable as written — that path would silently apply HALF_UP and produce wrongbround/ceil/floorresults. Native support requires adding HALF_EVEN / CEILING / FLOOR modes tospark_roundfor the integer and decimal paths. Float/Double would stayUnsupported, same BigDecimal-via-toStringreason that already blocksRoundon float/double (as #6 notes).Nuance for #6. BRound via codegen dispatch currently covers all numeric types incl. float/double. A pure-native int/decimal impl would gate float/double to fallback unless codegen dispatch is retained for those, i.e. a partial coverage regression for the codegen-on case.
Unified approach when this is picked up. One native change serves both items: add a
RoundingModeparameter (HALF_UP / HALF_EVEN / CEILING / FLOOR) tospark_roundfor int+decimal, thread it through proto, then addCometRoundCeil/CometRoundFloorserdes and optionally switchCometBRoundto native, all gating float/double + negative-scale decimal asUnsupportedto mirrorCometRound.Net: #5 is a clean coverage win (zero native accel today); #6 is marginal (already correct via codegen dispatch). Left as-is for now per investigation; no code change.
andygrove commentedon Jun 29, 2026
Item #7 (
signumrejects interval inputs) — investigated; no live defect, folded into #4756.Probed on Spark 3.5:
signum(double)andsignum(int)run natively and match Spark; day-time / year-month interval inputs fall back gracefully (intervals do not serialize), so there is no runtime error today.Like
greatest/least(item #4), this only becomes relevant once the in-flight native interval-support PRs land, at which pointsignum(interval)will serialize and reach the native UDF (which currently handles only double). Added to #4756 as a deferred item: once intervals serialize, either extend the nativesignumpath to take the sign of the interval's underlying integer representation (months / micros) or add an explicitgetSupportLevelgate.andygrove commentedon Jun 29, 2026
Closing summary
All seven follow-up items have been investigated. None turned out to be a true correctness defect; closing the tracker accordingly. Each item was probed empirically (Comet vs Spark, native-vs-fallback) before reaching a conclusion.
arithmetic.scalaconvert-time fallbacks intogetSupportLeveltry_mod). The one residual,CometUnaryMinus, is addressed in #4759.UnaryPositiveserde on Spark 3.4 / 3.5RemoveDispensableExpressionsremovesUnaryPositivebefore serde on all supported versions, so+colprojections already stay native.hex/unhexcollationhexoutput is collation-invariant (0-9A-F);unhexreturnsBinaryType. Any collation-aware downstream op already falls back.greatest/leastinput gatingceil/floorwith scale (RoundCeil/RoundFloor)spark_round(currently HALF_UP only).BRound(HALF_EVEN)spark_round.signuminterval inputsFollow-ups carried forward
CometUnaryMinus.greatest/leastandsignuminterval handling once native interval support lands, plus regression tests.spark_round) remain available as future enhancements; details captured in the comments above.Closing as there are no true correctness issues to track here.