You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/10/07 10:11:13 UTC
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2841: Simplify ArrowNativeType
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org