ARROW-9388: [C++] Division kernels #7748

liyafan82 · 2020-07-14T07:14:04Z

See https://issues.apache.org/jira/browse/ARROW-9388

github-actions · 2020-07-14T07:25:03Z

https://issues.apache.org/jira/browse/ARROW-9388

wesm

Thanks for starting this. I'm going to pull this down and make some changes per my comments

cpp/src/arrow/compute/kernels/scalar_arithmetic.cc

cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc

cpp/src/arrow/compute/kernels/scalar_cast_test.cc

wesm · 2020-07-14T15:26:40Z

This can't be merged yet. Divide by zero in the unchecked case causes SIGFPE process crash.

We should probably return null when dividing by zero, this is what Impala does

https://github.com/apache/impala/blob/b5805de3e65fd1c7154e4169b323bb38ddc54f4f/be/src/exprs/operators-ir.cc#L60

I'm curious what other database-type systems do

wesm · 2020-07-14T15:27:24Z

cc @pitrou

pitrou · 2020-07-14T15:28:00Z

This will wait for 2.0.

pitrou · 2020-07-14T15:28:31Z

But, yes, I'll take a look later.

paddyhoran · 2020-07-14T17:17:04Z

We have this implemented with SIMD (which was tricky) on the Rust side. We currently return a Result for division by zero.

cpp/src/arrow/compute/api_scalar.h

cpp/src/arrow/compute/kernels/codegen_internal.h

cpp/src/arrow/compute/kernels/scalar_arithmetic.cc

liyafan82 · 2020-07-15T09:23:59Z

Thanks for starting this. I'm going to pull this down and make some changes per my comments

@wesm Thanks a lot for your effort. Your changes look much more reasonable than mine.

liyafan82 · 2020-07-15T09:29:34Z

This can't be merged yet. Divide by zero in the unchecked case causes SIGFPE process crash.

We should probably return null when dividing by zero, this is what Impala does

https://github.com/apache/impala/blob/b5805de3e65fd1c7154e4169b323bb38ddc54f4f/be/src/exprs/operators-ir.cc#L60

I'm curious what other database-type systems do

