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/11/19 19:15:05 UTC
[GitHub] [arrow-rs] viirya opened a new pull request, #3144: Add add_mut and add_checked_mut
viirya opened a new pull request, #3144:
URL: https://github.com/apache/arrow-rs/pull/3144
# 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 #3143.
# 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] doki23 commented on a diff in pull request #3144: Add add_mut and add_checked_mut
Posted by GitBox <gi...@apache.org>.
doki23 commented on code in PR #3144:
URL: https://github.com/apache/arrow-rs/pull/3144#discussion_r1031414972
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -417,4 +584,37 @@ mod tests {
&expected
);
}
+
+ #[test]
+ fn test_binary_mut() {
+ let a = Int32Array::from(vec![15, 14, 9, 8, 1]);
+ let b = Int32Array::from(vec![Some(1), None, Some(3), None, Some(5)]);
+ let c = binary_mut(a, &b, |l, r| l + r).unwrap();
+
+ let expected = Int32Array::from(vec![Some(16), None, Some(12), None, Some(6)]);
+ assert_eq!(c, expected);
+ }
+
Review Comment:
```suggestion
#[test]
fn test_binary_mut() {
let a = Int32Array::from(vec![15, 14, 9, 8, 1]);
let b = Int32Array::from(vec![Some(1), None, Some(3), None, Some(5)]);
let c = binary_mut(a, &b, |l, r| l + r).unwrap();
let expected = Int32Array::from(vec![Some(16), None, Some(12), None, Some(6)]);
assert_eq!(c, expected);
}
```
--
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] doki23 commented on a diff in pull request #3144: Add add_mut and add_checked_mut
Posted by GitBox <gi...@apache.org>.
doki23 commented on code in PR #3144:
URL: https://github.com/apache/arrow-rs/pull/3144#discussion_r1031414972
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -417,4 +584,37 @@ mod tests {
&expected
);
}
+
+ #[test]
+ fn test_binary_mut() {
+ let a = Int32Array::from(vec![15, 14, 9, 8, 1]);
+ let b = Int32Array::from(vec![Some(1), None, Some(3), None, Some(5)]);
+ let c = binary_mut(a, &b, |l, r| l + r).unwrap();
+
+ let expected = Int32Array::from(vec![Some(16), None, Some(12), None, Some(6)]);
+ assert_eq!(c, expected);
+ }
+
Review Comment:
```suggestion
#[test]
fn test_binary_mut_no_null() {
let a = Int32Array::from(vec![15, 14, 9, 8, 1]);
let b = Int32Array::from(vec![1, 0, 3, 0, 5]);
let c = binary_mut(a, &b, |l, r| l + r).unwrap();
let expected = Int32Array::from(vec![16, 14, 12, 8, 6]);
assert_eq!(c, expected);
}
```
--
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] doki23 commented on a diff in pull request #3144: Add add_mut and add_checked_mut
Posted by GitBox <gi...@apache.org>.
doki23 commented on code in PR #3144:
URL: https://github.com/apache/arrow-rs/pull/3144#discussion_r1031395008
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -262,6 +328,79 @@ where
}
}
+/// Applies the provided fallible binary operation across `a` and `b` by mutating the mutable
+/// [`PrimitiveArray`] `a` with the results, returning any error. If any index is null in
+/// either `a` or `b`, the corresponding index in the result will also be null
+///
+/// Like [`try_unary`] the function is only evaluated for non-null indices
+///
+/// Mutable primitive array means that the buffer is not shared with other arrays.
+/// As a result, this mutates the buffer directly without allocating new buffer.
+///
+/// # Error
+///
+/// Return an error if the arrays have different lengths or
+/// the operation is under erroneous.
+/// This function gives error of original [`PrimitiveArray`] `a` if it is not a mutable
+/// primitive array.
+pub fn try_binary_mut<T, F>(
+ a: PrimitiveArray<T>,
+ b: &PrimitiveArray<T>,
+ op: F,
+) -> std::result::Result<
+ PrimitiveArray<T>,
+ std::result::Result<PrimitiveArray<T>, ArrowError>,
+>
+where
+ T: ArrowPrimitiveType,
+ F: Fn(T::Native, T::Native) -> Result<T::Native>,
+{
+ if a.len() != b.len() {
+ return Err(Err(ArrowError::ComputeError(
+ "Cannot perform binary operation on arrays of different length".to_string(),
+ )));
+ }
+ let len = a.len();
+
+ if a.is_empty() {
+ return Ok(PrimitiveArray::from(ArrayData::new_empty(&T::DATA_TYPE)));
+ }
Review Comment:
```suggestion
if a.is_empty() {
return Ok(PrimitiveArray::from(ArrayData::new_empty(&T::DATA_TYPE)));
}
let len = a.len();
```
--
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] doki23 commented on a diff in pull request #3144: Add add_mut and add_checked_mut
Posted by GitBox <gi...@apache.org>.
doki23 commented on code in PR #3144:
URL: https://github.com/apache/arrow-rs/pull/3144#discussion_r1031419881
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -417,4 +584,37 @@ mod tests {
&expected
);
}
+
+ #[test]
+ fn test_binary_mut() {
+ let a = Int32Array::from(vec![15, 14, 9, 8, 1]);
+ let b = Int32Array::from(vec![Some(1), None, Some(3), None, Some(5)]);
+ let c = binary_mut(a, &b, |l, r| l + r).unwrap();
+
+ let expected = Int32Array::from(vec![Some(16), None, Some(12), None, Some(6)]);
+ assert_eq!(c, expected);
+ }
+
+ #[test]
+ fn test_try_binary_mut() {
Review Comment:
We may need add test for no null branch.
--
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 #3144: Add binary_mut and try_binary_mut
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #3144:
URL: https://github.com/apache/arrow-rs/pull/3144#issuecomment-1332847901
Benchmark runs are scheduled for baseline = 989ab8d7a28745e76296a27445bf49921ec2d1cd and contender = 961e114af0bd74d31dfcaa30e91f9929a6e6d719. 961e114af0bd74d31dfcaa30e91f9929a6e6d719 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/b2590cb3a48c4b4fa170e668eaeade61...d7b6c2e9e6224428bbc6520224e980ec/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/ada151a9fb7943fab93d90b50bb13fe5...247b92cae09d41738ff5633a1a33be94/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/3d65d822c1d24e89bd211cee52ced969...d7a5b538463b482b99c7490bc4bfec66/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/88a5515311334edbab318c37df29c4b3...d711d907f80e465c82e7ea64c045874e/)
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
[GitHub] [arrow-rs] tustvold merged pull request #3144: Add binary_mut and try_binary_mut
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #3144:
URL: https://github.com/apache/arrow-rs/pull/3144
--
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 #3144: Add add_mut and add_checked_mut
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3144:
URL: https://github.com/apache/arrow-rs/pull/3144#discussion_r1028183113
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -205,6 +205,72 @@ where
Ok(unsafe { build_primitive_array(len, buffer, null_count, null_buffer) })
}
+/// Given two arrays of length `len`, calls `op(a[i], b[i])` for `i` in `0..len`, mutating
+/// the mutable [`PrimitiveArray`] `a`. If any index is null in either `a` or `b`, the
+/// corresponding index in the result will also be null.
+///
+/// Mutable primitive array means that the buffer is not shared with other arrays.
+/// As a result, this mutates the buffer directly without allocating new buffer.
+///
+/// Like [`unary`] the provided function is evaluated for every index, ignoring validity. This
+/// is beneficial when the cost of the operation is low compared to the cost of branching, and
+/// especially when the operation can be vectorised, however, requires `op` to be infallible
+/// for all possible values of its inputs
+///
+/// # Error
+///
+/// This function gives error if the arrays have different lengths.
+/// This function gives error of original [`PrimitiveArray`] `a` if it is not a mutable
+/// primitive array.
+pub fn binary_mut<T, F>(
+ a: PrimitiveArray<T>,
+ b: &PrimitiveArray<T>,
+ op: F,
+) -> std::result::Result<
Review Comment:
As mentioned on #3134 I think this Result should be the other way round
--
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 #3144: Add add_mut and add_checked_mut
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #3144:
URL: https://github.com/apache/arrow-rs/pull/3144#discussion_r1033858729
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -205,6 +205,75 @@ where
Ok(unsafe { build_primitive_array(len, buffer, null_count, null_buffer) })
}
+/// Given two arrays of length `len`, calls `op(a[i], b[i])` for `i` in `0..len`, mutating
+/// the mutable [`PrimitiveArray`] `a`. If any index is null in either `a` or `b`, the
+/// corresponding index in the result will also be null.
+///
+/// Mutable primitive array means that the buffer is not shared with other arrays.
+/// As a result, this mutates the buffer directly without allocating new buffer.
+///
+/// Like [`unary`] the provided function is evaluated for every index, ignoring validity. This
+/// is beneficial when the cost of the operation is low compared to the cost of branching, and
+/// especially when the operation can be vectorised, however, requires `op` to be infallible
+/// for all possible values of its inputs
+///
+/// # Error
+///
+/// This function gives error if the arrays have different lengths.
+/// This function gives error of original [`PrimitiveArray`] `a` if it is not a mutable
+/// primitive array.
+pub fn binary_mut<T, F>(
Review Comment:
Similarly, I will leave this only and remove `_mut` 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] doki23 commented on a diff in pull request #3144: Add add_mut and add_checked_mut
Posted by GitBox <gi...@apache.org>.
doki23 commented on code in PR #3144:
URL: https://github.com/apache/arrow-rs/pull/3144#discussion_r1031414972
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -417,4 +584,37 @@ mod tests {
&expected
);
}
+
+ #[test]
+ fn test_binary_mut() {
+ let a = Int32Array::from(vec![15, 14, 9, 8, 1]);
+ let b = Int32Array::from(vec![Some(1), None, Some(3), None, Some(5)]);
+ let c = binary_mut(a, &b, |l, r| l + r).unwrap();
+
+ let expected = Int32Array::from(vec![Some(16), None, Some(12), None, Some(6)]);
+ assert_eq!(c, expected);
+ }
+
Review Comment:
```suggestion
#[test]
fn test_binary_mut_no_null() {
let a = Int32Array::from(vec![15, 14, 9, 8, 1]);
let b = Int32Array::from(vec![1, 0, 3, 0, 5]);
let c = binary_mut(a, &b, |l, r| l + r).unwrap();
let expected = Int32Array::from(vec![16, 14, 12, 8, 6]);
assert_eq!(c, expected);
}
```
--
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] doki23 commented on a diff in pull request #3144: Add add_mut and add_checked_mut
Posted by GitBox <gi...@apache.org>.
doki23 commented on code in PR #3144:
URL: https://github.com/apache/arrow-rs/pull/3144#discussion_r1031415586
##########
arrow/src/compute/kernels/arity.rs:
##########
@@ -417,4 +584,37 @@ mod tests {
&expected
);
}
+
+ #[test]
+ fn test_binary_mut() {
+ let a = Int32Array::from(vec![15, 14, 9, 8, 1]);
+ let b = Int32Array::from(vec![Some(1), None, Some(3), None, Some(5)]);
+ let c = binary_mut(a, &b, |l, r| l + r).unwrap();
+
+ let expected = Int32Array::from(vec![Some(16), None, Some(12), None, Some(6)]);
+ assert_eq!(c, expected);
+ }
+
Review Comment:
We may need a test for the non null branch.
--
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