Add fallible versions of temporal functions that may panic #7737
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
adriangb
commented
Jun 22, 2025
arrow-arith/src/numeric.rs
Outdated
Contributor
Author
There was a problem hiding this comment.
I felt that it was better to just break this trait than introduce all of the new methods. It should be pretty easy for users to update the trait implementation itself, and it's always possible for them to just .expect(...) in their code to at least acknowledge the fallibility
Contributor
There was a problem hiding this comment.
I double checked that this is not a public trait and thus this change is not a breaking API change
adriangb
changed the title
Contributor
|
🤖 |
alamb
reviewed
Jun 23, 2025
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @adriangb -- this looks good to me -- I have kicked off some performance tests just to verify this doesn't change anything
I do think we should try and add some tests to this ideally showing that we can now catch overflows as errors rather than panics. Would you have time to do that?
arrow-arith/src/numeric.rs
Outdated
Contributor
There was a problem hiding this comment.
I double checked that this is not a public trait and thus this change is not a breaking API change
Contributor
|
Additional context is we hit this panic in DataFusion (and found a workaround) |
Contributor
Author
Yes I think I should have time in the next couple days! |
Contributor
|
🤖: Benchmark completed Details
|
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
…panic due to overflows
adriangb
force-pushed
the
fix-possible-panics
branch
from
3659a9e to
897fec6
Compare
Contributor
Author
|
@alamb I've added quite a bit of tests |
Contributor
Author
|
@alamb I don't have commit rights here, could you merge this if you think it's ready? thanks! |
alamb
pushed a commit
that referenced
this pull request
Jan 17, 2026
# Which issue does this PR close? This PR does the same thing as what was done in #7737 to handle overflow errors gracely instead of panicking. - Part of #4456. # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> # What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> # Are these changes tested? yes # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.