fix: modulo op with negative zero divisor produces Nan #585

vaibhawvipul · 2024-06-18T12:38:01Z

Which issue does this PR close?

Closes #521 and #665.

Rationale for this change

What changes are included in this PR?

Improves compatibility with apache spark.

How are these changes tested?

Added a relevant test case.

comphead

imho it has to be fixed on DataFusion side. I can see it returns NaN

> select 1 % -0.0;
+------------------------+
| Int64(1) % Float64(-0) |
+------------------------+
| NaN                    |
+------------------------+

PG, Trino fails on such query,
DuckDB, Spark returns NULL

I'll start a ticket in DF, if community supports to go with NULL, so no action needed for this ticket, if they decide to fail the query we might need some custom handling

comphead · 2024-06-21T17:01:35Z

Filed apache/datafusion#11051 lets see

vaibhawvipul · 2024-06-21T17:11:09Z

imho it has to be fixed on DataFusion side. I can see it returns NaN
> select 1 % -0.0;
+------------------------+
| Int64(1) % Float64(-0) |
+------------------------+
| NaN                    |
+------------------------+
PG, Trino fails on such query, DuckDB, Spark returns NULL

I'll start a ticket in DF, if community supports to go with NULL, so no action needed for this ticket, if they decide to fail the query we might need some custom handling

Make sense. I will keep my PR as it then until we get some resolution on the datafusion ticket.

Thank you @comphead

kazuyukitanimura · 2024-06-21T19:50:43Z

The direct reason why NaN is produced by dividing by -0.0 is that EqualTo condition is failing in nullIfWhenPrimitive() of QueryPlanSerde

DataFusion changed their behavior several versions ago, to fail for dividing by zero. The DataFusion community recommendation to keep old behavior was to put if condition such as nullIfWhenPrimitive().

I would say we should do something like

if (expression.dataType == DoubleType) {
    If(Or(EqualTo(expression, zero), EqualTo(expression, negZero)), Literal.create(null, expression.dataType), expression)
} else {
    If(EqualTo(expression, zero), Literal.create(null, expression.dataType), expression)
}

If is also supported natively, so it won't be too much overhead (at least that was the case last time I tried). I would hesitate to re-implement whole operator only for negative zero case. I feel we should reuse DataFusion as much as possible

vaibhawvipul · 2024-06-22T02:44:35Z

The direct reason why NaN is produced by dividing by -0.0 is that EqualTo condition is failing in nullIfWhenPrimitive() of QueryPlanSerde

DataFusion changed their behavior several versions ago, to fail for dividing by zero. The DataFusion community recommendation to keep old behavior was to put if condition such as nullIfWhenPrimitive().

I would say we should do something like
if (expression.dataType == DoubleType) {
    If(Or(EqualTo(expression, zero), EqualTo(expression, negZero)), Literal.create(null, expression.dataType), expression)
} else {
    If(EqualTo(expression, zero), Literal.create(null, expression.dataType), expression)
}
If is also supported natively, so it won't be too much overhead (at least that was the case last time I tried). I would hesitate to re-implement whole operator only for negative zero case. I feel we should reuse DataFusion as much as possible

makes a lot of sense. I have made a fix as per your comments.

please review @kazuyukitanimura

vaibhawvipul · 2024-06-23T15:59:52Z

can someone please re-run the CI? It seems like a task failed because of network error.

cc @kazuyukitanimura @comphead

kazuyukitanimura

Thank you @vaibhawvipul

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

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

vaibhawvipul · 2024-06-27T20:33:36Z

@kazuyukitanimura ready for review

kazuyukitanimura · 2024-06-27T22:05:54Z

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

