Support variadic tail slots in #[array_slots] macro - #8950
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
github-actions
Bot
removed
the
action/benchmark
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>
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
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>