You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2024/06/05 13:43:14 UTC
[PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
andygrove opened a new pull request, #5845:
URL: https://github.com/apache/arrow-rs/pull/5845
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
N/A
# 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.
-->
Downstream crates (such as DataFusion Comet) often need specific handling for overflow errors and we currently have no good way to detect them other than looking for specific words in error messages, which is not very robust. For example, we currently have code like this:
```rust
match self.inner_abs_func.invoke(args) {
Err(DataFusionError::ArrowError(ArrowError::ComputeError(msg), trace)) if msg.contains("overflow") => { ... }
```
It would be better if Arrow had an `ArithmeticOverflow` variant in the `ArrowError` enum, similar to the existing `DivideByZero` variant.
# 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.
-->
Add `ArithmeticOverflow` and update existing overflow handling.
# Are there any user-facing changes?
Yes, overflows will produce a different error now.
<!--
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 add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5845:
URL: https://github.com/apache/arrow-rs/pull/5845#issuecomment-2234180901
I rebased this branch to target the 53.0.0 dev branch but somehow messed up the commit author. @andygrove can you double check that this PR still looks good to you (and/or fix the author?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #5845: Add ArrowError::ArithmeticOverflow
URL: https://github.com/apache/arrow-rs/pull/5845
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5845:
URL: https://github.com/apache/arrow-rs/pull/5845#issuecomment-2150842713
> I would personally selfishly likely go with the latter as it would reduce overall maintainer burden
If you adopt the branching model you propose then yes, I feel fairly confident the former model is what people are more likely to expect/want. If I implement a non-breaking feature addition / performance improvement I would not expect to have to wait 3 months, or pester a maintainer for it to be usable.
A ticket is worthwhile given you appear to feel differently on this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5845:
URL: https://github.com/apache/arrow-rs/pull/5845#issuecomment-2150602730
> Then we can backport any fixes needed to 52.0.0_maintenance and keep development churning along on master
I think I would rather avoid reaching for git-flow style branch machinery until we have either:
* A non-trivial number of pending breaking changes
* Dependent / conflicting breaking changes that need to be applied in a particular order
Otherwise I think we're just creating work for ourselves.
I would prefer we mark this as a draft, and then look to bring it in once either the above criteria occur, or it is time for the breaking release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5845:
URL: https://github.com/apache/arrow-rs/pull/5845#issuecomment-2150525386
This makes sense to me, we should possibly make ArrowError non_exhaustive to avoid adding new variants being a breaking in future.
Unfortunately this has now missed the release and so will have to wait until the next breaking release which will be in 3 months time...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5845:
URL: https://github.com/apache/arrow-rs/pull/5845#issuecomment-2252441728
I merged the 53 dev branch and that seems to have closed this PR -- any chance you can reopen and retarget main?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5845:
URL: https://github.com/apache/arrow-rs/pull/5845#issuecomment-2150622626
> Otherwise I think we're just creating work for ourselves, especially as we'll want to bring everything bar breaking changes onto 52.0.0_maintenance for the patch releases.
" especially as we'll want to bring everything" -- I don't believe we have to do this. We could also consider only backporting things that the contributors want to backport and/or we deem is important (e.g. bug fixes). Otherwise we would just continue to develop on main as we have done in the past
> I would therefore propose we mark this as a draft, and then look to bring it in once either the above criteria occur, or it is time for the breaking release?
This is also a valid approach. One issue I see with this is that we will end up with a thundering herd / PR merge / conflict storm that happens as soon as we start merging the accumulated breaking API changes for the last few months.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5845:
URL: https://github.com/apache/arrow-rs/pull/5845#issuecomment-2234170469
Per discussion on https://github.com/apache/arrow-rs/issues/6050 I retargeted this PR tothe `53.0.0-dev` feature branch, where we are collecting PRs with breaking changes before `master` opens
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5845:
URL: https://github.com/apache/arrow-rs/pull/5845#issuecomment-2150899052
> I feel fairly confident the model that doesn't require additional ceremony for non-breaking changes is what people are more likely to expect/want
I agree. That doesn't me the maintainers are going to do it for them.
> If I implement a non-breaking feature addition / performance improvement I would not expect to have to wait 3 months, or pester a maintainer for it to be usable.
My idea is that there is no pestering required -- the person would simply create the cherry-pick themselves. I will file a ticket tomorrow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5845:
URL: https://github.com/apache/arrow-rs/pull/5845#issuecomment-2174236123
I filed https://github.com/apache/arrow-rs/issues/5907 to track the question on how to do releases and what to do with PRs such as this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5845:
URL: https://github.com/apache/arrow-rs/pull/5845#issuecomment-2150833007
> I had understood we were going to do monthly minor releases, i.e. not just patch releases of back-ported fixes? I presume in your proposed model these would have to come from the maintenance branch, which implies mirroring most commits across
I think the key question is "what do we want in the monthly releases" and thus "what is the criteria to include code there"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add ArrowError::ArithmeticOverflow [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5845:
URL: https://github.com/apache/arrow-rs/pull/5845#issuecomment-2150797641
I had understood we were going to do monthly minor releases, i.e. not just patch releases of back-ported fixes? I presume in your proposed model these would have to come from the maintenance branch, which implies mirroring most commits across
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org