tustvold commented on code in PR #2841:
URL: https://github.com/apache/arrow-rs/pull/2841#discussion_r989920458
##########
arrow/src/datatypes/native.rs:
##########
@@ -19,110 +19,72 @@ use crate::error::{ArrowError, Result};
pub use arrow_array::ArrowPrimitiveType;
pub use arrow_buffer::{ArrowNativeType, ToByteSlice};
use half::f16;
-use num::Zero;
-use std::ops::{Add, Div, Mul, Rem, Sub};
-mod private {
- pub trait Sealed {}
-}
-
-/// Trait for ArrowNativeType to provide overflow-checking and
non-overflow-checking
-/// variants for arithmetic operations. For floating point types, this
provides some
-/// default implementations. Integer types that need to deal with overflow can
implement
-/// this trait.
+/// Trait for [`ArrowNativeType`] that adds checked and unchecked arithmetic
operations,
+/// and totally ordered comparison operations
///
-/// The APIs with `_wrapping` suffix are the variant of non-overflow-checking.
If overflow
-/// occurred, they will supposedly wrap around the boundary of the type.
+/// The APIs with `_wrapping` suffix do not perform overflow-checking. For
integer
+/// types they will wrap around the boundary of the type. For floating point
types they
+/// will overflow to INF or -INF preserving the expected sign value
///
-/// The APIs with `_checked` suffix are the variant of overflow-checking which
return `None`
-/// if overflow occurred.
-pub trait ArrowNativeTypeOp:
- ArrowNativeType
- + Add<Output = Self>
- + Sub<Output = Self>
- + Mul<Output = Self>
- + Div<Output = Self>
- + Rem<Output = Self>
- + Zero
Review Comment:
I opted to remove this as it avoids committing to using the num traits in
our public API
##########
arrow/src/datatypes/native.rs:
##########
@@ -19,110 +19,72 @@ use crate::error::{ArrowError, Result};
pub use arrow_array::ArrowPrimitiveType;
pub use arrow_buffer::{ArrowNativeType, ToByteSlice};
use half::f16;
-use num::Zero;
-use std::ops::{Add, Div, Mul, Rem, Sub};
-mod private {
- pub trait Sealed {}
-}
-
-/// Trait for ArrowNativeType to provide overflow-checking and
non-overflow-checking
-/// variants for arithmetic operations. For floating point types, this
provides some
-/// default implementations. Integer types that need to deal with overflow can
implement
-/// this trait.
+/// Trait for [`ArrowNativeType`] that adds checked and unchecked arithmetic
operations,
+/// and totally ordered comparison operations
///
-/// The APIs with `_wrapping` suffix are the variant of non-overflow-checking.
If overflow
-/// occurred, they will supposedly wrap around the boundary of the type.
+/// The APIs with `_wrapping` suffix do not perform overflow-checking. For
integer
+/// types they will wrap around the boundary of the type. For floating point
types they
+/// will overflow to INF or -INF preserving the expected sign value
///
-/// The APIs with `_checked` suffix are the variant of overflow-checking which
return `None`
-/// if overflow occurred.
-pub trait ArrowNativeTypeOp:
- ArrowNativeType
- + Add<Output = Self>
- + Sub<Output = Self>
- + Mul<Output = Self>
- + Div<Output = Self>
- + Rem<Output = Self>
Review Comment:
Removing these ensures that kernels are correctly selecting either the
wrapping or checked variants
##########
arrow/src/datatypes/native.rs:
##########
@@ -19,110 +19,72 @@ use crate::error::{ArrowError, Result};
pub use arrow_array::ArrowPrimitiveType;
pub use arrow_buffer::{ArrowNativeType, ToByteSlice};
use half::f16;
-use num::Zero;
-use std::ops::{Add, Div, Mul, Rem, Sub};
-mod private {
- pub trait Sealed {}
-}
-
-/// Trait for ArrowNativeType to provide overflow-checking and
non-overflow-checking
-/// variants for arithmetic operations. For floating point types, this
provides some
-/// default implementations. Integer types that need to deal with overflow can
implement
-/// this trait.
+/// Trait for [`ArrowNativeType`] that adds checked and unchecked arithmetic
operations,
+/// and totally ordered comparison operations
///
-/// The APIs with `_wrapping` suffix are the variant of non-overflow-checking.
If overflow
-/// occurred, they will supposedly wrap around the boundary of the type.
+/// The APIs with `_wrapping` suffix do not perform overflow-checking. For
integer
+/// types they will wrap around the boundary of the type. For floating point
types they
+/// will overflow to INF or -INF preserving the expected sign value
///
-/// The APIs with `_checked` suffix are the variant of overflow-checking which
return `None`
-/// if overflow occurred.
-pub trait ArrowNativeTypeOp:
- ArrowNativeType
- + Add<Output = Self>
- + Sub<Output = Self>
- + Mul<Output = Self>
- + Div<Output = Self>
- + Rem<Output = Self>
- + Zero
- + private::Sealed
-{
- fn add_checked(self, rhs: Self) -> Result<Self> {
- Ok(self + rhs)
- }
-
- fn add_wrapping(self, rhs: Self) -> Self {
- self + rhs
- }
-
- fn sub_checked(self, rhs: Self) -> Result<Self> {
- Ok(self - rhs)
- }
-
- fn sub_wrapping(self, rhs: Self) -> Self {
- self - rhs
- }
-
- fn mul_checked(self, rhs: Self) -> Result<Self> {
- Ok(self * rhs)
- }
-
- fn mul_wrapping(self, rhs: Self) -> Self {
- self * rhs
- }
-
- fn div_checked(self, rhs: Self) -> Result<Self> {
- if rhs.is_zero() {
- Err(ArrowError::DivideByZero)
- } else {
- Ok(self / rhs)
- }
- }
+/// Note `div_wrapping` and `mod_wrapping` will panic for integer types if
`rhs` is zero
+/// although this may be subject to change
<https://github.com/apache/arrow-rs/issues/2647>
+///
+/// The APIs with `_checked` suffix perform overflow-checking. For integer
types
+/// these will return `Err` instead of wrapping. For floating point types they
will
+/// overflow to INF or -INF preserving the expected sign value
+///
+/// Comparison of integer types is as per normal integer comparison rules,
floating
+/// point values are compared as per IEEE 754's totalOrder predicate see
[`f32::total_cmp`]
+///
+pub trait ArrowNativeTypeOp: ArrowNativeType {
+ /// The additive identity
+ const ZERO: Self;
- fn div_wrapping(self, rhs: Self) -> Self {
- self / rhs
- }
+ /// The multiplicative identity
+ const ONE: Self;
- fn mod_checked(self, rhs: Self) -> Result<Self> {
- if rhs.is_zero() {
- Err(ArrowError::DivideByZero)
- } else {
- Ok(self % rhs)
- }
- }
+ fn add_checked(self, rhs: Self) -> Result<Self>;
+
+ fn add_wrapping(self, rhs: Self) -> Self;
+
+ fn sub_checked(self, rhs: Self) -> Result<Self>;
+
+ fn sub_wrapping(self, rhs: Self) -> Self;
- fn mod_wrapping(self, rhs: Self) -> Self {
- self % rhs
Review Comment:
As evidence of the confusion, these are the defaults for the integer types,
whereas the above are the defaults for floating point types :sweat_smile:
##########
arrow/src/datatypes/native.rs:
##########
@@ -19,110 +19,72 @@ use crate::error::{ArrowError, Result};
pub use arrow_array::ArrowPrimitiveType;
pub use arrow_buffer::{ArrowNativeType, ToByteSlice};
use half::f16;
-use num::Zero;
-use std::ops::{Add, Div, Mul, Rem, Sub};
-mod private {
- pub trait Sealed {}
-}
-
-/// Trait for ArrowNativeType to provide overflow-checking and
non-overflow-checking
-/// variants for arithmetic operations. For floating point types, this
provides some
-/// default implementations. Integer types that need to deal with overflow can
implement
-/// this trait.
+/// Trait for [`ArrowNativeType`] that adds checked and unchecked arithmetic
operations,
+/// and totally ordered comparison operations
///
-/// The APIs with `_wrapping` suffix are the variant of non-overflow-checking.
If overflow
-/// occurred, they will supposedly wrap around the boundary of the type.
+/// The APIs with `_wrapping` suffix do not perform overflow-checking. For
integer
+/// types they will wrap around the boundary of the type. For floating point
types they
+/// will overflow to INF or -INF preserving the expected sign value
///
-/// The APIs with `_checked` suffix are the variant of overflow-checking which
return `None`
-/// if overflow occurred.
-pub trait ArrowNativeTypeOp:
- ArrowNativeType
- + Add<Output = Self>
- + Sub<Output = Self>
- + Mul<Output = Self>
- + Div<Output = Self>
- + Rem<Output = Self>
- + Zero
- + private::Sealed
-{
- fn add_checked(self, rhs: Self) -> Result<Self> {
- Ok(self + rhs)
Review Comment:
I perpetually found the default definitions confusing, moving them into the
separate macros makes it clear what applies to what types
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1480,12 +1476,16 @@ pub fn divide_dyn_opt(left: &dyn Array, right: &dyn
Array) -> Result<ArrayRef> {
}
}
-/// Perform `left / right` operation on two arrays without checking for
division by zero.
-/// For floating point types, the result of dividing by zero follows normal
floating point
-/// rules. For other numeric types, dividing by zero will panic,
-/// If either left or right value is null then the result is also null. If any
right hand value is zero then the result of this
+/// Perform `left / right` operation on two arrays without checking for
+/// division by zero or overflow.
+///
+/// For floating point types, overflow and division by zero follows normal
floating point rules
+///
+/// For integer types overflow will wrap around. Division by zero will
currently panic, although
Review Comment:
I plan to address this in a follow up, the current kernel is effectively
useless for integer types
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
