sunchao commented on code in PR #6447:
URL: https://github.com/apache/datafusion-comet/pull/6447#discussion_r4178233892
##########
native/spark-expr/src/comet_scalar_funcs.rs:
##########
@@ -315,6 +316,11 @@ pub fn create_comet_physical_fun_with_eval_mode(
// SparkMakeTime already throws on invalid input, so accept the flag
here rather
// than falling through to the registry fail-closed path.
"make_time" =>
Ok(Arc::new(ScalarUDF::new_from_impl(SparkMakeTime::new()))),
+ // Floats, and arrays and structs holding them, need Spark's float
ordering. Other types
+ // keep DataFusion's `greatest` and `least` from the registry.
+ "greatest" | "least" if SparkGreatestLeast::handles(&data_type) =>
Ok(Arc::new(
Review Comment:
[P2] Preserve Spark’s reuse of equivalent extrema expressions before
enabling this routing unconditionally. For a Parquet row `(a, b) = (0.0D,
-0.0D)`, `SELECT CAST(greatest(a,b) AS STRING), CAST(greatest(b,a) AS STRING)
FROM t` returns `('0.0', '0.0')` in Spark with default
`spark.sql.subexpressionElimination.enabled=true`. The new native calls retain
their respective first arguments, producing `(0.0, -0.0)` and therefore
different strings. The previous DataFusion implementation returned positive
zero for both calls, so this worsens a previously matching query. This affects
observable results and downstream expressions. The compatibility guide
documents the divergence, but the default native path remains enabled. Please
preserve Spark’s shared representative for equivalent expressions or fall back
for affected projections, and test both argument orders together.
Evidence: Fresh Spark 3.5.9 execution over Spark-written Parquet returned
`Row(g1='0.0', g2='0.0')` with expression reuse enabled and opposite signs when
disabled. An exact-head `PhysicalPlanner::create_expr` probe produced bits
`[0x0000000000000000, 0x8000000000000000]` and failed the Spark-result
assertion. A before/after kernel probe confirmed the previous DataFusion
implementation produced `[0, 0]`. Spark’s supported-version
`Greatest.canonicalized` treats argument orders as equivalent. Reproductions:
`/tmp/review6447-root-planner-probe.rs` and
`/tmp/review6447-root-expression-probe.rs`. Logs:
`/tmp/review6447-root-native-planner-probe.log`,
`/tmp/review6447-root-native-probe.log`, and
`/tmp/review6447-36546-root-spark-cse.log`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
