chore: Introduce ANSI support for remainder operation by rishvin · Pull Request #1971 · apache/datafusion-comet

rishvin · 2025-07-01T18:50:29Z

Which issue does this PR close?

Closes #532

Rationale for this change

Spark in ANSI mode can throw division by zero exception for remainder, however, the remainder implementation in Comet is not ANSI compliant and rather than throwing division by zero, returns NULL. This PR introduces remainder that is compliant with Spark behavior, both in ANSI and non-ANSI mode.

Ref doc.
As per the doc,
try_mod: identical to the remainder operator %, except that it returns NULL result instead of throwing an exception on dividing 0.

What changes are included in this PR?

Adds a new physical expression ModuloExpr. This expression supports both ANSI and non-ANSI mode (via fail_on_error flag). In ANSI mode, it translates the Arrow's division by zero error to Spark compliant error message.
Removes the call to nullIfWhenPrimitive, when serializing the remainder exception, and implements equivalent functionality in ModuloExpr. This method set null values for RHS record batches that have zero values in them. This is required for non-ANSI mode, otherwise Arrow's remainder implementation will throw divide by zero exception.
Adds rust and scala tests cases to verify remainder behaviour both in ANSI and non-ANSI mode.

How are these changes tested?

Added rust and scala tests to verify the behavior.

Results from spark-shell,

Without Comet (ANSI/non-ANSI)

With Comet (ANSI/non-ANSI)

rishvin · 2025-07-01T19:10:19Z

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

