jaideeppyne commented on code in PR #10835:
URL: https://github.com/apache/arrow-rs/pull/10835#discussion_r3879239847
##########
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:
Dropped it. The rewrite after your other comment removed that assert
entirely, so there is no new panic path left.
##########
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:
Right, and it is back to `self.buffers.clone()` now. Setting it to an empty
vec was doing nothing except hiding the assumption.
##########
arrow-array/src/array/struct_array.rs:
##########
@@ -732,6 +735,97 @@ mod tests {
}
}
+ #[test]
+ fn test_struct_array_data_slice() {
Review Comment:
Thanks for running it. Same failure I get on master.
##########
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:
Cut, along with the rest of the history in that block.
--
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]
