jaideeppyne commented on code in PR #10835:
URL: https://github.com/apache/arrow-rs/pull/10835#discussion_r3879241528
##########
arrow-data/src/data.rs:
##########
@@ -696,19 +696,34 @@ impl ArrayData {
assert!(end <= self.len());
if let DataType::Struct(_) = self.data_type() {
- // Slice into children
- let new_offset = self.offset + offset;
+ // As documented on [`Self::offset`], a struct has no buffers of
its
+ // own and its offset composes with each child's offset: logical
+ // element `i` is element `self.offset + i` of every child.
+ //
+ // The slice must therefore be applied exactly once. Previously it
Review Comment:
Done, that block now just says what the code does.
##########
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:
Agreed, dropped it here and everywhere else in the PR. The tests now just
say the offset has to be applied to the children once.
##########
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:
Took the terminology point, "window" is gone from the whole PR now.
I did not apply the suggestion literally, because it describes keeping the
offset on the parent and the code no longer does that. After the earlier round
with @alamb the parent offset is reset to 0 and the cumulative offset goes to
the children, since leaving any of it on the parent is what let
`From<ArrayData> for StructArray` apply it twice. So the comment reads:
```rust
// A struct has no buffers of its own, and reading child element `i`
// combines this array's offset with the child's own offset. Applying
// the slice to both would count it twice, so the cumulative offset
// goes to the children and this array's offset is reset to 0.
```
You were also right that the old wording was wrong about the null buffer.
`nulls` belongs to this array, not the children, so it is sliced by `offset`
alone and the comment says that now.
--
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]
