alamb commented on code in PR #10835:
URL: https://github.com/apache/arrow-rs/pull/10835#discussion_r3853232782
##########
arrow-data/src/data.rs:
##########
@@ -637,13 +637,22 @@ impl ArrayData {
assert!(end <= self.len());
if let DataType::Struct(_) = self.data_type() {
- // Slice into children
- let new_offset = self.offset + offset;
+ assert!(
+ self.buffers.is_empty(),
+ "StructArrays should not contain buffers"
+ );
+ // A struct's offset windows its child data (and null buffer), so
+ // the slice is applied by pushing `offset` down into the children
+ // rather than by also adding it to the parent's own offset. Doing
+ // both would double-count the offset (see #7595): the parent
offset
+ // would then window children that have already been windowed. We
Review Comment:
I thnk the last sentence here is redundant (as it repeats what the code
immediately below it does)
##########
arrow-array/src/array/struct_array.rs:
##########
@@ -732,6 +735,97 @@ mod tests {
}
}
+ #[test]
+ fn test_struct_array_data_slice() {
Review Comment:
This test fails like this without the code change
```
andrewlamb@Andrews-MacBook-Pro-3:~/Software/arrow-rs$ cargo test -p
arrow-array --lib test_struct_array_data_slice
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.07s
Running unittests src/lib.rs
(target/debug/deps/arrow_array-03206b6bec814e8b)
running 1 test
test array::struct_array::tests::test_struct_array_data_slice ... FAILED
failures:
---- array::struct_array::tests::test_struct_array_data_slice stdout ----
thread 'array::struct_array::tests::test_struct_array_data_slice' (35050226)
panicked at arrow-data/src/data.rs:637:9:
assertion failed: end <= self.len()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```
##########
arrow-data/src/data.rs:
##########
@@ -637,13 +637,22 @@ impl ArrayData {
assert!(end <= self.len());
if let DataType::Struct(_) = self.data_type() {
- // Slice into children
- let new_offset = self.offset + offset;
+ assert!(
+ self.buffers.is_empty(),
Review Comment:
this will generate a new panic -- so it is now possible that some cases
that used to not panic will start doing so. I don't think that is warranted in
this case -- can we change this to debug_assert?
##########
arrow-data/src/data.rs:
##########
@@ -637,13 +637,22 @@ impl ArrayData {
assert!(end <= self.len());
if let DataType::Struct(_) = self.data_type() {
- // Slice into children
- let new_offset = self.offset + offset;
+ assert!(
+ self.buffers.is_empty(),
+ "StructArrays should not contain buffers"
+ );
+ // A struct's offset windows its child data (and null buffer), so
+ // the slice is applied by pushing `offset` down into the children
+ // rather than by also adding it to the parent's own offset. Doing
+ // both would double-count the offset (see #7595): the parent
offset
+ // would then window children that have already been windowed. We
+ // therefore keep `self.offset` unchanged and let the cumulative
+ // child offsets carry the new slice.
ArrayData {
data_type: self.data_type().clone(),
len: length,
- offset: new_offset,
- buffers: self.buffers.clone(),
+ offset: self.offset,
+ buffers: vec![],
Review Comment:
technically self.buffers should always be an empty vec anyways, right? So
this change should be a no-op
--
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]
