docs(arrow-data): Document how ArrayData::offset applies to buffers, child_data and nulls - #10838
Merged
Merged
Conversation
alamb
force-pushed
the
alamb/document_array_data_offset
branch
from
August 25, 2026 20:57
562a08f to
80f24bb
Compare
…child_data and nulls
alamb
force-pushed
the
alamb/document_array_data_offset
branch
from
August 25, 2026 21:06
80f24bb to
ffbc3d5
Compare
alamb
commented
Aug 25, 2026
| /// For non-nested types, the offset skips leading elements in the buffers. | ||
| /// Logical element `i` is stored at physical position `offset + i`. | ||
| /// | ||
| /// For example, with `offset = 2` and `len = 3` the following array |
Contributor
Author
There was a problem hiding this comment.
I spent quite a while trying to make comments and ASCII art that illustrate what is going on
| /// logical index 0 1 2 | ||
| /// ``` | ||
| /// | ||
| /// # Offsets for Struct types |
Contributor
Author
There was a problem hiding this comment.
I think the offset rules are different for other types (like Unions) but this PR is already pretty tricky so we can perhaps add more detailed information as a follow on PR
alamb
marked this pull request as ready for review
August 25, 2026 21:06
Jefffrey
approved these changes
Aug 26, 2026
| /// For non-nested types, the offset skips leading elements in the buffers. | ||
| /// Logical element `i` is stored at physical position `offset + i`. | ||
| /// | ||
| /// For example, with `offset = 2` and `len = 3` the following array |
Contributor
Author
|
I am happy to change this documentation if we find other issues with it. However, I am merging it in for now to unblock progress on #10835 |
jaideeppyne
added a commit
to jaideeppyne/arrow-rs
that referenced
this pull request
Aug 27, 2026
The previous revision left `self.offset` on the parent and windowed the children, which still applied the slice twice whenever the input already carried a non-zero offset (as an FFI/C-data-interface import does): `From<ArrayData> for StructArray` re-windows every child by the parent offset, so `slice()` on such data panicked with the same `(offset + length) <= self.len()` assertion. Per the invariants documented in apache#10838, a struct's offset composes with each child's offset, so the window has to be recorded in exactly one place. Push the whole cumulative offset (`self.offset + offset`) into the children and reset the parent's offset to 0. Keeping the window on the children (rather than only on the parent) also preserves what the IPC writer assumes: it serialises struct `child_data` without applying the parent's offset, so a parent-offset-only slice would have written the wrong values and defeated buffer truncation. Add a regression test for slicing struct data that already carries an offset.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
The invariants governing
ArrayData::offsetare currently documented only on the private fields ofArrayData, so they never appear in rendered rustdoc.arrow-rs/arrow-data/src/data.rs
Lines 215 to 219 in 7d9bdfd
arrow-rs/arrow-data/src/data.rs
Lines 235 to 244 in 7d9bdfd
This was making it hard for me to reason about what a correct fix looks like when offset-handling like #7595 / #7750 and #10835, where the correct output of
ArrayData::slicedepends on these definitionsWhat changes are included in this PR?
This PR surfaces those invariants on the public accessors so they are visible in the docs and can be cited as the authority in code and reviews.
Are these changes tested?
Docs only; covered by CI doc builds.
Are there any user-facing changes?
Documentation only — no behavior changes.