fix(arrow-data): don't double-count offset when slicing struct ArrayData by jaideeppyne · Pull Request #10835 · apache/arrow-rs

jaideeppyne · 2026-08-25T10:02:35Z

Which issue does this PR close?

Closes Panic (offset + length) <= self.len() in make_array on sliced struct in arrow 55 #7750.
Closes Slicing a struct ArrayData with an offset double-counts the offset #7595.

Rationale for this change

Slicing a Struct ArrayData pushed the new offset into the child data and also added it to the parent's own offset. Rebuilding an array from the sliced data (make_array / From<ArrayData> for StructArray) then windowed the already-windowed children a second time and panicked with (offset + length) <= self.len(). This is a regression from 54.3.0 (it panics on 55.0.0 through current main).

This finishes the approach from #7596, which @alamb approved pending additional testing before it went stale.

What changes are included in this PR?

In the Struct arm of ArrayData::slice, keep self.offset unchanged and let the cumulative child offsets carry the slice (a struct's ArrayData has no buffers of its own, now guarded by an assert).

Are these changes tested?

Yes. Added test_struct_array_data_slice (the C data interface offset representation) and test_make_array_sliced_struct_data (the exact #7750 reproducer), both failing before the change and passing after. Verified no regressions across arrow-data, arrow-array, arrow-select, arrow-ord, arrow-cast, and arrow-ipc.

Are there any user-facing changes?

Slicing a struct ArrayData no longer panics on rebuild. The sliced ArrayData now represents the offset on its children with the parent offset unchanged — consistent with how From<ArrayData> for StructArray already interprets it.

Slicing a `Struct` `ArrayData` pushed the new offset into the child data *and* also added it to the parent's own `offset`. Rebuilding an array from the sliced data (e.g. via `make_array` / `From<ArrayData> for StructArray`) then windowed the already-windowed children a second time and panicked with `assertion failed: (offset + length) <= self.len()`. Keep the parent `offset` unchanged and let the cumulative child offsets carry the slice, matching how `From<ArrayData> for StructArray` interprets a struct's offset. Struct `ArrayData` never carries buffers of its own, so the sliced result now holds an empty buffer set (guarded by an assertion). Closes apache#7750. Closes apache#7595.

alamb · 2026-08-25T21:07:18Z

I spent quite a while working on understanding what the current behavior is meant to be and updating the docs here

docs(arrow-data): Document how ArrayData::offset applies to buffers, child_data and nulls #10838

jaideeppyne · 2026-08-26T01:27:27Z

Thanks for digging into this — surfacing the offset/child_data invariants in the public docs is exactly what this area needed (I'd been reasoning from the private field comments too). Once #10838 lands I'll re-check my struct-slice change against it and cite the invariants in the PR. Happy to adjust the approach (currently: keep the parent offset, push the slice into the children) if the documented semantics point elsewhere — or to rebase on #10838 if that's easier to review.

Jefffrey · 2026-08-26T10:05:55Z

+            // 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.


Suggested change

// 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.

// 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

Jefffrey · 2026-08-26T10:10:44Z

+    #[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


im finding this window terminology a little confusing, especially as i dont think we talk about offsets like this anywhere else?

…, child_data and nulls (#10838) # Which issue does this PR close? - Part of #7595. # Rationale for this change The invariants governing `ArrayData::offset` are currently documented only on the **private fields** of `ArrayData`, so they never appear in rendered rustdoc. - offset field https://github.com/apache/arrow-rs/blob/7d9bdfd8a83eb66672826757c324e6fc73d20d53/arrow-data/src/data.rs#L215-L219 - child_data field https://github.com/apache/arrow-rs/blob/7d9bdfd8a83eb66672826757c324e6fc73d20d53/arrow-data/src/data.rs#L235-L244 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::slice` depends on these definitions # What 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.

alamb · 2026-08-26T21:37:43Z

Thanks for digging into this — surfacing the offset/child_data invariants in the public docs is exactly what this area needed (I'd been reasoning from the private field comments too). Once #10838 lands I'll re-check my struct-slice change against it and cite the invariants in the PR. Happy to adjust the approach (currently: keep the parent offset, push the slice into the children) if the documented semantics point elsewhere — or to rebase on #10838 if that's easier to review.

Ok, have merged #10838

It seems as if the C++ implementation and Rust implementation differ in how they deal with sliced struct children, which is perhaps the root cause of this issue.

alamb

It seems like the slice is potentially applied again

arrow-rs/arrow-array/src/array/struct_array.rs

Lines 437 to 439 in 576cc10

    
           if parent_offset != 0 || parent_len != cd.len() { 
        
               make_array(cd.slice(parent_offset, parent_len)) 
        
           } else {

alamb · 2026-08-25T13:05:07Z

+            // 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


I thnk the last sentence here is redundant (as it repeats what the code immediately below it does)

alamb · 2026-08-25T13:19:19Z

    }

+    #[test]
+    fn test_struct_array_data_slice() {


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

alamb · 2026-08-25T13:20:31Z

-                offset: new_offset,
-                buffers: self.buffers.clone(),
+                offset: self.offset,
+                buffers: vec![],


technically self.buffers should always be an empty vec anyways, right? So this change should be a no-op

alamb · 2026-08-25T13:23:47Z

-            // Slice into children
-            let new_offset = self.offset + offset;
+            assert!(
+                self.buffers.is_empty(),


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?

alamb · 2026-08-26T21:38:23Z

(sorry I forgot to post my review yesterday)

…slice-double-offset

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.

jaideeppyne · 2026-08-27T01:38:28Z

You're right, thanks for catching that. My version left self.offset on the parent, so for a struct that already carries an offset (an FFI import, say) From<ArrayData> for StructArray still re-windowed children I'd already windowed — same (offset + length) <= self.len() panic, just moved to a rarer input.

Per the ArrayData::offset docs you added in #10838, the struct offset composes cumulatively with each child's, so the window has to be recorded once. I've pushed self.offset + offset down into the children and reset the parent offset to 0, which makes lines 437-439 a no-op.

I did try the simpler "only bump the parent offset, leave children alone" version first, but the IPC writer serialises struct child_data without applying the parent offset, so that wrote wrong values for sliced maps — hence keeping the window on the children. Added a test for slicing struct data that already has an offset.

github-actions Bot added arrow Changes to the arrow crate arrow-data arrow-array labels Aug 25, 2026

This was referenced Aug 25, 2026

Release arrow-rs / parquet Minor version 59.3.0 (August 2026) #10738

Open

docs(arrow-data): Document how ArrayData::offset applies to buffers, child_data and nulls #10838

Merged

Jefffrey reviewed Aug 26, 2026

View reviewed changes

alamb reviewed Aug 26, 2026

View reviewed changes

jaideeppyne added 2 commits August 27, 2026 07:00

Merge remote-tracking branch 'origin/main' into fix/struct-arraydata-…

b4ac7aa

…slice-double-offset

Navigation Menu

Uh oh!

fix(arrow-data): don't double-count offset when slicing struct ArrayData - #10835

fix(arrow-data): don't double-count offset when slicing struct ArrayData#10835
jaideeppyne wants to merge 3 commits into
apache:mainfrom
jaideeppyne:fix/struct-arraydata-slice-double-offset

jaideeppyne commented Aug 25, 2026

Uh oh!

alamb commented Aug 25, 2026

Uh oh!

jaideeppyne commented Aug 26, 2026

Uh oh!

Jefffrey Aug 26, 2026

Uh oh!

Jefffrey Aug 26, 2026

Uh oh!

alamb commented Aug 26, 2026 •

edited

Loading

Uh oh!

alamb left a comment

Uh oh!

alamb Aug 25, 2026

Uh oh!

alamb Aug 25, 2026

Uh oh!

alamb Aug 25, 2026

Uh oh!

alamb Aug 25, 2026

Uh oh!

alamb commented Aug 26, 2026

Uh oh!

jaideeppyne commented Aug 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if parent_offset != 0 \|\| parent_len != cd.len() {
	make_array(cd.slice(parent_offset, parent_len))
	} else {

Uh oh!

Conversation

jaideeppyne commented Aug 25, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb commented Aug 25, 2026

Uh oh!

jaideeppyne commented Aug 26, 2026

Uh oh!

Jefffrey Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

Jefffrey Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

alamb commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

alamb commented Aug 26, 2026

Uh oh!

jaideeppyne commented Aug 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alamb commented Aug 26, 2026 •

edited

Loading