You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by gi...@apache.org on 2026/10/03 15:58:44 UTC
(datafusion-comet) branch gh-readonly-queue/main/pr-6556-432523fb59efe75b208ae5688eef6012c208a4d6 created (now 2f7e1d3be1)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-6556-432523fb59efe75b208ae5688eef6012c208a4d6
in repository https://gitbox.apache.org/repos/asf/datafusion-comet.git
at 2f7e1d3be1 fix: return -0.0 for signum(-0.0), as Spark does (#6556)
This branch includes the following new commits:
new 2f7e1d3be1 fix: return -0.0 for signum(-0.0), as Spark does (#6556)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org
(datafusion-comet) 01/01: fix: return -0.0 for signum(-0.0), as Spark does (#6556)
Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-6556-432523fb59efe75b208ae5688eef6012c208a4d6
in repository https://gitbox.apache.org/repos/asf/datafusion-comet.git
commit 2f7e1d3be1e31b5dca3e73ddde9c5cef6da63973
Author: Feng Zhang <fe...@apache.org>
AuthorDate: Sat Oct 3 15:58:21 2026 +0000
fix: return -0.0 for signum(-0.0), as Spark does (#6556)
* fix: return -0.0 for signum(-0.0), as Spark does
DataFusion's signum returns 0.0 for both zeros, where Math.signum returns
the zero it is given. Use a Comet kernel instead. Interval inputs, which
failed at execution, now fall back.
Closes #6522
* add -0.0 literal to the query example
---
.../expression-audits/math_funcs.md | 1 +
native/spark-expr/src/comet_scalar_funcs.rs | 5 +
native/spark-expr/src/lib.rs | 6 +-
native/spark-expr/src/math_funcs/mod.rs | 2 +
native/spark-expr/src/math_funcs/signum.rs | 115 +++++++++++++++++++++
.../org/apache/comet/serde/QueryPlanSerde.scala | 2 +-
.../main/scala/org/apache/comet/serde/math.scala | 25 ++++-
.../sql-tests/expressions/math/signum.sql | 28 ++++-
.../apache/comet/CometFloatSemanticsSuite.scala | 6 +-
9 files changed, 179 insertions(+), 11 deletions(-)
diff --git a/docs/source/contributor-guide/expression-audits/math_funcs.md b/docs/source/contributor-guide/expression-audits/math_funcs.md
index 984a0468fd..2810abac90 100644
--- a/docs/source/contributor-guide/expression-audits/math_funcs.md
+++ b/docs/source/contributor-guide/expression-audits/math_funcs.md
@@ -234,6 +234,7 @@ Internal fused expression that replaces the `CheckOverflow(Cast(expr, Decimal128
## signum
- Spark 3.4.3, 3.5.8, 4.0.1, 4.1.1 (audited 2026-05-27): `Signum(child)` over `DoubleType`. Spark also restricts to the two interval types via `inputTypes`; Comet handles only the `Double` case via DataFusion `signum`.
+- Spark 3.4, 3.5, 4.0, 4.1, 4.2 (2026-10-02, [#6522](https://github.com/apache/datafusion-comet/issues/6522)): `Math.signum` returns the zero it is given, so `signum(-0.0)` is `-0.0`, where DataFusion's `signum` returns `0.0`. Comet now uses its own `spark_signum` kernel for the `Double` case. Interval inputs fall back; DataFusion's `signum` failed on them at execution.
## sin
diff --git a/native/spark-expr/src/comet_scalar_funcs.rs b/native/spark-expr/src/comet_scalar_funcs.rs
index 7b3c980639..4ed5e561f7 100644
--- a/native/spark-expr/src/comet_scalar_funcs.rs
+++ b/native/spark-expr/src/comet_scalar_funcs.rs
@@ -25,6 +25,7 @@ use crate::math_funcs::checked_arithmetic::{checked_add, checked_div, checked_mu
use crate::math_funcs::log::spark_log;
use crate::math_funcs::modulo_expr::spark_modulo;
use crate::math_funcs::pow::spark_pow;
+use crate::math_funcs::signum::spark_signum;
use crate::math_funcs::sqrt::spark_sqrt;
use crate::{
spark_ceil, spark_day_name, spark_decimal_div, spark_decimal_integral_div, spark_floor,
@@ -224,6 +225,10 @@ pub fn create_comet_physical_fun_with_eval_mode(
let func = Arc::new(spark_pow);
make_comet_scalar_udf!("pow", func, without data_type)
}
+ "spark_signum" => {
+ let func = Arc::new(spark_signum);
+ make_comet_scalar_udf!("spark_signum", func, without data_type)
+ }
"spark_sqrt" => {
let func = Arc::new(spark_sqrt);
make_comet_scalar_udf!("spark_sqrt", func, without data_type)
diff --git a/native/spark-expr/src/lib.rs b/native/spark-expr/src/lib.rs
index 991fd5e52e..b1447e37d6 100644
--- a/native/spark-expr/src/lib.rs
+++ b/native/spark-expr/src/lib.rs
@@ -97,9 +97,9 @@ pub use json_funcs::{FromJson, JsonArrayLength, ToJson};
pub use math_funcs::{
abs, checked_add, checked_div, checked_mul, checked_sub, create_modulo_expr,
create_negate_expr, spark_ceil, spark_decimal_div, spark_decimal_integral_div, spark_floor,
- spark_log, spark_make_decimal, spark_modulo, spark_pow, spark_round, spark_sqrt, spark_unhex,
- spark_unscaled_value, CheckOverflow, CheckedBinaryExpr, DecimalRescaleCheckOverflow,
- NegativeExpr, WideDecimalBinaryExpr, WideDecimalOp,
+ spark_log, spark_make_decimal, spark_modulo, spark_pow, spark_round, spark_signum, spark_sqrt,
+ spark_unhex, spark_unscaled_value, CheckOverflow, CheckedBinaryExpr,
+ DecimalRescaleCheckOverflow, NegativeExpr, WideDecimalBinaryExpr, WideDecimalOp,
};
pub use query_context::{create_query_context_map, QueryContext, QueryContextMap};
pub use string_funcs::*;
diff --git a/native/spark-expr/src/math_funcs/mod.rs b/native/spark-expr/src/math_funcs/mod.rs
index 1c993cd2fe..9b72966dc6 100644
--- a/native/spark-expr/src/math_funcs/mod.rs
+++ b/native/spark-expr/src/math_funcs/mod.rs
@@ -27,6 +27,7 @@ pub mod modulo_expr;
mod negative;
pub(crate) mod pow;
mod round;
+pub(crate) mod signum;
pub(crate) mod sqrt;
pub(crate) mod unhex;
mod utils;
@@ -45,6 +46,7 @@ pub use modulo_expr::{create_modulo_expr, spark_modulo};
pub use negative::{create_negate_expr, NegativeExpr};
pub use pow::spark_pow;
pub use round::spark_round;
+pub use signum::spark_signum;
pub use sqrt::spark_sqrt;
pub use unhex::spark_unhex;
pub use wide_decimal_binary_expr::{WideDecimalBinaryExpr, WideDecimalOp};
diff --git a/native/spark-expr/src/math_funcs/signum.rs b/native/spark-expr/src/math_funcs/signum.rs
new file mode 100644
index 0000000000..3f5670e4d0
--- /dev/null
+++ b/native/spark-expr/src/math_funcs/signum.rs
@@ -0,0 +1,115 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::{ArrayRef, AsArray, Float64Array};
+use arrow::datatypes::Float64Type;
+use datafusion::common::{DataFusionError, ScalarValue};
+use datafusion::physical_plan::ColumnarValue;
+use std::sync::Arc;
+
+/// Spark-compatible `signum`, matching `java.lang.Math.signum`: a zero is returned as it is, so
+/// `-0.0` keeps its sign. DataFusion's own `signum` returns `0.0` for both zeros.
+pub fn spark_signum(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(format!(
+ "spark_signum requires 1 argument, got {}",
+ args.len()
+ )));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => {
+ let values = array.as_primitive_opt::<Float64Type>().ok_or_else(|| {
+ DataFusionError::Internal(format!(
+ "spark_signum expected Float64, got {:?}",
+ array.data_type()
+ ))
+ })?;
+ let result: Float64Array = values.unary(signum);
+ Ok(ColumnarValue::Array(Arc::new(result) as ArrayRef))
+ }
+ ColumnarValue::Scalar(ScalarValue::Float64(v)) => {
+ Ok(ColumnarValue::Scalar(ScalarValue::Float64(v.map(signum))))
+ }
+ ColumnarValue::Scalar(other) => Err(DataFusionError::Internal(format!(
+ "spark_signum expected Float64 scalar, got {other:?}",
+ ))),
+ }
+}
+
+/// `f64::signum` returns `1.0` and `-1.0` for the two zeros, so a zero is passed through instead.
+/// NaN is not equal to zero, and `f64::signum` returns NaN for it.
+#[inline]
+fn signum(v: f64) -> f64 {
+ if v == 0.0 {
+ v
+ } else {
+ v.signum()
+ }
+}
+
+#[cfg(test)]
+mod test {
+ use super::*;
+ use arrow::array::Array;
+
+ #[test]
+ fn test_spark_signum_keeps_the_sign_of_zero() {
+ let input = Float64Array::from(vec![
+ Some(5.5),
+ Some(-5.5),
+ Some(0.0),
+ Some(-0.0),
+ Some(f64::NAN),
+ Some(f64::INFINITY),
+ Some(f64::NEG_INFINITY),
+ None,
+ ]);
+ let result = spark_signum(&[ColumnarValue::Array(Arc::new(input))]).unwrap();
+ let ColumnarValue::Array(result) = result else {
+ unreachable!()
+ };
+ let result = result.as_primitive::<Float64Type>();
+ assert_eq!(result.value(0), 1.0);
+ assert_eq!(result.value(1), -1.0);
+ // `0.0 == -0.0`, so the zeros are compared by their bits.
+ assert_eq!(result.value(2).to_bits(), 0.0_f64.to_bits());
+ assert_eq!(result.value(3).to_bits(), (-0.0_f64).to_bits());
+ assert!(result.value(4).is_nan());
+ assert_eq!(result.value(5), 1.0);
+ assert_eq!(result.value(6), -1.0);
+ assert!(result.is_null(7));
+ }
+
+ #[test]
+ fn test_spark_signum_scalar_negative_zero() {
+ let result =
+ spark_signum(&[ColumnarValue::Scalar(ScalarValue::Float64(Some(-0.0)))]).unwrap();
+ let ColumnarValue::Scalar(ScalarValue::Float64(Some(result))) = result else {
+ unreachable!()
+ };
+ assert_eq!(result.to_bits(), (-0.0_f64).to_bits());
+ }
+
+ #[test]
+ fn test_spark_signum_scalar_null() {
+ let result = spark_signum(&[ColumnarValue::Scalar(ScalarValue::Float64(None))]).unwrap();
+ let ColumnarValue::Scalar(ScalarValue::Float64(None)) = result else {
+ unreachable!()
+ };
+ }
+}
diff --git a/spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala b/spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala
index b41b37a011..bcdb37122b 100644
--- a/spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala
+++ b/spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala
@@ -151,7 +151,7 @@ object QueryPlanSerde extends Logging with CometExprShim with CometTypeShim {
classOf[Rint] -> CometScalarFunction("rint"),
classOf[Round] -> CometRound,
classOf[Sec] -> CometScalarFunction("sec"),
- classOf[Signum] -> CometScalarFunction("signum"),
+ classOf[Signum] -> CometSignum,
classOf[Sin] -> CometScalarFunction("sin"),
classOf[Sinh] -> CometScalarFunction("sinh"),
classOf[Sqrt] -> CometSqrt,
diff --git a/spark/src/main/scala/org/apache/comet/serde/math.scala b/spark/src/main/scala/org/apache/comet/serde/math.scala
index ed0efb1637..e1d305cadf 100644
--- a/spark/src/main/scala/org/apache/comet/serde/math.scala
+++ b/spark/src/main/scala/org/apache/comet/serde/math.scala
@@ -19,7 +19,7 @@
package org.apache.comet.serde
-import org.apache.spark.sql.catalyst.expressions.{Abs, Add, Atan2, Attribute, BRound, Ceil, CheckOverflow, Conv, Expression, Floor, Greatest, Hex, Hypot, If, Least, LessThanOrEqual, Literal, Log, Log10, Log1p, Log2, Logarithm, NaNvl, Pmod, Pow, Sqrt, UnaryPositive, Unhex, WidthBucket}
+import org.apache.spark.sql.catalyst.expressions.{Abs, Add, Atan2, Attribute, BRound, Ceil, CheckOverflow, Conv, Expression, Floor, Greatest, Hex, Hypot, If, Least, LessThanOrEqual, Literal, Log, Log10, Log1p, Log2, Logarithm, NaNvl, Pmod, Pow, Signum, Sqrt, UnaryPositive, Unhex, WidthBucket}
import org.apache.spark.sql.types.{DecimalType, DoubleType, NumericType}
import org.apache.comet.serde.QueryPlanSerde.{exprToProtoInternal, scalarFunctionExprToProto, scalarFunctionExprToProtoWithReturnType, serializeDataType}
@@ -239,6 +239,29 @@ object CometSqrt extends CometExpressionSerde[Sqrt] {
}
}
+// Uses a custom spark_signum UDF because DataFusion's own `signum` returns 0.0 for both zeros,
+// while Spark's Signum (java.lang.Math.signum) returns the zero it is given, so -0.0 keeps its
+// sign. The kernel takes doubles only. Spark's Signum also accepts the two interval types, which
+// DataFusion's `signum` failed on at execution, so those fall back. As with CometSqrt, the
+// Comet-only name needs its return type set explicitly.
+object CometSignum extends CometExpressionSerde[Signum] {
+
+ override def getUnsupportedReasons(): Seq[String] = Seq("Only `DoubleType` is supported")
+
+ override def getSupportLevel(expr: Signum): SupportLevel = expr.child.dataType match {
+ case DoubleType => Compatible()
+ case other => Unsupported(Some(s"signum does not support input type $other"))
+ }
+
+ override def convert(
+ expr: Signum,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[ExprOuterClass.Expr] = {
+ val childExpr = exprToProtoInternal(expr.child, inputs, binding)
+ scalarFunctionExprToProtoWithReturnType("spark_signum", DoubleType, false, childExpr)
+ }
+}
+
sealed trait MathExprBase {
protected def nullIfNegative(expression: Expression): Expression = {
val zero = Literal.default(expression.dataType)
diff --git a/spark/src/test/resources/sql-tests/expressions/math/signum.sql b/spark/src/test/resources/sql-tests/expressions/math/signum.sql
index 04db8c3461..c8f40887c9 100644
--- a/spark/src/test/resources/sql-tests/expressions/math/signum.sql
+++ b/spark/src/test/resources/sql-tests/expressions/math/signum.sql
@@ -26,4 +26,30 @@ SELECT signum(d) FROM test_signum
-- literal arguments
query
-SELECT signum(-5.0), signum(5.0), signum(0.0), signum(NULL)
+SELECT signum(-5.0), signum(5.0), signum(0.0), signum(-0.0), signum(NULL)
+
+-- java.lang.Math.signum returns the zero it is given, so -0.0 keeps its sign
+-- (https://github.com/apache/datafusion-comet/issues/6522)
+statement
+CREATE TABLE test_signum_zero(d double, f float) USING parquet
+
+statement
+INSERT INTO test_signum_zero VALUES (double('-0.0'), float('-0.0')), (double('0.0'), float('0.0')), (-1.5, float('-1.5')), (2.5, float('2.5')), (NULL, NULL)
+
+query
+SELECT signum(d), signum(f) FROM test_signum_zero
+
+-- the string form shows the sign of a zero
+query
+SELECT CAST(signum(d) AS STRING), CAST(signum(f) AS STRING) FROM test_signum_zero
+
+-- Spark's Signum also accepts the two interval types. The native kernel takes doubles only, so
+-- these fall back.
+statement
+CREATE TABLE test_signum_iv(m int) USING parquet
+
+statement
+INSERT INTO test_signum_iv VALUES (18), (-18), (0), (NULL)
+
+query expect_fallback(signum does not support input type)
+SELECT signum(make_ym_interval(0, m)), signum(make_dt_interval(m, 0, 0, 0)) FROM test_signum_iv
diff --git a/spark/src/test/scala/org/apache/comet/CometFloatSemanticsSuite.scala b/spark/src/test/scala/org/apache/comet/CometFloatSemanticsSuite.scala
index 89c7d6ee81..8715e06cc8 100644
--- a/spark/src/test/scala/org/apache/comet/CometFloatSemanticsSuite.scala
+++ b/spark/src/test/scala/org/apache/comet/CometFloatSemanticsSuite.scala
@@ -358,9 +358,5 @@ object CometFloatSemanticsSuite {
KnownGap(
issue(5312),
"collect_set before Spark 4.2 treats -0.0 and 0.0 as one value and NaNs as distinct.",
- c => in("aggregate", "collect_set")(c) && !CometSparkSessionExtensions.isSpark42Plus),
- KnownGap(
- issue(6522),
- "signum(-0.0) returns 0.0, where Spark returns -0.0.",
- in("expression", "signum")))
+ c => in("aggregate", "collect_set")(c) && !CometSparkSessionExtensions.isSpark42Plus))
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org