HaoYang670 commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r973647643
##########
arrow/src/datatypes/native.rs:
##########
@@ -102,6 +103,28 @@ pub(crate) mod native_op {
fn div_wrapping(self, rhs: Self) -> Self {
self / rhs
}
+
+ /// Check `DivideByZero` error and `Overflow` error.
+ fn mod_fully_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
+ Ok(self.mod_wrapping(rhs))
+ }
+ }
+
+ /// Only check `DivideByZero` error
+ fn mod_checked_divide_by_zero(self, rhs: Self) -> Result<Self> {
Review Comment:
The reason I add 3 APIs here is that `mod` and `div` is different from other
ops.
For `add`, `sub`, `mul` ..., we only need to consider the `Overflow` error,
so two APIs (_wrapping, _checked) are enough.
But for `mod` and `div`, there are 2 kinds of errors `Overflow` and
`DivideByZero`, which means we need to add new APIs if we only want to check
one of them. And in the arithmetic kernel, we have 3 different `divide` APIs
(`mod` is similar ):
- `divide`: doesn't check `DivideByZero` or `Overflow` (This uses _wrapping)
- `divide_opt`: only checks the `DivideByZero`. (This is similar to the
`_checked_divided_by_zero` API I add, except that it returns an `Option` but
not `Result)
- `divide_checked`: checks both `DivideByZero` error and `Overflow` error.
(this uses _checked)
An alternative I think is that we could make `mod_checked_divide_by_zero`
return an `Option` to match the behavior of `div_opt` / `mod_opt`
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
