Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use of mul_checked to avoid silent overflow in interval arithmetic #3886

Merged
merged 1 commit into from
Mar 21, 2023
Merged

Use of mul_checked to avoid silent overflow in interval arithmetic #3886

merged 1 commit into from
Mar 21, 2023

Conversation

Weijun-H
Copy link
Member

@Weijun-H Weijun-H commented Mar 18, 2023 •
edited by tustvold
Loading

Which issue does this PR close?

Relates to #3809

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Mar 18, 2023
@alamb alamb changed the title making use of checked multiplication and addition to avoid silent overflow Use of checked multiplication and addition to avoid silent overflow in interval arithmetic Mar 20, 2023
@alamb alamb changed the title Use of checked multiplication and addition to avoid silent overflow in interval arithmetic Use of mul_checked to avoid silent overflow in interval arithmetic Mar 20, 2023
alamb
alamb approved these changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks like a nice improvement to me -- thank you @Weijun-H

I am not sure it closes Use Fixed Point Arithmetic in Interval Parsing #3809 however, which seems to describe a more substantial change (to use fixedpoint arithmetic, like i128, rather than f64, for example)

I think this PR makes the code better than master so 👍 from me

cc @tustvold

tustvold
tustvold approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

Yeah this doesn't quite cover #3809 as it still makes use of floating point arithmetic and therefore still suffers from silent truncation, but as @alamb says this is an improvement over master so 👍

@tustvold tustvold merged commit 6688363 into apache:master Mar 21, 2023
spebern pushed a commit to spebern/arrow-rs that referenced this pull request Mar 25, 2023
@Weijun-H @spebern
making use of checked multiplication and addition to avoid silent ove…
702b8f0 
…rflow (apache#3886)
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

@tustvold tustvold tustvold approved these changes

Assignees
No one assigned
Labels
arrow Changes to the arrow crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Weijun-H @alamb @tustvold