+        (-1.0, 0.0),
+        (0.0, -1.0)),
+      "t") {
+      checkSparkAnswerAndOperator("SELECT _1 == _2 FROM t")


We may need to test != and <=>as well, but can be a separate PR

sure, I will create a different PR for it.

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

vaibhawvipul · 2024-06-28T05:06:55Z

@kazuyukitanimura ready for review.

codecov-commenter · 2024-06-30T21:14:11Z

Codecov Report

Attention: Patch coverage is 52.08333% with 23 lines in your changes missing coverage. Please review.

Project coverage is 34.08%. Comparing base (2fc45a2) to head (6e843da).
Report is 20 commits behind head on main.

Files	Patch %	Lines
.../scala/org/apache/comet/serde/QueryPlanSerde.scala	52.08%	9 Missing and 14 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #585      +/-   ##
============================================
- Coverage     34.13%   34.08%   -0.06%     
+ Complexity      809      796      -13     
============================================
  Files           106      108       +2     
  Lines         38586    38767     +181     
  Branches       8566     8581      +15     
============================================
+ Hits          13172    13212      +40     
- Misses        22674    22799     +125     
- Partials       2740     2756      +16

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

vaibhawvipul · 2024-07-02T03:14:32Z

@kazuyukitanimura could you please review once? CI passed.

kazuyukitanimura · 2024-07-02T17:01:42Z

@vaibhawvipul Sorry, I got busy with something else. I may be unable to review or work on OSS a week or two.

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

kazuyukitanimura · 2024-07-09T20:22:07Z

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

-            builder.setRight(rightExpr.get)
+          val leftExpr =
+            if (left.dataType == DoubleType || left.dataType == FloatType) {
+              exprToProtoInternal(If(EqualTo(left, negZeroLeft), leftZero, left), inputs)


Hmm exprToProtoInternal recursively gets executed and EqualTo(left, negZeroLeft) will match the above case-match and gets converted at

case (_, `negZeroRight`) => return buildEqualExpr( exprToProtoInternal(left, inputs), exprToProtoInternal(Abs(right).child, inputs))

??

Hmmm still trying to understand how this recursion works...

vaibhawvipul · 2024-07-26T09:29:11Z

@andygrove / @kazuyukitanimura can we please get CI triggered? and also a review?

kazuyukitanimura · 2024-07-26T21:04:09Z

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

+              case (_, `negZeroRight`) =>
+                return buildEqualExpr(
+                  exprToProtoInternal(left, inputs),
+                  exprToProtoInternal(Abs(right).child, inputs))


Do you mind explaining why we need Abs(right).child? I thought Abs(right).child == right

kazuyukitanimura · 2024-07-26T21:04:58Z

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

+                // also return none if one side is nullable and the other is not
+                if ((left.nullable && !right.nullable) &&


What is the reason of the left.nullable && !right.nullable requirement?

kazuyukitanimura · 2024-07-26T21:05:29Z

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

-            builder.setRight(rightExpr.get)
+          val leftExpr =
+            if (left.dataType == DoubleType || left.dataType == FloatType) {
+              exprToProtoInternal(If(EqualTo(left, negZeroLeft), leftZero, left), inputs)


Hmmm still trying to understand how this recursion works...

kazuyukitanimura · 2024-09-23T16:17:49Z

The main issue was fixed by #953
The test cases from this PR would be still helpful

kazuyukitanimura · 2024-09-23T23:35:23Z

Please never mind my comment above, I posted a workaroud PR #960

comphead · 2024-09-24T21:00:16Z

@kazuyukitanimura can we close it?

kazuyukitanimura · 2024-09-24T21:21:04Z

Yes @comphead @vaibhawvipul

vaibhawvipul added 4 commits June 18, 2024 18:06

adding the test case to reproduce the bug

774acef

Remove comments

45b876b

bug fix

90494da

modulo expression wrapper to improve spark's compatibility

d82ffbf

vaibhawvipul marked this pull request as ready for review June 21, 2024 07:41

comphead reviewed Jun 21, 2024

View reviewed changes

comphead marked this pull request as draft June 21, 2024 21:48

changed the logic, modified nullifwhenprimitive function

b658eac

vaibhawvipul marked this pull request as ready for review June 22, 2024 02:44

vaibhawvipul requested a review from comphead June 22, 2024 02:44

kazuyukitanimura reviewed Jun 24, 2024

View reviewed changes

vaibhawvipul added 5 commits June 25, 2024 11:00

adding tests for float and decimal, code refactor

3585f14

adding more tests and code refactor

06febb3

code refactor

447e327

handle neg zero in equalto

20ac2ba

code refactor and adding test case for equalto

00e44ed

vaibhawvipul requested a review from kazuyukitanimura June 27, 2024 16:33

bug fix

cd53284

kazuyukitanimura reviewed Jun 27, 2024

View reviewed changes

vaibhawvipul added 2 commits June 28, 2024 10:07

resolving pr comments

81f4e62

handling left and right zero and negzero separately

9696aaf

vaibhawvipul requested a review from kazuyukitanimura June 28, 2024 05:06

removing unnecessary comments

14f493a

vaibhawvipul added 3 commits June 29, 2024 12:01

bug fixes

2878d71

bug fixes

79b0a4b

bug fix

6e843da

Merge branch 'main' into issue-521

8a95de9

kazuyukitanimura reviewed Jul 9, 2024

View reviewed changes

kazuyukitanimura mentioned this pull request Jul 17, 2024

Divide by zero produces Infinity instead of NULL #665

Closed

vaibhawvipul added 3 commits July 24, 2024 13:15

Merge branch 'main' into issue-521

790e369

resolving merge conflicts

683c57c

adding comments to explain if statements

fd70aa7

vaibhawvipul requested a review from kazuyukitanimura July 24, 2024 11:04

kazuyukitanimura reviewed Jul 26, 2024

View reviewed changes

Merge branch 'main' into issue-521

080130b

kazuyukitanimura mentioned this pull request Sep 20, 2024

fix: Normalize NaN and zeros for floating number comparison #953

Merged

kazuyukitanimura mentioned this pull request Sep 24, 2024

fix: div and rem by negative zero #960

Merged

kazuyukitanimura closed this Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: modulo op with negative zero divisor produces Nan #585

fix: modulo op with negative zero divisor produces Nan #585

vaibhawvipul commented Jun 18, 2024 •

edited

Loading

comphead left a comment

comphead commented Jun 21, 2024

vaibhawvipul commented Jun 21, 2024

kazuyukitanimura commented Jun 21, 2024 •

edited

Loading

vaibhawvipul commented Jun 22, 2024 •

edited

Loading

vaibhawvipul commented Jun 23, 2024

kazuyukitanimura left a comment

vaibhawvipul commented Jun 27, 2024

kazuyukitanimura Jun 27, 2024

vaibhawvipul Jun 28, 2024

vaibhawvipul commented Jun 28, 2024

codecov-commenter commented Jun 30, 2024 •

edited

Loading

vaibhawvipul commented Jul 2, 2024

kazuyukitanimura commented Jul 2, 2024

kazuyukitanimura Jul 9, 2024

vaibhawvipul Jul 24, 2024

kazuyukitanimura Jul 26, 2024

vaibhawvipul commented Jul 26, 2024

kazuyukitanimura Jul 26, 2024

kazuyukitanimura Jul 26, 2024

kazuyukitanimura Jul 26, 2024

kazuyukitanimura commented Sep 23, 2024

kazuyukitanimura commented Sep 23, 2024 •

edited

Loading

comphead commented Sep 24, 2024

kazuyukitanimura commented Sep 24, 2024

		// also return none if one side is nullable and the other is not
		if ((left.nullable && !right.nullable) &&

fix: modulo op with negative zero divisor produces Nan #585

fix: modulo op with negative zero divisor produces Nan #585

Conversation

vaibhawvipul commented Jun 18, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

comphead left a comment

Choose a reason for hiding this comment

comphead commented Jun 21, 2024

vaibhawvipul commented Jun 21, 2024

kazuyukitanimura commented Jun 21, 2024 • edited Loading

vaibhawvipul commented Jun 22, 2024 • edited Loading

vaibhawvipul commented Jun 23, 2024

kazuyukitanimura left a comment

Choose a reason for hiding this comment

vaibhawvipul commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaibhawvipul commented Jun 28, 2024

codecov-commenter commented Jun 30, 2024 • edited Loading

Codecov Report

vaibhawvipul commented Jul 2, 2024

kazuyukitanimura commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaibhawvipul commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazuyukitanimura commented Sep 23, 2024

kazuyukitanimura commented Sep 23, 2024 • edited Loading

comphead commented Sep 24, 2024

kazuyukitanimura commented Sep 24, 2024

vaibhawvipul commented Jun 18, 2024 •

edited

Loading

kazuyukitanimura commented Jun 21, 2024 •

edited

Loading

vaibhawvipul commented Jun 22, 2024 •

edited

Loading

codecov-commenter commented Jun 30, 2024 •

edited

Loading

kazuyukitanimura commented Sep 23, 2024 •

edited

Loading