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/06/09 09:46:56 UTC
[GitHub] [arrow-rs] tustvold opened a new pull request, #1825: Replace RawPtrBox with ScalarBuffer (#1811)
tustvold opened a new pull request, #1825:
URL: https://github.com/apache/arrow-rs/pull/1825
Fix value_offsets for empty variable length arrays (#1824)
# Which issue does this PR close?
Closes #1811
Closes #1824
# Rationale for this change
The previous logic was more unsafe, and in fact implementing this PR found a soundness bug in the form of #1824, so already got some ROI :tada:
# What changes are included in this PR?
Removes RawPtrBox and changes to using ScalarBuffer
# Are there any user-facing changes?
Yes, `value_offsets` will now always return an empty slice for zero-length lists
--
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 pull request #1825: Replace RawPtrBox with ScalarBuffer, reduce `unsafe` usage (#1811)
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #1825:
URL: https://github.com/apache/arrow-rs/pull/1825#issuecomment-1152554632
Sadly adding `inline` didn't seem to help. It is probably something trivial but I need some focus time to look into it.
--
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 #1825: Replace RawPtrBox with ScalarBuffer, reduce `unsafe` usage (#1811)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #1825:
URL: https://github.com/apache/arrow-rs/pull/1825#discussion_r1028105824
##########
arrow/src/array/array_primitive.rs:
##########
@@ -100,17 +92,15 @@ impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
/// caller must ensure that the passed in offset is less than the array len()
#[inline]
pub unsafe fn value_unchecked(&self, i: usize) -> T::Native {
- let offset = i + self.offset();
- *self.raw_values.as_ptr().add(offset)
+ *self.raw_values.get_unchecked(i)
Review Comment:
The slice creation is unchecked, so they should compile down to the same thing. Whether they do though...
--
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 #1825: Replace RawPtrBox with ScalarBuffer (#1811)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #1825:
URL: https://github.com/apache/arrow-rs/pull/1825#discussion_r893302923
##########
arrow/src/array/array_binary.rs:
##########
@@ -64,67 +64,40 @@ impl<OffsetSize: OffsetSizeTrait> GenericBinaryArray<OffsetSize> {
/// Returns a clone of the value data buffer
pub fn value_data(&self) -> Buffer {
- self.data.buffers()[1].clone()
+ self.value_data.clone()
}
/// Returns the offset values in the offsets buffer
#[inline]
pub fn value_offsets(&self) -> &[OffsetSize] {
- // Soundness
- // pointer alignment & location is ensured by RawPtrBox
- // buffer bounds/offset is ensured by the ArrayData instance.
- unsafe {
- std::slice::from_raw_parts(
- self.value_offsets.as_ptr().add(self.data.offset()),
- self.len() + 1,
- )
- }
+ &self.value_offsets
}
/// Returns the element at index `i` as bytes slice
/// # Safety
/// Caller is responsible for ensuring that the index is within the bounds of the array
pub unsafe fn value_unchecked(&self, i: usize) -> &[u8] {
- let end = *self.value_offsets().get_unchecked(i + 1);
- let start = *self.value_offsets().get_unchecked(i);
-
- // Soundness
- // pointer alignment & location is ensured by RawPtrBox
- // buffer bounds/offset is ensured by the value_offset invariants
-
// Safety of `to_isize().unwrap()`
// `start` and `end` are &OffsetSize, which is a generic type that implements the
// OffsetSizeTrait. Currently, only i32 and i64 implement OffsetSizeTrait,
// both of which should cleanly cast to isize on an architecture that supports
// 32/64-bit offsets
- std::slice::from_raw_parts(
- self.value_data.as_ptr().offset(start.to_isize().unwrap()),
- (end - start).to_usize().unwrap(),
- )
+ let start = self.value_offsets.get_unchecked(i).to_isize().unwrap();
Review Comment:
I don't like this `to_isize()` dance, really I just want `ArrowNativeType` to provide `AsPrimitive` so that we can do an unchecked cast. Will file a follow up 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] alamb commented on a diff in pull request #1825: Replace RawPtrBox with ScalarBuffer, reduce `unsafe` usage (#1811)
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #1825:
URL: https://github.com/apache/arrow-rs/pull/1825#discussion_r893357988
##########
arrow/src/array/array_binary.rs:
##########
@@ -64,67 +64,40 @@ impl<OffsetSize: OffsetSizeTrait> GenericBinaryArray<OffsetSize> {
/// Returns a clone of the value data buffer
pub fn value_data(&self) -> Buffer {
- self.data.buffers()[1].clone()
+ self.value_data.clone()
}
/// Returns the offset values in the offsets buffer
#[inline]
pub fn value_offsets(&self) -> &[OffsetSize] {
Review Comment:
❤️
##########
arrow/src/array/array_binary.rs:
##########
@@ -39,8 +39,8 @@ use crate::{buffer::MutableBuffer, datatypes::DataType};
/// binary data.
pub struct GenericBinaryArray<OffsetSize: OffsetSizeTrait> {
data: ArrayData,
- value_offsets: RawPtrBox<OffsetSize>,
- value_data: RawPtrBox<u8>,
+ value_offsets: ScalarBuffer<OffsetSize>,
Review Comment:
I wonder if calling `ScalarBuffer` something like `TypedBuffer` would make its purpose slightly clearer 🤔
##########
arrow/src/array/array_string.rs:
##########
@@ -89,30 +81,28 @@ impl<OffsetSize: OffsetSizeTrait> GenericStringArray<OffsetSize> {
/// caller is responsible for ensuring that index is within the array bounds
#[inline]
pub unsafe fn value_unchecked(&self, i: usize) -> &str {
- let end = self.value_offsets().get_unchecked(i + 1);
- let start = self.value_offsets().get_unchecked(i);
-
- // Soundness
- // pointer alignment & location is ensured by RawPtrBox
- // buffer bounds/offset is ensured by the value_offset invariants
- // ISSUE: utf-8 well formedness is not checked
-
// Safety of `to_isize().unwrap()`
// `start` and `end` are &OffsetSize, which is a generic type that implements the
// OffsetSizeTrait. Currently, only i32 and i64 implement OffsetSizeTrait,
// both of which should cleanly cast to isize on an architecture that supports
// 32/64-bit offsets
- let slice = std::slice::from_raw_parts(
- self.value_data.as_ptr().offset(start.to_isize().unwrap()),
- (*end - *start).to_usize().unwrap(),
- );
+ let start = self.value_offsets.get_unchecked(i).to_isize().unwrap();
+ let end = self.value_offsets.get_unchecked(i + 1).to_isize().unwrap();
+
+ // buffer bounds/offset is ensured by the value_offset invariants
Review Comment:
and to be clear, this method is `unsafe` so getting unchecked values seems like it would be ok...
##########
arrow/src/array/array_string.rs:
##########
@@ -573,6 +566,18 @@ mod tests {
.expect("All null array has valid array data");
}
+ #[test]
+ fn test_string_array_empty_offsets() {
Review Comment:
👍
##########
arrow/src/array/raw_pointer.rs:
##########
@@ -1,67 +0,0 @@
-// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
🔥 👍
--
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] jhorstmann commented on pull request #1825: Replace RawPtrBox with ScalarBuffer, reduce `unsafe` usage (#1811)
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on PR #1825:
URL: https://github.com/apache/arrow-rs/pull/1825#issuecomment-1151670264
Looks good, nice improvement. Regarding the benchmarks, maybe the `Deref for ScalarBuffer` needs an explicit `#[inline]` annotation. Everything else in the hot methods, including the `to_isize`, should already get inlined, maybe the compiler decides the method is over some size threshold.
--
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 #1825: Replace RawPtrBox with ScalarBuffer (#1811)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #1825:
URL: https://github.com/apache/arrow-rs/pull/1825#discussion_r893324850
##########
arrow/src/array/array_list.rs:
##########
@@ -940,7 +938,7 @@ mod tests {
}
#[test]
- #[should_panic(expected = "index out of bounds: the len is 10 but the index is 11")]
+ #[should_panic(expected = "index out of bounds: the len is 9 but the index is 10")]
Review Comment:
The index changes because we check the length of the array, not the end offset first
--
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] codecov-commenter commented on pull request #1825: Replace RawPtrBox with ScalarBuffer (#1811)
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1825:
URL: https://github.com/apache/arrow-rs/pull/1825#issuecomment-1150955103
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1825?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1825](https://codecov.io/gh/apache/arrow-rs/pull/1825?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7a40745) into [master](https://codecov.io/gh/apache/arrow-rs/commit/8f1fd128e387fb2ccf8d74ef3e542e4455f1c6b9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8f1fd12) will **decrease** coverage by `0.00%`.
> The diff coverage is `94.11%`.
> :exclamation: Current head 7a40745 differs from pull request most recent head c5ba200. Consider uploading reports for the commit c5ba200 to get more accurate results
```diff
@@ Coverage Diff @@
## master #1825 +/- ##
==========================================
- Coverage 83.45% 83.44% -0.01%
==========================================
Files 200 199 -1
Lines 56697 56685 -12
==========================================
- Hits 47315 47301 -14
- Misses 9382 9384 +2
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1825?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/array/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1825/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L21vZC5ycw==) | `100.00% <ø> (ø)` | |
| [arrow/src/buffer/scalar.rs](https://codecov.io/gh/apache/arrow-rs/pull/1825/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2J1ZmZlci9zY2FsYXIucnM=) | `93.47% <ø> (ø)` | |
| [arrow/src/array/array\_map.rs](https://codecov.io/gh/apache/arrow-rs/pull/1825/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X21hcC5ycw==) | `85.18% <90.90%> (+0.36%)` | :arrow_up: |
| [arrow/src/array/array\_list.rs](https://codecov.io/gh/apache/arrow-rs/pull/1825/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X2xpc3QucnM=) | `96.16% <92.30%> (+<0.01%)` | :arrow_up: |
| [arrow/src/array/array\_binary.rs](https://codecov.io/gh/apache/arrow-rs/pull/1825/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X2JpbmFyeS5ycw==) | `93.13% <93.75%> (-0.18%)` | :arrow_down: |
| [arrow/src/array/array\_string.rs](https://codecov.io/gh/apache/arrow-rs/pull/1825/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X3N0cmluZy5ycw==) | `97.41% <94.73%> (-0.32%)` | :arrow_down: |
| [arrow/src/array/array.rs](https://codecov.io/gh/apache/arrow-rs/pull/1825/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5LnJz) | `89.67% <100.00%> (ø)` | |
| [arrow/src/array/array\_boolean.rs](https://codecov.io/gh/apache/arrow-rs/pull/1825/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X2Jvb2xlYW4ucnM=) | `93.12% <100.00%> (-0.06%)` | :arrow_down: |
| [arrow/src/array/array\_primitive.rs](https://codecov.io/gh/apache/arrow-rs/pull/1825/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X3ByaW1pdGl2ZS5ycw==) | `95.03% <100.00%> (-0.04%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1825/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `93.46% <0.00%> (-0.20%)` | :arrow_down: |
| ... and [1 more](https://codecov.io/gh/apache/arrow-rs/pull/1825/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1825?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1825?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8f1fd12...c5ba200](https://codecov.io/gh/apache/arrow-rs/pull/1825?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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 pull request #1825: Replace RawPtrBox with ScalarBuffer, reduce `unsafe` usage (#1811)
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #1825:
URL: https://github.com/apache/arrow-rs/pull/1825#issuecomment-1151183397
Benchmarks show it making a fairly non-trivial difference to the following kernels:
* like_utf8
* length
* bit_length
* divide
I need to look a bit more into what is going on here
[arrow-benches.txt](https://github.com/apache/arrow-rs/files/8871216/arrow-benches.txt)
--
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 #1825: Replace RawPtrBox with ScalarBuffer, reduce `unsafe` usage (#1811)
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #1825:
URL: https://github.com/apache/arrow-rs/pull/1825#discussion_r1027343300
##########
arrow/src/array/array_primitive.rs:
##########
@@ -100,17 +92,15 @@ impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
/// caller must ensure that the passed in offset is less than the array len()
#[inline]
pub unsafe fn value_unchecked(&self, i: usize) -> T::Native {
- let offset = i + self.offset();
- *self.raw_values.as_ptr().add(offset)
+ *self.raw_values.get_unchecked(i)
Review Comment:
This `value_unchecked` seems more costly than before? Previously it simply takes value from the ptr (with an offset).
But `get_unchecked` on `ScalarBuffer` will create a slice first?
--
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