Repository navigation
Support variadic tail slots in #[array_slots] macro - #8950
Merged
Merged
Conversation
…-macro-design-pemah1 Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> # Conflicts: # vortex-array/src/arrays/masked/execute.rs # vortex-compressor/src/compressor/cascade.rs
joseph-isaacs
force-pushed
the
claude/array-slots-macro-design-pemah1
branch
from
July 24, 2026 15:40
434c4ac to
5b06b3a
Compare
joseph-isaacs
marked this pull request as ready for review
July 24, 2026 15:40
This was referenced Jul 24, 2026
The merge with develop left `vortex-compressor` importing a `VariantArrayExt` that does not exist, which broke the workspace build and cascaded into nearly every CI job (Python, Java, C++, generated-files, all Rust test matrices). - Import `VariantArraySlotsExt` in the cascade compressor. - Update `vortex-duckdb`'s chunked exporter for the `chunk_offsets` -> `chunk_offset_values` rename; `chunk_offsets` now names the generated slot accessor returning `&ArrayRef`. - Drop the intra-doc link to `::vortex_array::SlotSlice` from the `#[array_slots]` docs. `vortex-array-macros` does not depend on `vortex-array`, so the link failed `RUSTDOCFLAGS="-D warnings"`. - Remove a dead `#[allow(clippy::len_without_is_empty)]` on `SlotSlice::len`, which does define `is_empty`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Merging this PR will improve performance by 10.91%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
The Union and Struct construction helpers routed borrowed children
through `Slots { tail: slice.to_vec() }.into_slots()`, which allocated a
throwaway `Vec`, cloned every child into it, then moved each element
again into the `SmallVec` — an extra allocation and N moves versus the
one-pass build they had before the macro migration.
- make_union_parts / make_struct_slots now clone the borrowed children
straight into an `ArraySlots` sized with `with_capacity`, in a single
pass. `into_slots` is kept only where the caller genuinely owns the
`Vec` (Chunked::new_unchecked), where it moves with zero clones.
- UnionArray::into_data_parts collects children directly into the
`Arc<[ArrayRef]>` via `iter_children().cloned().collect()` instead of
`children().to_vec().into()` (drops an intermediate `Vec`).
- Export ChunkedSlots/ChunkedSlotsView and StructSlots/StructSlotsView so
the generated zero-copy view/accessors are public API (parity with
UnionSlots).
Read paths are unchanged: the generated tail accessor returns a
`SlotSlice` borrow with no allocation. The remaining owned-conversion
`to_vec`/`collect` sites are the minimal clone required at a borrow
boundary for construction, matching pre-migration behavior.
Verification:
- cargo test -p vortex-array --lib -- arrays::union arrays::struct_ arrays::chunked arrays::masked patched::array::tests (192 passed)
- cargo clippy --all-targets -p vortex-array
- cargo check -p vortex-compressor -p vortex-file -p vortex-layout -p vortex-row -p vortex-arrow
- cargo +nightly fmt --all
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017BF3K2eZTEnCsqMsN3wGtc
Contributor
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
Contributor
Benchmarks: PolarSignals Profiling 📖Vortex (geomean): 1.015x ➖ How to read Verdict and Engines
datafusion / vortex-file-compressed (1.015x ➖, 1↑ 1↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-H SF=1 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.023x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.009x ➖, 0↑ 0↓)
datafusion / parquet (1.003x ➖, 1↑ 0↓)
datafusion / arrow (0.994x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed (1.004x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.997x ➖, 1↑ 0↓)
duckdb / parquet (1.002x ➖, 0↑ 1↓)
duckdb / duckdb (1.000x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: FineWeb NVMe 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.094x ➖, 0↑ 4↓)
datafusion / vortex-compact (1.041x ➖, 0↑ 2↓)
datafusion / parquet (1.041x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.048x ➖, 0↑ 1↓)
duckdb / vortex-compact (1.049x ➖, 0↑ 1↓)
duckdb / parquet (1.022x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: Vortex queries 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.944x ➖, 0↑ 0↓)
datafusion / parquet (0.952x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.973x ➖, 0↑ 0↓)
duckdb / parquet (0.974x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-DS SF=1 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.999x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.000x ➖, 1↑ 0↓)
datafusion / parquet (0.987x ➖, 2↑ 0↓)
duckdb / vortex-file-compressed (1.000x ➖, 3↑ 2↓)
duckdb / vortex-compact (0.994x ➖, 3↑ 0↓)
duckdb / parquet (1.003x ➖, 1↑ 0↓)
duckdb / duckdb (1.000x ➖, 1↑ 3↓)
No file size changes detected. |
Contributor
Benchmarks: FineWeb S3 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.825x ➖, 3↑ 0↓)
datafusion / vortex-compact (1.093x ➖, 0↑ 2↓)
datafusion / parquet (0.979x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.040x ➖, 1↑ 1↓)
duckdb / vortex-compact (1.072x ➖, 0↑ 0↓)
duckdb / parquet (1.018x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Statistical and Population Genetics 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
duckdb / vortex-file-compressed (1.070x ➖, 0↑ 2↓)
duckdb / vortex-compact (1.044x ➖, 0↑ 1↓)
duckdb / parquet (1.036x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: Clickbench Sorted on NVME 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.895x ✅, 6↑ 1↓)
datafusion / parquet (0.657x ✅, 9↑ 0↓)
duckdb / vortex-file-compressed (1.136x ❌, 0↑ 7↓)
duckdb / parquet (1.013x ➖, 0↑ 0↓)
duckdb / duckdb (1.020x ➖, 0↑ 0↓)
File Size Changes (201 files changed, -0.0% overall, 87↑ 114↓)
Totals:
|
Contributor
Benchmarks: TPC-H SF=10 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.007x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.001x ➖, 0↑ 0↓)
datafusion / parquet (0.996x ➖, 0↑ 0↓)
datafusion / arrow (0.999x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed (1.003x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.003x ➖, 0↑ 0↓)
duckdb / parquet (1.009x ➖, 0↑ 0↓)
duckdb / duckdb (0.998x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: Random Access 📖Vortex (geomean): 0.940x ➖ How to read Verdict and Engines
unknown / unknown (0.940x ➖, 5↑ 0↓)
|
Contributor
Benchmarks: TPC-H SF=1 on S3 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.990x ➖, 0↑ 0↓)
datafusion / vortex-compact (0.999x ➖, 1↑ 0↓)
datafusion / parquet (1.029x ➖, 1↑ 1↓)
duckdb / vortex-file-compressed (1.068x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.131x ➖, 0↑ 1↓)
duckdb / parquet (1.088x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Clickbench on NVME 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.931x ➖, 8↑ 0↓)
datafusion / parquet (0.612x ✅, 31↑ 0↓)
duckdb / vortex-file-compressed (0.992x ➖, 0↑ 2↓)
duckdb / parquet (0.994x ➖, 0↑ 0↓)
duckdb / duckdb (0.986x ➖, 2↑ 0↓)
File Size Changes (1 files changed, -0.0% overall, 0↑ 1↓)
Totals:
|
Contributor
Benchmarks: Appian on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.942x ➖, 0↑ 0↓)
datafusion / parquet (0.945x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.958x ➖, 0↑ 0↓)
duckdb / parquet (0.968x ➖, 0↑ 0↓)
duckdb / duckdb (0.969x ➖, 0↑ 0↓)
File Size Changes (1 files changed, -0.0% overall, 0↑ 1↓)
Totals:
|
Contributor
Benchmarks: TPC-H SF=10 on S3 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.019x ➖, 0↑ 1↓)
datafusion / vortex-compact (1.051x ➖, 0↑ 0↓)
datafusion / parquet (1.029x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.029x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.032x ➖, 0↑ 0↓)
duckdb / parquet (1.084x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Compression 📖Vortex (geomean): 1.008x ➖ How to read Verdict and Engines
unknown / unknown (1.010x ➖, 0↑ 0↓)
|
robert3005
approved these changes
Jul 27, 2026
This was referenced Jul 27, 2026
joseph-isaacs
added a commit
that referenced
this pull request
Jul 27, 2026
## Rationale for this change Follow-up to #8950, which landed the `#[array_slots]` macro with variadic tail support. The macro derives slot indices from **field declaration order**. That makes the physical storage layout an implicit consequence of how the struct happens to be written: reordering two fields, grouping related fields together, or inserting a new field in the middle silently renumbers the slots. Nothing in the source says which index a field occupies, so the change looks cosmetic in review while actually altering the layout arrays are built from and read back with. Slot indices are a storage contract. They should be stated, not inferred. ## What changes are included in this PR? ### Macro (`vortex-array-macros/src/lib.rs`) Every field of an `#[array_slots]` struct must now carry a `#[slot(N)]` attribute naming the exact index it maps to — `#[slot(N..)]` for a variadic `Vec<ArrayRef>` tail: ```rust #[array_slots(Chunked)] pub struct ChunkedSlots { #[slot(0)] pub chunk_offsets: ArrayRef, #[slot(1..)] pub chunks: Vec<ArrayRef>, } ``` The annotation — not the declaration order — drives the generated constants, `from_slots`, and `into_slots`. `into_slots` emits in annotation order, and `NAMES` / `FIXED_NAMES` are sorted by index, so declaration order becomes purely cosmetic. Because indices are now authoritative, the variadic tail no longer has to be the last *declared* field. `#[slot(..)]` is inert helper syntax consumed during expansion and stripped from the re-emitted struct. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-authored-by: Claude <noreply@anthropic.com>
joseph-isaacs
pushed a commit
that referenced
this pull request
Jul 29, 2026
…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
added a commit
that referenced
this pull request
Jul 29, 2026
…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
added a commit
that referenced
this pull request
Jul 29, 2026
…_fields() (#9006) ## 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 a `Vec` and bumped every field's refcount purely to hand the caller something it could index or iterate. --------- Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
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
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
The
#[array_slots]macro currently supports fixed-size slot layouts with required (ArrayRef) and optional (Option<ArrayRef>) fields. However, several encodings likeChunked,Struct, andUnionhave variable-length slot runs (e.g.,[chunk_offsets, chunks...]) that cannot be cleanly expressed with the current macro.This change extends the macro to support a variadic tail — a final field of type
Vec<ArrayRef>— enabling these encodings to use the macro-generated slot accessors and constants instead of hand-written slot index management.What changes are included in this PR?
Macro enhancements (
vortex-array-macros/src/lib.rs)SlotFieldType::VariadicTailvariant to recognizeVec<ArrayRef>fieldsfixed_specsand optionaltail_specFIXED_COUNT: number of fixed slotsFIXED_NAMES: names of fixed slots{FIELD}_OFFSET: offset where the tail beginsslot_name(idx)function: returns slot names like"chunks[3]"for tail slotsfrom_slots()andinto_slots()to handle tail draining/extendingSlotSlice<'a>instead ofVec<ArrayRef>