Jefffrey commented on code in PR #10835:
URL: https://github.com/apache/arrow-rs/pull/10835#discussion_r3861670193
##########
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.
Review Comment:
```suggestion
// Keep existing offset and only apply new offset to child_data,
// otherwise we double count the offset since indexing to
children
// considers both offset of this StructArray and the offset of
the
// child array
```
i find the `window` terminology confusing, also its incorrect to state it
applies to the null buffer
##########
arrow-array/src/array/struct_array.rs:
##########
@@ -732,6 +735,97 @@ mod tests {
}
}
+ #[test]
+ fn test_struct_array_data_slice() {
+ // Slicing a struct's `ArrayData` and then rebuilding an array from it
+ // must window the children exactly once. Previously the offset was
Review Comment:
im finding this window terminology a little confusing, especially as i dont
think we talk about offsets like this anywhere else?
--
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]