+    fn evaluate_statistics(&self, children: &[&Distribution]) -> Result<Distribution> {
+        let (left, right) = (children[0], children[1]);
+
+        if let (Gaussian(left), Gaussian(right)) = (left, right) {


Borrowed these changes from BinaryExpr expression, here.

rishvin · 2025-07-01T19:13:04Z

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

+        (Ok(DataType::Decimal128(p1, s1)), Ok(DataType::Decimal128(p2, s2)))
+            if max(s1, s2) as u8 + max(p1 - s1 as u8, p2 - s2 as u8) > DECIMAL128_MAX_PRECISION =>
+        {
+            let left = Arc::new(Cast::new(


Borrowed this code from

datafusion-comet/native/core/src/execution/planner.rs

Line 885 in 2dd6ca2

let data_type = return_type.map(to_arrow_datatype).unwrap();

. We were doing these casting before creating modulo operation.

codecov-commenter · 2025-07-01T20:20:22Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.77%. Comparing base (f09f8af) to head (5ff9643).
⚠️ Report is 344 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1971      +/-   ##
============================================
+ Coverage     56.12%   58.77%   +2.64%     
- Complexity      976     1154     +178     
============================================
  Files           119      134      +15     
  Lines         11743    13054    +1311     
  Branches       2251     2417     +166     
============================================
+ Hits           6591     7672    +1081     
- Misses         4012     4155     +143     
- Partials       1140     1227      +87

☔ 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.

rishvin · 2025-07-02T00:25:57Z

Note that vanilla spark message is a bit misleading. It says - Use try_divide(), however, it should have said - Use try_mod(). I checked this message in spark-4 also, the message even today says Use try_divide(). This is spark specific message problem, however, I just wanted to flag this out. Here is the sample from spark-4,

scala> spark.sql("SELECT mod(10, 0)").show()
     |
org.apache.spark.SparkArithmeticException: [DIVIDE_BY_ZERO] Division by zero. Use `try_divide` to tolerate divisor being 0 and return NULL instead. If necessary set "spark.sql.ansi.enabled" to "false" to bypass this error. SQLSTATE: 22012
== SQL (line 1, position 8) ==
SELECT mod(10, 0)

Filed minor ticket for spark: https://issues.apache.org/jira/browse/SPARK-52659

rishvin · 2025-07-03T19:27:51Z

@andygrove : Could you please review and check if this is how you envisioned these changes?

andygrove · 2025-07-03T19:45:18Z

@andygrove : Could you please review and check if this is how you envisioned these changes?

Thanks @rishvin. I took a first pass through, and this looks great. Thanks for adding comprehensive tests both in Scala and Rust. I do wonder if we should implement the expression as a ScalarUDFImpl rather than PhysicalExpr though. What do you think?

rishvin · 2025-07-04T03:17:55Z

@andygrove : Could you please review and check if this is how you envisioned these changes?

Thanks @rishvin. I took a first pass through, and this looks great. Thanks for adding comprehensive tests both in Scala and Rust. I do wonder if we should implement the expression as a ScalarUDFImpl rather than PhysicalExpr though. What do you think?

Thanks @andygrove , yes SGTM. Let me rewrite these changes as UDF.

rishvin · 2025-07-08T18:22:15Z

@andygrove I have made the changes, could you please recheck.

andygrove

LGTM. Thanks @rishvin.

andygrove · 2025-07-10T22:11:48Z

@parthchandra @mbutrovich would you also like to review?

native/spark-expr/src/comet_scalar_funcs.rs

mbutrovich

Thanks @rishvin! LGTM

chore: ANSI support for remainder operation.

a8c281b

rishvin mentioned this pull request Jul 1, 2025

Add ANSI support for Remainder #532

Closed

rishvin commented Jul 1, 2025

View reviewed changes

Apply cargo fmt

e4be397

rishvin changed the title ~~chore: ANSI support for remainder operation.~~ chore: Introduce ANSI support for remainder operation Jul 2, 2025

Fixed clippy errors

e9b0439

Rewrite modulo physical expression to scalar UDF

5153fce

andygrove approved these changes Jul 9, 2025

View reviewed changes

upmerge

5ff9643

mbutrovich self-requested a review July 11, 2025 14:47

mbutrovich reviewed Jul 11, 2025

View reviewed changes

native/spark-expr/src/comet_scalar_funcs.rs Show resolved Hide resolved

mbutrovich approved these changes Jul 11, 2025

View reviewed changes

mbutrovich merged commit 496cad9 into apache:main Jul 11, 2025
91 checks passed

rishvin deleted the rem_ansi_support branch July 11, 2025 19:28

This was referenced Oct 12, 2025

bug: ANSI mode is not supported in Remainder function (despite having support on native side) #2555

Closed

feat:support ansi mode remainder function #2556

Merged

rishvin mentioned this pull request Dec 5, 2025

Handle remainder by zero error in spark_modulo function for Spark 4.1 and later. #2855

Open

coderfender pushed a commit to coderfender/datafusion-comet that referenced this pull request Dec 13, 2025

chore: Introduce ANSI support for remainder operation (apache#1971)

c331de4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Introduce ANSI support for remainder operation#1971

chore: Introduce ANSI support for remainder operation#1971
mbutrovich merged 5 commits intoapache:mainfrom
rishvin:rem_ansi_support

rishvin commented Jul 1, 2025 •

edited

Loading

Uh oh!

rishvin Jul 1, 2025 •

edited

Loading

Uh oh!

rishvin Jul 1, 2025

Uh oh!

codecov-commenter commented Jul 1, 2025 •

edited

Loading

Uh oh!

rishvin commented Jul 2, 2025 •

edited

Loading

Uh oh!

rishvin commented Jul 3, 2025

Uh oh!

andygrove commented Jul 3, 2025

Uh oh!

rishvin commented Jul 4, 2025

Uh oh!

rishvin commented Jul 8, 2025

Uh oh!

andygrove left a comment

Uh oh!

andygrove commented Jul 10, 2025

Uh oh!

Uh oh!

mbutrovich left a comment •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

rishvin commented Jul 1, 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!

rishvin Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishvin Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

rishvin commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rishvin commented Jul 3, 2025

Uh oh!

andygrove commented Jul 3, 2025

Uh oh!

rishvin commented Jul 4, 2025

Uh oh!

rishvin commented Jul 8, 2025

Uh oh!

andygrove left a comment

Choose a reason for hiding this comment

Uh oh!

andygrove commented Jul 10, 2025

Uh oh!

Uh oh!

mbutrovich left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rishvin commented Jul 1, 2025 •

edited

Loading

rishvin Jul 1, 2025 •

edited

Loading

codecov-commenter commented Jul 1, 2025 •

edited

Loading

rishvin commented Jul 2, 2025 •

edited

Loading

mbutrovich left a comment •

edited

Loading