I made some initial investigations, and it shows that some systems return null upon divide by zero (e.g. Impala and Hive (https://stackoverflow.com/questions/41165708/hive-dividing-numbers-from-the-same-column)), while others returns an error (like MySQL (https://www.got-it.ai/solutions/sqlquerychat/sql-help/data-manipulation/mysql-divide-by-zero-error-querychat/) and SQL Server (https://stackoverflow.com/questions/23230041/how-to-protect-sql-statement-from-divide-by-zero-error/23230084))

Anyway, we should handle it, instead of letting the process crash.

nealrichardson

Thanks for doing this. Could you please be sure to add this function to https://github.com/apache/arrow/blob/master/docs/source/cpp/compute.rst?

liyafan82 · 2020-07-16T10:50:54Z

Thanks for doing this. Could you please be sure to add this function to https://github.com/apache/arrow/blob/master/docs/source/cpp/compute.rst?

Done. Thanks for your kind reminder.

cpp/src/arrow/compute/kernels/scalar_arithmetic.cc

bkietz · 2020-08-13T13:17:01Z

cpp/src/arrow/compute/kernels/scalar_arithmetic.cc

Suggested change

return left / right;

if (ARROW_PREDICT_FALSE(right == 0)) {

ctx->SetStatus(Status::Invalid("divide by zero"));

return 0;

}

return left / right;

Thanks for your suggestion.
According to the IEEE 754 standard, dividing by zero should be well-defined for floating point numbers. Please see, for example, https://dl.acm.org/doi/10.1145/103162.103163, page 26.

This is true, but division by zero can result in SIGFPE if feenableexcept() or a hardware exception if _control87() are used. For example:

g++ -lm -xc++ - <<! #include <fenv.h> int main(int argc, char* argv[]) { feenableexcept(FE_ALL_EXCEPT); return static_cast<int>(1.0 / 0.0); } ! ./a.out # abort

These are rare, so maybe we don't need to support them (or at least restrict this check to the checked division kernel), but they do represent a potential crash when dividing by 0.0. @pitrou ?

I'd say if the user really wants to enable FP exceptions process-wise, we shouldn't try to counteract them.

Alright, I withdraw this suggestion

cpp/src/arrow/compute/kernels/scalar_arithmetic.cc

cpp/src/arrow/compute/api_scalar.h

pitrou · 2020-08-13T14:30:38Z

docs/source/cpp/compute.rst

Please keep this table in alphabetical order.

Done. Thanks for your kind reminder.

pitrou · 2020-08-13T14:32:08Z

cpp/src/arrow/compute/api_scalar.h

Can you move the zero divisor mention to the body of the docstring (together with "If either argument is null...")?

Done. Thank you for the good suggestion.

pitrou · 2020-08-13T14:32:36Z

cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc

Please don't remove these checks, instead write the tests so that they don't require approximate equality checks.

I have restored the checks.
I still think it makes sense to provide approx equality checks for scalar?

Yes, it does make sense.

pitrou · 2020-08-13T14:34:22Z

cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc

It's not useful to test so many values. We know the CPU handles division properly. Instead, focus on test cases that may trigger specific behaviour:

empty array

nulls

zeros

for floating-point: infinities, perhaps not-a-number

Thank you for the good suggestion.
I have removed some test cases, and made sure the remaining cases cover the above items, with one exception: the inifinity comparisons should depend on another PR: #7826

#7826 was merged now. Also, the tested values are still plethoric IMHO, which makes the tests less readable.

Thanks a lot for your effort. I have added test case for infinity.
In addition, I have removed/simplified more test cases, please check if it looks good to you.

pitrou · 2020-08-13T14:36:22Z

cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc

Should also check what happens when the overflow checks are disabled?

When overflow checks are disabled, we get SIGFP, and the tests crash.

Hmm, we should definitly not crash on overflow. In this case, I think overflow should be detected and a dummy value be output (0 perhaps?).

Sounds reasonable. I have updated the PR accordingly.

liyafan82 · 2020-08-19T09:32:29Z

After more experiments, I find @bkietz is right, as the test case crashed on AMD64 Ubuntu 18.04 when dividing by floating point 0.
So I have accepted his suggestions (Thank you @bkietz ).

nealrichardson · 2020-08-19T14:40:51Z

@pitrou if you're satisfied, could you please merge?

pitrou · 2020-08-20T11:36:12Z

I think unchecked floating point division by zero should simply return an infinite, but we can leave that for a separate JIRA.

liyafan82 · 2020-08-20T12:03:11Z

I think unchecked floating point division by zero should simply return an infinite, but we can leave that for a separate JIRA.

@pitrou Thanks for your feedback, and sorry I did not take care of this problem. I will fix it up in the follow up issue.

As discussed in #7748 (comment), we need to compare scalars approximately in some scenarios. Also: * Fix comparison of same-pointer NaN values * Fix scalar comparison result when both inputs are null (ARROW-8956) * Fix scalar kernel result type when result is null scalar Closes #8438 from liyafan82/fly_1012_scl Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>

liyafan82 force-pushed the fly_0710_div branch 4 times, most recently from 7e9aede to 39f1d7d Compare July 14, 2020 10:17

kszucs requested review from bkietz and wesm July 14, 2020 10:30

wesm reviewed Jul 14, 2020

View reviewed changes

cpp/src/arrow/compute/kernels/scalar_arithmetic.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_cast_test.cc Outdated Show resolved Hide resolved

wesm force-pushed the fly_0710_div branch from 39f1d7d to 016e442 Compare July 14, 2020 15:25

bkietz requested changes Jul 14, 2020

View reviewed changes

liyafan82 force-pushed the fly_0710_div branch 3 times, most recently from 7103e27 to 418e8ac Compare July 15, 2020 12:04

nealrichardson requested changes Jul 15, 2020

View reviewed changes

liyafan82 force-pushed the fly_0710_div branch 3 times, most recently from 3c2c8d7 to dbd6c7e Compare August 13, 2020 02:16

bkietz requested changes Aug 13, 2020

View reviewed changes

pitrou requested changes Aug 13, 2020

View reviewed changes

liyafan82 force-pushed the fly_0710_div branch 3 times, most recently from cc9e3e2 to 0fed34c Compare August 17, 2020 10:12

bkietz approved these changes Aug 18, 2020

View reviewed changes

liyafan82 force-pushed the fly_0710_div branch 2 times, most recently from 15bd314 to 2b7204d Compare August 19, 2020 09:28

ARROW-9388: [C++] Division kernels

88ce81d

liyafan82 force-pushed the fly_0710_div branch from 2b7204d to 88ce81d Compare August 20, 2020 07:12

pitrou closed this in cb7d1c1 Aug 20, 2020

liyafan82 mentioned this pull request Oct 12, 2020

ARROW-10277: [C++] Support comparing scalars approximately #8438

Closed

This was referenced Aug 20, 2020

[C++] Division kernels #25469

Closed

[C++] Support comparing scalars approximately #18325

Closed

-    return left / right;
+    if (ARROW_PREDICT_FALSE(right == 0)) {
+      ctx->SetStatus(Status::Invalid("divide by zero"));
+      return 0;
+    }
+    return left / right;

ARROW-9388: [C++] Division kernels #7748

ARROW-9388: [C++] Division kernels #7748

Uh oh!

Conversation

liyafan82 commented Jul 14, 2020

Uh oh!

github-actions bot commented Jul 14, 2020

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wesm commented Jul 14, 2020

Uh oh!

wesm commented Jul 14, 2020

Uh oh!

pitrou commented Jul 14, 2020

Uh oh!

pitrou commented Jul 14, 2020

Uh oh!

paddyhoran commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liyafan82 commented Jul 15, 2020

Uh oh!

liyafan82 commented Jul 15, 2020

Uh oh!

nealrichardson left a comment

Choose a reason for hiding this comment

Uh oh!

liyafan82 commented Jul 16, 2020

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!

Choose a reason for hiding this comment

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!

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!

paddyhoran commented Jul 14, 2020 •

edited

Loading