You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kazuyukitanimura (via GitHub)" <gi...@apache.org> on 2023/07/14 21:15:04 UTC
[GitHub] [arrow-datafusion] kazuyukitanimura opened a new pull request, #6970: Make previous unchecked division kernel available
kazuyukitanimura opened a new pull request, #6970:
URL: https://github.com/apache/arrow-datafusion/pull/6970
# Which issue does this PR close?
Closes #6967
# Rationale for this change
With #6792, now divisions check dividing by zero.
That is an API change and we are (and some other users may be) looking for an old behavior.
# What changes are included in this PR?
This PR implements `DivideUnchecked` operation that behaves like the previous Division operator (return null for dividing by zero)
# Are these changes tested?
Yes
# Are there any user-facing changes?
No, but users have a choice explicitly to use the previous division behavior.
--
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
[GitHub] [arrow-datafusion] ozankabak commented on pull request #6970: Make previous unchecked division kernel available
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #6970:
URL: https://github.com/apache/arrow-datafusion/pull/6970#issuecomment-1638090716
> That being said it does seem inconsistent to special case integer division by zero and not all the other sources of errors in DF, from casting to arithmetic overflow.
This is true. We will probably have a small family of flags to control these (adding them as we make progress in the project and needs arise) and have an accompanying general (exception-vs-special-value) flag to collectively handle them. Most users will simply deal with that one flag and be happy. Advanced users will be able to set that one flag to establish their baseline and fine tune the behavior according to their own requirements by toggling individual flags. The experience will be in line with compilers' approach to these things so we would have POLS even for advanced users.
--
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
[GitHub] [arrow-datafusion] alamb commented on pull request #6970: Make previous unchecked division kernel available
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6970:
URL: https://github.com/apache/arrow-datafusion/pull/6970#issuecomment-1636887757
> This operator has actually been removed upstream... Perhaps this could be implemented as a nullif zero prior to division? This would be consistent with postgres.
Confirming you mean something like https://docs.rs/datafusion/latest/datafusion/logical_expr/fn.nullif.html so like
```rust
let expr = nullif(expr, lit(0.0f64))
``` ?
What do you think @kazuyukitanimura ?
--
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
[GitHub] [arrow-datafusion] tustvold commented on pull request #6970: Make previous unchecked division kernel available
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6970:
URL: https://github.com/apache/arrow-datafusion/pull/6970#issuecomment-1638035235
I would be fine with making this configurable and use the nullif kernel as suggested.
That being said it does seem inconsistent to special case integer division by zero and not all the other sources of errors in DF, from casting to arithmetic overflow. I wonder if there is a higher level issue if not producing errors is important? FWIW in ANSI mode, which I think is what we should be aiming to replicate, Spark will return errors.
--
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
[GitHub] [arrow-datafusion] alamb commented on pull request #6970: Make previous unchecked division kernel available
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6970:
URL: https://github.com/apache/arrow-datafusion/pull/6970#issuecomment-1640972822
Marking as a draft so I don't accidentally merge this PR
--
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
[GitHub] [arrow-datafusion] ozankabak commented on pull request #6970: Make previous unchecked division kernel available
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #6970:
URL: https://github.com/apache/arrow-datafusion/pull/6970#issuecomment-1637973141
Let's make this behavior (exception vs. NaN vs. null during division) configurable. There are many use cases where one wants the output of division to be nullable, or NaN for floats. PostgreSQL goes the exception route, but many others (e.g. Spark) proceed with execution. Applications involving numerics often prefer the latter and handle such situations downstream. The entire family of streaming use cases is also another example where exception behavior is problematic.
--
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
[GitHub] [arrow-datafusion] tustvold commented on pull request #6970: Make previous unchecked division kernel available
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6970:
URL: https://github.com/apache/arrow-datafusion/pull/6970#issuecomment-1636754605
This operator has actually been removed upstream...
--
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
[GitHub] [arrow-datafusion] kazuyukitanimura commented on pull request #6970: Make previous unchecked division kernel available
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #6970:
URL: https://github.com/apache/arrow-datafusion/pull/6970#issuecomment-1638957485
Thank you all for the reviews. I am currently testing the performance of the `nullif` approach.
--
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
[GitHub] [arrow-datafusion] kazuyukitanimura closed pull request #6970: Make previous unchecked division kernel available
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura closed pull request #6970: Make previous unchecked division kernel available
URL: https://github.com/apache/arrow-datafusion/pull/6970
--
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
[GitHub] [arrow-datafusion] kazuyukitanimura commented on pull request #6970: Make previous unchecked division kernel available
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #6970:
URL: https://github.com/apache/arrow-datafusion/pull/6970#issuecomment-1659109904
It seems no performance degradation for the `nullif` solution. Closing.
Thanks
--
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