Add overflow-checking variant for primitive arithmetic kernels and explicitly define overflow behavior #2643

viirya · 2022-09-03T05:31:02Z

Which issue does this PR close?

Closes #2641.
Closes #2642.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

…erflow behavior for add

arrow/src/datatypes/native.rs

HaoYang670 · 2022-09-03T12:51:24Z

arrow/src/compute/kernels/arithmetic.rs

+}
+
+/// This is similar to `math_checked_op` but just for divide op.
+fn math_checked_divide<LT, RT, F>(


What is the difference between this function and math_checked_divide_op and why do we need both of them?

Finally I hope we can just have one. Currently math_checked_divide_op is used by divide_dyn and I want to limit the range of change to primitive kernels only.

HaoYang670 · 2022-09-03T13:01:05Z

arrow/src/compute/kernels/arithmetic.rs

@@ -1042,15 +1196,18 @@ pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
 /// Perform `left / right` operation on two arrays without checking for division by zero.
 /// The result of dividing by zero follows normal floating point rules.


We should update the doc

Do you mean "The result of dividing by zero follows normal floating point rules"? I think this is not changed? It will panic as usual.

Do you mean "The result of dividing by zero follows normal floating point rules"?

Yes. But why follows normal floating point rules here ? It seems like the function has supported other numeric types. (T: datatypes::ArrowNumericType)

I think this is not changed? It will panic as usual.

Nope, but float will never panic. Divide by zero in float type gives inf or nan. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=972d301e807f9a6cfd2ba644b763b86c

Maybe it is better to doc the different behaviour between float and other types

for float, dividing by zero follows the normal floating point rules, for other numeric types, dividing be zero will panic, ...

Oh, I see. Yea, let me update the doc.

tustvold · 2022-09-03T13:48:36Z

I'm not a massive fan of forcing users to choose between slow but correct or fast but may have inconsistent behaviour, especially as having parallel kernels increases the likelihood of further divergent behaviour...

Taking a step back I wonder if we could just define the overflow behaviour as wrapping, and use explicit wrapping_op to avoid signed overflow panics in non-release builds. This avoids runtime penalties, is consistent with how Rust handles overflow (unlike C++ signed integer overflow is actually defined, the debug panics are just "helpful"), and is what I at least would expect to occur.

I'm not sure what SQL says on the topic of overflow, if anything, which may be relevant here? Perhaps @alamb knows?

viirya · 2022-09-03T16:49:33Z

I'm not a massive fan of forcing users to choose between slow but correct or fast but may have inconsistent behaviour, especially as having parallel kernels increases the likelihood of further divergent behaviour...

I think that you're talking about divide_checked. Another thought is, I guess the non-simd one should be optimized by the compiler? Not sure how much performance difference between them.

I was thinking if possibly to do same thing on simd_checked_divide_op. But seems simd integers (packed_simd2) don't provide similar wrapping/checked APIs.

Taking a step back I wonder if we could just define the overflow behaviour as wrapping, and use explicit wrapping_op to avoid signed overflow panics in non-release builds. This avoids runtime penalties, is consistent with how Rust handles overflow (unlike C++ signed integer overflow is actually defined, the debug panics are just "helpful"), and is what I at least would expect to occur.

Hmm, is that something we want to have? Actually it may cause more difficulty for us to use this crate. As I mentioned below, we actually need two variants: overflow-checking (currently it could be by setting overflow-checks cargo flag) and overflow-as-null. I don't think defining the overflow behavior as wrapping is good idea. It sounds like a regression from current status. Users cannot choose overflow-checking behavior after that.

I'm not sure what SQL says on the topic of overflow, if anything, which may be relevant here? Perhaps @alamb knows?

This is the next think we want to do. Actually it is more important to us. In Spark, once configured, it is allowed to have overflow. Overflowing value will be represented as NULL.

That's being said, we can skip this change (overflow-checking variant/non-overflow-checking variant) if it cannot reach consensus. I just thought to have overflow/non-overflow variants like C++ is a good idea.

We actually need an overflow-checking variant and an overflow-as-null variant. And the current arithmetic kernels are overflow-checking variant already (if overflow-checks is enabled by users). We just need to add an overflow-as-null variant.

tustvold

Apologies I misread what this PR is doing. The important thing for me is the default kernels do not perform expensive overflow checking.

Having checked variants that return an error makes sense to me

Edit: It occurs to me that we still need to update the dyn kernels and the scalar kernels to be consistent, potentially adding checked variants, but that can easily be done as a follow up

tustvold · 2022-09-03T18:07:10Z

arrow/src/compute/kernels/arithmetic.rs

+                    Ok(Some(r))
+                } else {
+                    // Overflow
+                    Err(ArrowError::ComputeError("Overflow happened".to_string()))


Perhaps we could print the problematic values

tustvold · 2022-09-03T18:08:02Z

arrow/src/compute/kernels/arithmetic.rs

+                        Err(ArrowError::ComputeError("DivideByZero".to_string()))
+                    } else {
+                        // Overflow
+                        Err(ArrowError::ComputeError("Overflow happened".to_string()))


tustvold · 2022-09-03T18:08:20Z

arrow/src/compute/kernels/arithmetic.rs

-    T::Native: Add<Output = T::Native>,
+    T::Native: ArrowNativeTypeOp,
+{
+    math_op(left, right, |a, b| a.wrapping_add_if_applied(b))


tustvold · 2022-09-03T18:12:02Z

arrow/src/datatypes/native.rs

+macro_rules! native_type_op {
+    ($t:tt) => {
+        impl ArrowNativeTypeOp for $t {
+            fn checked_add_if_applied(self, rhs: Self) -> Option<Self> {


Why the if_applied suffix? Is it because wrapping operations are not applicable to floating point types? Perhaps we could call them add and add_checked to match the kernels?

We can just have one API name here as we cannot distinguish what native type we are calling from the kernels. That's why I make such API name. :)

I can rename it to add_checked and add some comments to say for floating point types, it is simply add without check.

tustvold · 2022-09-03T18:12:35Z

arrow/src/datatypes/native.rs

@@ -114,6 +115,98 @@ pub trait ArrowPrimitiveType: 'static {
    }
 }

+/// Trait for ArrowNativeType to provide overflow-aware operations.
+pub trait ArrowNativeTypeOp:


Does this need to be public?

No, I think. I will remove pub in next commit.

Ah, it has to be, as it is used as type bound in public APIs.

You could use a trick of putting the trait in a private module, up to you

I see. Sounds good. Let me update it.

tustvold · 2022-09-03T18:14:41Z

arrow/src/datatypes/native.rs

+pub trait ArrowNativeTypeOp:
+    ArrowNativeType
+    + Add<Output = Self>
+    + Sub<Output = Self>


I find the default impls a bit confusing here, it took me a bit to realise they're only used by the floating point types

Yea, I will add some comments.

tustvold · 2022-09-03T18:19:03Z

arrow/src/compute/kernels/arithmetic.rs

+///
+/// This detects overflow and returns an `Err` for that. For an non-overflow-checking variant,
+/// use `multiply` instead.
+pub fn multiply_check<T>(


Suggested change

pub fn multiply_check<T>(

pub fn multiply_checked<T>(

…pes.

tustvold · 2022-09-04T05:09:03Z

arrow/src/datatypes/native.rs

+/// The APIs with `wrapping` suffix are the variant of non-overflow-checking. If overflow
+/// occurred, they will supposedly wrap around the boundary of the type.
+///
+/// The APIs with `_check` suffix are the variant of overflow-checking which return `None`


tustvold · 2022-09-04T05:13:31Z

arrow/src/compute/kernels/arithmetic.rs

@@ -1013,18 +1171,21 @@ where
 /// Perform `left / right` operation on two arrays. If either left or right value is null
 /// then the result is also null. If any right hand value is zero then the result of this
 /// operation will be `Err(ArrowError::DivideByZero)`.
-pub fn divide<T>(
+///
+/// When `simd` feature is not enabled. This detects overflow and returns an `Err` for that.


What happens when SIMD is enabled?

Got signal: 8, SIGFPE: erroneous arithmetic operation. This is original behavior.

Interesting, that would imply rust division is always checked 🤔

Yup - and LLVM cannot vectorize it correctly - https://rust.godbolt.org/z/T8eTGM8zn

tustvold · 2022-09-04T05:19:01Z

arrow/src/compute/kernels/arithmetic.rs

@@ -1040,17 +1201,21 @@ pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
 }

 /// Perform `left / right` operation on two arrays without checking for division by zero.
-/// The result of dividing by zero follows normal floating point rules.
+/// For floating point types, the result of dividing by zero follows normal floating point
+/// rules. For other numeric types, dividing by zero will panic,


This seems inconsistent with the other APIs, perhaps it should saturate instead

Hmm, saturating_div also panics when dividing by zero.

ursabot · 2022-09-04T09:51:19Z

Benchmark runs are scheduled for baseline = 4c1bb00 and contender = 6d86472. 6d86472 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

viirya · 2022-09-04T20:01:54Z

Thanks for review.

Add overflow-checking variant for add kernel and explicitly define ov…

154f8a5

…erflow behavior for add

github-actions bot added the arrow Changes to the arrow crate label Sep 3, 2022

viirya changed the title ~~Add overflow-checking variant for add kernel and explicitly define overflow behavior for add~~ Add overflow-checking variant for primitive arithmetic kernels and explicitly define overflow behavior Sep 3, 2022

viirya added the api-change Changes to the arrow API label Sep 3, 2022

For subtract, multiply, divide

02a2a80

viirya force-pushed the overflow_arithmetic_add branch from 41b1637 to 02a2a80 Compare September 3, 2022 06:19

viirya added 2 commits September 3, 2022 00:22

Fix tests

74d8cc9

Fix different error message

314886f

viirya force-pushed the overflow_arithmetic_add branch from 9548eff to 314886f Compare September 3, 2022 07:54

HaoYang670 reviewed Sep 3, 2022

View reviewed changes

Fix typo

de42e04

tustvold approved these changes Sep 3, 2022

View reviewed changes

viirya added 4 commits September 3, 2022 17:37

Rename APIs and add more comments. Print values in error message.

e8218c4

Add one more test to distinct divide_by_zero behavior on divide.

06f4ce3

Fix clippy

4aa4432

Update divide doc with dividing by zero behavior for other numeric ty…

3d98aff

…pes.

tustvold reviewed Sep 4, 2022

View reviewed changes

Hide ArrowNativeTypeOp

c930f85

viirya force-pushed the overflow_arithmetic_add branch from bf075f6 to c930f85 Compare September 4, 2022 05:47

Fix a typo

34ea894

tustvold merged commit 6d86472 into apache:master Sep 4, 2022

chunshao90 mentioned this pull request Sep 14, 2022

Occurs attempt to subtract with overflow panic apache/horaedb#253

Closed

tustvold mentioned this pull request Sep 15, 2022

Upgrade to arrow 23.0.0 apache/datafusion#3483

Merged

eejbyfeldt mentioned this pull request Jul 3, 2024

feat: ANSI support for Add apache/datafusion-comet#616

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add overflow-checking variant for primitive arithmetic kernels and explicitly define overflow behavior #2643

Add overflow-checking variant for primitive arithmetic kernels and explicitly define overflow behavior #2643

viirya commented Sep 3, 2022

HaoYang670 Sep 3, 2022

viirya Sep 3, 2022

HaoYang670 Sep 3, 2022

viirya Sep 4, 2022

HaoYang670 Sep 4, 2022 •

edited

Loading

viirya Sep 4, 2022

tustvold commented Sep 3, 2022

viirya commented Sep 3, 2022

tustvold left a comment •

edited

Loading

tustvold Sep 3, 2022

tustvold Sep 3, 2022

tustvold Sep 3, 2022

tustvold Sep 3, 2022

viirya Sep 3, 2022

tustvold Sep 3, 2022

viirya Sep 3, 2022

viirya Sep 4, 2022

tustvold Sep 4, 2022

viirya Sep 4, 2022

tustvold Sep 3, 2022

viirya Sep 3, 2022

tustvold Sep 3, 2022

tustvold Sep 4, 2022

tustvold Sep 4, 2022

viirya Sep 4, 2022

tustvold Sep 4, 2022

tustvold Sep 4, 2022

tustvold Sep 4, 2022

viirya Sep 4, 2022

ursabot commented Sep 4, 2022

viirya commented Sep 4, 2022

		@@ -1042,15 +1196,18 @@ pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
		/// Perform `left / right` operation on two arrays without checking for division by zero.
		/// The result of dividing by zero follows normal floating point rules.

Add overflow-checking variant for primitive arithmetic kernels and explicitly define overflow behavior #2643

Add overflow-checking variant for primitive arithmetic kernels and explicitly define overflow behavior #2643

Conversation

viirya commented Sep 3, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoYang670 Sep 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Sep 3, 2022

viirya commented Sep 3, 2022

tustvold left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ursabot commented Sep 4, 2022

viirya commented Sep 4, 2022

HaoYang670 Sep 4, 2022 •

edited

Loading

tustvold left a comment •

edited

Loading