Skip to content

Scalar arithmetic should return error when overflows.#5811

Merged
alamb merged 4 commits intoapache:mainfrom
zhzy0077:main
Apr 11, 2023
Merged

Scalar arithmetic should return error when overflows.#5811
alamb merged 4 commits intoapache:mainfrom
zhzy0077:main

Conversation

@zhzy0077
Copy link
Copy Markdown
Contributor

@zhzy0077 zhzy0077 commented Mar 31, 2023

Which issue does this PR close?

Closes #5810.

Rationale for this change

Repro in the bug itself. ScalarValue returns an error for many arithmetic errors, but not overflows.

What changes are included in this PR?

Use checked_* and check results.

Are these changes tested?

Tests added.

Are there any user-facing changes?

No.

alamb
alamb approved these changes Mar 31, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @zhzy0077 -- this looks like a great improvement to me

Can you also please add a test that ensures the overflow behavior is consistent with the arrow compute kernels (I can't remember if they error on overflow). Perhaps following the model of:

https://github.com/apache/arrow-datafusion/blob/ab2ad35e821e754c5ba20e92da0f6cacf205b831/datafusion/common/src/scalar.rs#L4896-L4913

comphead
comphead reviewed Apr 1, 2023
View reviewed changes
Comment thread datafusion/common/src/scalar.rs Outdated
}

macro_rules! primitive_checked_op {
($LEFT:expr, $RIGHT:expr, $OPERATION:tt, Float64) => {
Copy link
Copy Markdown
Contributor

@comphead comphead Apr 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool!

@zhzy0077
Copy link
Copy Markdown
Contributor Author

zhzy0077 commented Apr 2, 2023

Thank you @zhzy0077 -- this looks like a great improvement to me

Can you also please add a test that ensures the overflow behavior is consistent with the arrow compute kernels (I can't remember if they error on overflow). Perhaps following the model of:

https://github.com/apache/arrow-datafusion/blob/ab2ad35e821e754c5ba20e92da0f6cacf205b831/datafusion/common/src/scalar.rs#L4896-L4913

Thank you. Added a few tests in scalar_add_overflow_test. Not sure if this covers all your concerns.

@alamb alamb requested a review from tustvold April 3, 2023 13:05
@alamb
Copy link
Copy Markdown
Contributor

alamb commented Apr 3, 2023

@tustvold can you give this one a review (mostly for consistency with the semantics of arrow-rs)?

@tustvold
Copy link
Copy Markdown
Contributor

tustvold commented Apr 3, 2023 •
edited
Loading

mostly for consistency with the semantics of arrow-rs

So this is not consistent with the arrow kernels that DataFusion makes use of, in particular, DataFusion is currently using the unchecked arithmetic kernels, which do not return an error on overflow. This PR will therefore introduce inconsistency between ScalarValue arithmetic, and any arithmetic involving arrays.

The major reason I'm a little apprehensive about changing this is that the checked kernels are at least an order of magnitude slower in the presence of nulls, as LLVM can't vectorise them correctly. This will likely remain the case until SIMD intrinsics are finally stabilised, which will hopefully be sometime this decade...

add(0.1)                time:   [7.4481 µs 7.4539 µs 7.4608 µs]
Found 6 outliers among 100 measurements (6.00%)
  2 (2.00%) high mild
  4 (4.00%) high severe

add_checked(0.1)        time:   [101.62 µs 106.31 µs 111.40 µs]

I definitely think whatever semantics we settle on should be consistent and well documented, but I don't have a strong opinion what it should be. However, I do feel that changing the current semantics warrants some communication due to the major performance regression it would entail.

@zhzy0077
Copy link
Copy Markdown
Contributor Author

zhzy0077 commented Apr 3, 2023

Thank you for the detailed explanation @tustvold. What do you think if we introduce the other set of operands, say "add_checked", "mul_checked", etc.?

@zhzy0077
Copy link
Copy Markdown
Contributor Author

zhzy0077 commented Apr 6, 2023 •
edited
Loading

@alamb @tustvold PR is updated. add/sub methods are no longer touched and add_checked/sub_checked are added. Could you help to take another look?

distance is modified to use *_checked ops.

@waynexia
Copy link
Copy Markdown
Member

waynexia commented Apr 10, 2023

Test error:

---- scalar::tests::scalar_sub_trait_int64_overflow_test stdout ----
thread 'scalar::tests::scalar_sub_trait_int64_overflow_test' panicked at 'attempt to subtract with overflow', datafusion/common/src/scalar.rs:1934:9

It looks like the methods called in unit tests should also be *_checked

@zhzy0077
Copy link
Copy Markdown
Contributor Author

zhzy0077 commented Apr 11, 2023

UT failure fixed.

waynexia
waynexia approved these changes Apr 11, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks

alamb
alamb reviewed Apr 11, 2023
View reviewed changes
Comment thread datafusion/common/src/scalar.rs
}

// Verifies that ScalarValue has the same behavior with compute kernal when it overflows.
fn check_scalar_add_overflow<T>(left: ScalarValue, right: ScalarValue)
Copy link
Copy Markdown
Contributor

@alamb alamb Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this test

alamb
alamb reviewed Apr 11, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @zhzy0077 and @waynexia for the review

@alamb alamb merged commit aed319c into apache:main Apr 11, 2023
korowa pushed a commit to korowa/arrow-datafusion that referenced this pull request Apr 13, 2023
@zhzy0077 @korowa
Scalar arithmetic should return error when overflows. (apache#5811)
ac043c1 
* Scalar arithmetic should return error when overflows.

* Add a few tests.

* add new checked_* ops.

* fix test failure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

@comphead comphead comphead left review comments

@waynexia waynexia waynexia approved these changes

@tustvold tustvold Awaiting requested review from tustvold

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Arithmetic overflow in aggregate_statistics::AggregateStatistics when MAX-MIN overflows

5 participants

@zhzy0077 @alamb @tustvold @waynexia @comphead
⚡