feat: Add support for `abs` #2689

andygrove · 2025-11-05T02:11:08Z

Which issue does this PR close?

Closes #666

Rationale for this change

This PR is based on #2595

What changes are included in this PR?

The differences since #2595 are:

Revert test changes
Implement new tests
Fallback to Spark for unsupported types DayTimeIntervalType and YearMonthIntervalType
Fix a potential bug with null scalar handling (although I am not sure if this code path even gets used with the Spark integration)

How are these changes tested?

andygrove · 2025-11-05T02:42:59Z

spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala

-  // https://github.com/apache/datafusion-comet/issues/666
-  ignore("abs") {


I implemented new tests using the fuzz data generator

andygrove · 2025-11-05T02:45:46Z

@hsiang-c could you review?

codecov-commenter · 2025-11-05T03:09:04Z

Codecov Report

❌ Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.32%. Comparing base (f09f8af) to head (a8c1d0a).
⚠️ Report is 661 commits behind head on main.

Files with missing lines	Patch %	Lines
...k/src/main/scala/org/apache/comet/serde/math.scala	85.71%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2689      +/-   ##
============================================
+ Coverage     56.12%   59.32%   +3.20%     
- Complexity      976     1448     +472     
============================================
  Files           119      147      +28     
  Lines         11743    13816    +2073     
  Branches       2251     2370     +119     
============================================
+ Hits           6591     8197    +1606     
- Misses         4012     4393     +381     
- Partials       1140     1226      +86

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

hsiang-c · 2025-11-05T06:12:21Z

native/spark-expr/src/math_funcs/abs.rs

+            | ScalarValue::UInt32(_)
+            | ScalarValue::UInt64(_) => Ok(args[0].clone()),
+            ScalarValue::Int8(a) => match a {
+                None => Ok(args[0].clone()),


Good catch, thanks!

hsiang-c · 2025-11-05T06:13:44Z

native/spark-expr/src/math_funcs/abs.rs

+    #[test]
+    fn test_abs_i8_scalar() {
+        with_fail_on_error(|fail_on_error| {
+            let args = ColumnarValue::Scalar(ScalarValue::Int8(Some(i8::MIN)));


We might need a scalar test for the None case.

hsiang-c · 2025-11-05T06:18:36Z

spark/src/main/scala/org/apache/comet/serde/math.scala

+      case _: NumericType =>
+        Compatible()
+      case _ =>
+        // Spark supports NumericType, DayTimeIntervalType, and YearMonthIntervalType


hsiang-c · 2025-11-05T06:19:20Z

spark/src/test/scala/org/apache/comet/CometMathExpressionSuite.scala

+        StructField("c1", DataTypes.ShortType, nullable = true),
+        StructField("c2", DataTypes.IntegerType, nullable = true),
+        StructField("c3", DataTypes.LongType, nullable = true),
+        StructField("c4", DataTypes.FloatType, nullable = true),


Are NaNs included in the test data?

Yes, the fuzz generator generates edge cases like NaN, Infinity, -0.0 (optionally).

hsiang-c

LGTM, thanks Andy.

martin-g · 2025-11-05T09:37:28Z

spark/src/test/scala/org/apache/comet/CometMathExpressionSuite.scala

+        f.dataType == DataTypes.FloatType || f.dataType == DataTypes.DoubleType)) {
+      val col = field.name
+      checkSparkAnswerAndOperator(
+        s"SELECT $col, abs($col) FROM tbl WHERE signum($col) < 0 ORDER BY $col")


If the value of $col is -0.0 (a negative zero) then signum($col) would return 0.
Just checking whether this is the intended behavior because the test is about negative zero specifically.

Thanks @martin-g. I updated the test. Results:

+----+-------+ |c4 |abs(c4)| +----+-------+ |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | |-0.0|0.0 | +----+-------+

martin-g · 2025-11-05T09:41:05Z

native/spark-expr/src/math_funcs/abs.rs

+                    Ok(())
+                }
+                Err(e) => {
+                    if fail_on_error {


Is it possible that it would overflow for unsigned integers ?
IMO it should always panic here, i.e. leave only the else clause body.

updated, thanks

martin-g · 2025-11-05T09:52:09Z

native/spark-expr/src/math_funcs/abs.rs

+
+    fn with_fail_on_error<F: Fn(bool) -> Result<()>>(test_fn: F) {
+        for fail_on_error in [true, false] {
+            let _ = test_fn(fail_on_error);


This would ignore the returned Result.

Suggested change

let _ = test_fn(fail_on_error);

let _ = test_fn(fail_on_error)?;

Fixed. I couldn't add ? but added .expect

martin-g · 2025-11-05T09:54:05Z

native/spark-expr/src/math_funcs/abs.rs

+        false
+    };
+
+    match &args[0] {


There is no check that the function is called with at least one argument.

martin-g · 2025-11-05T09:59:19Z

native/spark-expr/src/math_funcs/abs.rs

+                },
+            },
+            ScalarValue::Int16(a) => match a {
+                None => Ok(args[0].clone()),


nit: The code for handling Int16/32/64 is very similar to Int8.
It could be extracted to a declarative macro.

native/spark-expr/src/math_funcs/abs.rs

comphead · 2025-11-05T16:42:11Z

native/spark-expr/src/math_funcs/abs.rs

+    #[test]
+    fn test_abs_f32_array() {
+        with_fail_on_error(|fail_on_error| {
+            let input = Float32Array::from(vec![Some(-1f32), Some(f32::MIN), Some(f32::MAX), None]);


can we also have NaN, Infinities?

Co-authored-by: Oleks V <comphead@users.noreply.github.com>

comphead · 2025-11-05T20:51:07Z

native/spark-expr/src/math_funcs/abs.rs

+    }
+
+    #[test]
+    fn test_abs_f32_array() {


Would be nice to have this edge case

scala> spark.sql("select abs(-0.0), abs(0.0)").show(false) +--------+--------+ |abs(0.0)|abs(0.0)| +--------+--------+ |0.0 |0.0 | +--------+--------+

Because Rust

fn main() { println!("{}", -0f32.abs()); } -0

Added. We also have specific Spark test for negative zero case.

comphead

Thanks @andygrove and @hsiang-c for supporting abs

hsiang-c and others added 13 commits October 16, 2025 14:32

Removed old ABS implementation

58d8c9a

Define Comet's ABS in Scala

d5b70c3

Implement Comet's ABS in rust

7b1965a

Enable ABS tests in legacy/ANSI mode

eb94cb6

Fix bit position b/c schema change in CometTestBase

ce1c1c2

Update docs

9cb2f6c

Fix style

6016f69

Update doc

1ddff98

Merge remote-tracking branch 'apache/main' into enable_abs

945cfc0

new test

3a48453

format

2351dfb

Revert

4952286

negative zero test

c414db9

andygrove marked this pull request as ready for review November 5, 2025 02:28

fix null scalar issue

4236dde

andygrove mentioned this pull request Nov 5, 2025

fix: re-enable Comet abs #2595

Closed

andygrove commented Nov 5, 2025

View reviewed changes

andygrove requested review from comphead, mbutrovich and parthchandra November 5, 2025 02:45

add new test to CI

0c1b172

fix

7fcfdd3

hsiang-c reviewed Nov 5, 2025

View reviewed changes

hsiang-c approved these changes Nov 5, 2025

View reviewed changes

martin-g reviewed Nov 5, 2025

View reviewed changes

andygrove added 3 commits November 5, 2025 08:02

fix bug in negative zero test

abddd9e

address review feedback

3089dca

address feedback

f0e9c6e

comphead reviewed Nov 5, 2025

View reviewed changes

native/spark-expr/src/math_funcs/abs.rs Show resolved Hide resolved

comphead reviewed Nov 5, 2025

View reviewed changes

native/spark-expr/src/math_funcs/abs.rs Show resolved Hide resolved

comphead reviewed Nov 5, 2025

View reviewed changes

native/spark-expr/src/math_funcs/abs.rs Outdated Show resolved Hide resolved

comphead reviewed Nov 5, 2025

View reviewed changes

native/spark-expr/src/math_funcs/abs.rs Outdated Show resolved Hide resolved

comphead reviewed Nov 5, 2025

View reviewed changes

native/spark-expr/src/math_funcs/abs.rs Outdated Show resolved Hide resolved

comphead reviewed Nov 5, 2025

View reviewed changes

andygrove and others added 4 commits November 5, 2025 12:40

Update native/spark-expr/src/math_funcs/abs.rs

50f899f

Co-authored-by: Oleks V <comphead@users.noreply.github.com>

Update native/spark-expr/src/math_funcs/abs.rs

25f86ff

Co-authored-by: Oleks V <comphead@users.noreply.github.com>

Update native/spark-expr/src/math_funcs/abs.rs

437cfdc

Co-authored-by: Oleks V <comphead@users.noreply.github.com>

address feedback

a596fc5

martin-g approved these changes Nov 5, 2025

View reviewed changes

comphead reviewed Nov 5, 2025

View reviewed changes

add negative zero in native code

a8c1d0a

comphead approved these changes Nov 6, 2025

View reviewed changes

andygrove merged commit fc3e6e9 into apache:main Nov 6, 2025
175 of 178 checks passed

andygrove deleted the enable_abs branch November 6, 2025 00:17

		// https://github.com/apache/datafusion-comet/issues/666
		ignore("abs") {

	let _ = test_fn(fail_on_error);
	let _ = test_fn(fail_on_error)?;

feat: Add support for abs #2689

feat: Add support for abs #2689

Uh oh!

Conversation

andygrove commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove commented Nov 5, 2025

Uh oh!

codecov-commenter commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hsiang-c left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

feat: Add support for `abs` #2689

feat: Add support for `abs` #2689

andygrove commented Nov 5, 2025 •

edited

Loading

codecov-commenter commented Nov 5, 2025 •

edited

Loading