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/09/12 19:44:34 UTC
[GitHub] [arrow-rs] viirya opened a new pull request, #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
viirya opened a new pull request, #2713:
URL: https://github.com/apache/arrow-rs/pull/2713
# 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.
-->
Closes #2712.
# 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.
-->
# 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.
-->
# Are there any user-facing changes?
<!--
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
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#discussion_r969965555
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -162,6 +177,30 @@ where
}
}
+/// Applies a fallible unary function to an array with primitive values.
+pub fn try_unary_dyn<F, T>(array: &dyn Array, op: F) -> Result<ArrayRef>
+where
+ T: ArrowPrimitiveType,
+ F: Fn(T::Native) -> Result<T::Native>,
+{
+ downcast_dictionary_array! {
+ array => try_unary_dict::<_, F, T>(array, op),
Review Comment:
Indeed, there is no type-bound for the dictionary value type. Just do a simple test. At runtime `downcast_ref` will fail in `unary_dict`. I will address it in other 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-rs] sunchao commented on a diff in pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
sunchao commented on code in PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#discussion_r971322647
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -162,6 +177,30 @@ where
}
}
+/// Applies a fallible unary function to an array with primitive values.
+pub fn try_unary_dyn<F, T>(array: &dyn Array, op: F) -> Result<ArrayRef>
+where
+ T: ArrowPrimitiveType,
+ F: Fn(T::Native) -> Result<T::Native>,
+{
+ downcast_dictionary_array! {
+ array => try_unary_dict::<_, F, T>(array, op),
Review Comment:
Follow up sounds fine to me. Perhaps we can just check the type here:
```rust
downcast_dictionary_array! {
array => if array.values().data_type() == &T::DATA_TYPE {
try_unary_dict::<_, F, T>(array, op)
} else {
// throw error
},
t => {
```
--
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-rs] viirya commented on pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#issuecomment-1247412914
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
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#discussion_r970020295
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -834,12 +834,34 @@ where
/// Add every value in an array by a scalar. If any value in the array is null then the
/// result is also null. The given array must be a `PrimitiveArray` of the type same as
/// the scalar, or a `DictionaryArray` of the value type same as the scalar.
+///
+/// This doesn't detect overflow. Once overflowing, the result will wrap around.
+/// For an overflow-checking variant, use `add_scalar_checked_dyn` instead.
pub fn add_scalar_dyn<T>(array: &dyn Array, scalar: T::Native) -> Result<ArrayRef>
where
T: ArrowNumericType,
- T::Native: Add<Output = T::Native>,
+ T::Native: ArrowNativeTypeOp,
{
- unary_dyn::<_, T>(array, |value| value + scalar)
+ unary_dyn::<_, T>(array, |value| value.add_wrapping(scalar))
+}
+
+/// Add every value in an array by a scalar. If any value in the array is null then the
+/// result is also null. The given array must be a `PrimitiveArray` of the type same as
+/// the scalar, or a `DictionaryArray` of the value type same as the scalar.
+///
+/// This detects overflow and returns an `Err` for that. For an non-overflow-checking variant,
+/// use `add_scalar_dyn` instead.
+pub fn add_scalar_checked_dyn<T>(array: &dyn Array, scalar: T::Native) -> Result<ArrayRef>
Review Comment:
Yea, I'm going to add a few lines mentioning that.
--
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-rs] viirya merged pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
viirya merged PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713
--
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-rs] viirya commented on a diff in pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#discussion_r969967099
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -162,6 +177,30 @@ where
}
}
+/// Applies a fallible unary function to an array with primitive values.
+pub fn try_unary_dyn<F, T>(array: &dyn Array, op: F) -> Result<ArrayRef>
+where
+ T: ArrowPrimitiveType,
+ F: Fn(T::Native) -> Result<T::Native>,
+{
+ downcast_dictionary_array! {
+ array => try_unary_dict::<_, F, T>(array, op),
Review Comment:
Normally as the op is provided by users, I suppose that users know dictionary value is same type as the scalar. But it is good to return a meaningful Err instead of runtime panic. I will do it in a follow-up.
--
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-rs] tustvold commented on a diff in pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#discussion_r969852770
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -834,12 +834,34 @@ where
/// Add every value in an array by a scalar. If any value in the array is null then the
/// result is also null. The given array must be a `PrimitiveArray` of the type same as
/// the scalar, or a `DictionaryArray` of the value type same as the scalar.
+///
+/// This doesn't detect overflow. Once overflowing, the result will wrap around.
+/// For an overflow-checking variant, use `add_scalar_checked_dyn` instead.
pub fn add_scalar_dyn<T>(array: &dyn Array, scalar: T::Native) -> Result<ArrayRef>
where
T: ArrowNumericType,
- T::Native: Add<Output = T::Native>,
+ T::Native: ArrowNativeTypeOp,
{
- unary_dyn::<_, T>(array, |value| value + scalar)
+ unary_dyn::<_, T>(array, |value| value.add_wrapping(scalar))
+}
+
+/// Add every value in an array by a scalar. If any value in the array is null then the
+/// result is also null. The given array must be a `PrimitiveArray` of the type same as
+/// the scalar, or a `DictionaryArray` of the value type same as the scalar.
+///
+/// This detects overflow and returns an `Err` for that. For an non-overflow-checking variant,
+/// use `add_scalar_dyn` instead.
+pub fn add_scalar_checked_dyn<T>(array: &dyn Array, scalar: T::Native) -> Result<ArrayRef>
Review Comment:
If it is anything like the non-scalar kernels, it is about 10x slower. Aside from the branching costs, it prevents LLVM from vectorising it correctly
--
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-rs] viirya commented on a diff in pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#discussion_r971363755
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -162,6 +177,30 @@ where
}
}
+/// Applies a fallible unary function to an array with primitive values.
+pub fn try_unary_dyn<F, T>(array: &dyn Array, op: F) -> Result<ArrayRef>
+where
+ T: ArrowPrimitiveType,
+ F: Fn(T::Native) -> Result<T::Native>,
+{
+ downcast_dictionary_array! {
+ array => try_unary_dict::<_, F, T>(array, op),
Review Comment:
Oh, right, actually I thought to handle it at `try_unary_dict`. But this fix looks okay as `try_unary_dict` is currently used here and not public. I may fix at `try_unary_dict` at another followup.
--
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-rs] viirya commented on pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#issuecomment-1244579644
cc @sunchao
--
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-rs] alamb commented on pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#issuecomment-1249378756
Label added automatically
--
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-rs] sunchao commented on a diff in pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
sunchao commented on code in PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#discussion_r969838371
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -834,12 +834,34 @@ where
/// Add every value in an array by a scalar. If any value in the array is null then the
/// result is also null. The given array must be a `PrimitiveArray` of the type same as
/// the scalar, or a `DictionaryArray` of the value type same as the scalar.
+///
+/// This doesn't detect overflow. Once overflowing, the result will wrap around.
+/// For an overflow-checking variant, use `add_scalar_checked_dyn` instead.
Review Comment:
perhaps explain a bit when it will return `Err`
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -834,12 +834,34 @@ where
/// Add every value in an array by a scalar. If any value in the array is null then the
/// result is also null. The given array must be a `PrimitiveArray` of the type same as
/// the scalar, or a `DictionaryArray` of the value type same as the scalar.
+///
+/// This doesn't detect overflow. Once overflowing, the result will wrap around.
+/// For an overflow-checking variant, use `add_scalar_checked_dyn` instead.
pub fn add_scalar_dyn<T>(array: &dyn Array, scalar: T::Native) -> Result<ArrayRef>
where
T: ArrowNumericType,
- T::Native: Add<Output = T::Native>,
+ T::Native: ArrowNativeTypeOp,
{
- unary_dyn::<_, T>(array, |value| value + scalar)
+ unary_dyn::<_, T>(array, |value| value.add_wrapping(scalar))
+}
+
+/// Add every value in an array by a scalar. If any value in the array is null then the
+/// result is also null. The given array must be a `PrimitiveArray` of the type same as
+/// the scalar, or a `DictionaryArray` of the value type same as the scalar.
+///
+/// This detects overflow and returns an `Err` for that. For an non-overflow-checking variant,
+/// use `add_scalar_dyn` instead.
+pub fn add_scalar_checked_dyn<T>(array: &dyn Array, scalar: T::Native) -> Result<ArrayRef>
Review Comment:
curious: do we have benchmark to track how much slower `add_scalar_checked_dyn` is comparing to `add_scalar_dyn`?
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -162,6 +177,30 @@ where
}
}
+/// Applies a fallible unary function to an array with primitive values.
+pub fn try_unary_dyn<F, T>(array: &dyn Array, op: F) -> Result<ArrayRef>
+where
+ T: ArrowPrimitiveType,
+ F: Fn(T::Native) -> Result<T::Native>,
+{
+ downcast_dictionary_array! {
+ array => try_unary_dict::<_, F, T>(array, op),
Review Comment:
hmm how do we know the dictionary value type matches `T`?
--
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-rs] sunchao commented on a diff in pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
sunchao commented on code in PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#discussion_r969859994
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -834,12 +834,34 @@ where
/// Add every value in an array by a scalar. If any value in the array is null then the
/// result is also null. The given array must be a `PrimitiveArray` of the type same as
/// the scalar, or a `DictionaryArray` of the value type same as the scalar.
+///
+/// This doesn't detect overflow. Once overflowing, the result will wrap around.
+/// For an overflow-checking variant, use `add_scalar_checked_dyn` instead.
pub fn add_scalar_dyn<T>(array: &dyn Array, scalar: T::Native) -> Result<ArrayRef>
where
T: ArrowNumericType,
- T::Native: Add<Output = T::Native>,
+ T::Native: ArrowNativeTypeOp,
{
- unary_dyn::<_, T>(array, |value| value + scalar)
+ unary_dyn::<_, T>(array, |value| value.add_wrapping(scalar))
+}
+
+/// Add every value in an array by a scalar. If any value in the array is null then the
+/// result is also null. The given array must be a `PrimitiveArray` of the type same as
+/// the scalar, or a `DictionaryArray` of the value type same as the scalar.
+///
+/// This detects overflow and returns an `Err` for that. For an non-overflow-checking variant,
+/// use `add_scalar_dyn` instead.
+pub fn add_scalar_checked_dyn<T>(array: &dyn Array, scalar: T::Native) -> Result<ArrayRef>
Review Comment:
I see. I wonder if we should point that out in the doc of this method, in case it's not obvious to the 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-rs] viirya commented on a diff in pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#discussion_r969908592
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -834,12 +834,34 @@ where
/// Add every value in an array by a scalar. If any value in the array is null then the
/// result is also null. The given array must be a `PrimitiveArray` of the type same as
/// the scalar, or a `DictionaryArray` of the value type same as the scalar.
+///
+/// This doesn't detect overflow. Once overflowing, the result will wrap around.
+/// For an overflow-checking variant, use `add_scalar_checked_dyn` instead.
pub fn add_scalar_dyn<T>(array: &dyn Array, scalar: T::Native) -> Result<ArrayRef>
where
T: ArrowNumericType,
- T::Native: Add<Output = T::Native>,
+ T::Native: ArrowNativeTypeOp,
{
- unary_dyn::<_, T>(array, |value| value + scalar)
+ unary_dyn::<_, T>(array, |value| value.add_wrapping(scalar))
+}
+
+/// Add every value in an array by a scalar. If any value in the array is null then the
+/// result is also null. The given array must be a `PrimitiveArray` of the type same as
+/// the scalar, or a `DictionaryArray` of the value type same as the scalar.
+///
+/// This detects overflow and returns an `Err` for that. For an non-overflow-checking variant,
+/// use `add_scalar_dyn` instead.
+pub fn add_scalar_checked_dyn<T>(array: &dyn Array, scalar: T::Native) -> Result<ArrayRef>
Review Comment:
Yea, it will be much slower. As by default (ansi-mode disabled) in our case, non-checked kernels will be used. So most of time users will use faster one, except they have special need to use checked kernels.
--
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-rs] ursabot commented on pull request #2713: Add overflow-checking variants of arithmetic scalar dyn kernels
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #2713:
URL: https://github.com/apache/arrow-rs/pull/2713#issuecomment-1247417574
Benchmark runs are scheduled for baseline = 2a0fc7703420f99d28141516cabdd0408a583dfc and contender = 7594db6367515473efdb130e7de91060079a4d88. 7594db6367515473efdb130e7de91060079a4d88 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/ef4691b8a8d74fd7887f0f6c38329856...238a84628e20441296175d42553ba2dd/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/8f27f18629de4d7e8c8fc10346dcd434...5de35a220f164cd4bb1dea3d9ac61723/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/3af3cf0574934b3f89980123c472de4f...b4046941248c4ce38616be3042b59dbd/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/4c7ff86a1b8a410ca6a535bfb38c34fd...c648b821c6784f88a686e288000578d4/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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