Route StructArray through the array_slots accessors and drop unmasked_fields() - #9006
Merged
Conversation
robert3005
approved these changes
Jul 27, 2026
joseph-isaacs
force-pushed
the
claude/array-slots-perf-fixes
branch
from
July 29, 2026 10:54
3f1732e to
cad971e
Compare
joseph-isaacs
changed the title
joseph-isaacs
marked this pull request as ready for review
July 29, 2026 10:55
Merging this PR will improve performance by 13.18%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
joseph-isaacs
force-pushed
the
claude/array-slots-perf-fixes
branch
from
July 29, 2026 11:14
cad971e to
f3ee94e
Compare
…elds() Follow-up to #8950 (variadic #[array_slots]) and #9013 (slot-construction fast paths). After those, StructSlots was still only used for its slot-index constants -- the generated view, ext trait and conversions were dead code, and StructArray kept indexing raw slots by hand. Union already subtypes its generated ext trait; Struct now does too, so the macro is the single source of truth for struct slot access. Use the macro: - StructArrayExt extends the generated StructArraySlotsExt supertrait, matching UnionArrayExt, and StructArraySlotsExt is exported for parity with UnionArraySlotsExt. - struct_validity(), iter_unmasked_fields() and unmasked_field() go through the generated validity()/fields() accessors instead of slots()[FIELDS_OFFSET + i] plus hand-written vortex_expect. - into_data_parts(), remove_column() and with_column() -- the last hand-rolled slot-indexing sites in the file -- do the same. with_column() passes its chained iterator straight to try_new_with_dtype rather than collecting a staging Vec. Remove unmasked_fields(): It allocated a Vec and bumped every field's refcount purely to hand the caller something it could index or iterate. The fields already live contiguously in the slots and SlotSlice borrows them for free, so the owned Vec had no remaining justification. Every caller was audited; none needed it: - is_constant, listview conversion, sparse canonicalize and vortex-tui browse only iterate -> iter_unmasked_fields(), no allocation. - struct MaskReduce, struct take, push_validity_into_children, masked struct execute and the vortex-fuzz mask reference impl feed a StructArray constructor -> iter_unmasked_fields().cloned(); the constructors take impl IntoIterator<Item = ArrayRef>, so the single collect happens inside the constructor instead of building an intermediate Vec first. - vx_array_get_field (FFI), chunked tests and the struct cast-rules test want one field or a count -> fields().get(i) / unmasked_field(i) / iter_unmasked_fields().len(). A caller that genuinely needs an owned Vec can still write fields().to_vec(), the same escape hatch UnionArray uses via children().to_vec() -- as runend's rules.rs does, because it mutates the Vec afterwards. Stop re-cloning fields the callers already own: - make_struct_slots takes impl IntoIterator<Item = ArrayRef> and is now the single slot-building path for new_unchecked, try_new_with_dtype and new_fieldless_with_len, mirroring make_union_parts. - Struct deserialization staged its field children in a Vec before moving them into the slots. make_struct_slots needs an infallible iterator, so struct_slots_with_capacity is split out and the VTable pushes fallibly into it: one fewer Vec allocation plus N ArrayRef moves per decoded array. - push_validity_into_children's no-op fast path used try_new, which walked every field, cloned its DType and rebuilt the Arc<StructFieldsInner> plus its memoised name->index map. The fields are unchanged there, so it now uses try_new_with_dtype with a StructFields clone (one refcount bump). - remove_column collected a Filter into ArraySlots; its size_hint lower bound is 0, so smallvec reserved nothing and grew by powers of two. It now builds with an exact capacity. - mask_validity_union and Union's MaskReduce passed children().to_vec() to constructors taking impl IntoIterator; they use iter_children().cloned(), matching the struct branch 13 lines above. Not addressed here: ChunkedArrayExt still hand-rolls raw slot indexing, returns Box<dyn Iterator> from iter_chunks, and exposes the same owned-Vec chunks() accessor removed here from Struct. It has 38 call sites and the generated chunks() name collides with the hand-written one, so it wants its own PR. Verification: - cargo test -p vortex-array -p vortex-sparse -p vortex-layout -p vortex-file (3649 passed, 0 failed) - cargo test --doc -p vortex-array (72 passed) - cargo clippy --all-targets -p vortex-array -p vortex-ffi -p vortex-fuzz -p vortex-tui -p vortex-sparse - cargo check --all-targets over every workspace crate except vortex-duckdb and vortex-sqllogictest, whose DuckDB source download is blocked by the sandbox network policy; neither touches the changed API - cargo +nightly fmt --all, git diff --check Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014xtnKDjfwuVP13p43A1YjC
joseph-isaacs
force-pushed
the
claude/array-slots-perf-fixes
branch
from
July 29, 2026 11:15
f3ee94e to
8274bba
Compare
robert3005
reviewed
Jul 29, 2026
…fail Review feedback: vx_array_get_field should go through a struct-named accessor rather than the generated slots trait. It can't use unmasked_field, which panics -- the function's documented contract is to return NULL and set error_out on an out-of-bounds index, and both the Rust test at vortex-ffi/src/array.rs and vortex-ffi/test/scan.cpp assert that. Routing a bad index from a C caller through vortex_expect would still surface an error (try_or_default catches the unwind) but report it as VX_ERROR_CODE_PANIC with the message "StructArray field slot" instead of "Field index out of bounds". So add the Option-returning variant the trait was missing, matching the unmasked_field_by_name_opt / unmasked_field_by_name pair beside it and UnionArrayExt::child. unmasked_field is now defined in terms of it, and documents that it panics. vortex-ffi goes back to importing StructArrayExt. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014xtnKDjfwuVP13p43A1YjC
joseph-isaacs
force-pushed
the
claude/array-slots-perf-fixes
branch
from
July 29, 2026 12:00
6cf7cc5 to
ce11f58
Compare
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.
Rationale for this change
Follow-up to #8950 (variadic
#[array_slots]) and #9013 (slot-construction fast paths).Doing that exposed
StructArrayExt::unmasked_fields() -> Vec<ArrayRef>, which allocated aVecand bumped every field's refcount purely to hand the caller something it could index or iterate.