optimize the array creation (slots) - #9013
Merged
Merged
Conversation
robert3005
approved these changes
Jul 27, 2026
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
joseph-isaacs
added
changelog/performance
github-actions
Bot
removed
the
action/benchmark-sql-full
joseph-isaacs
added
the
action/benchmark
github-actions
Bot
removed
the
action/benchmark
Contributor
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
Contributor
Benchmarks: PolarSignals Profiling 📖Vortex (geomean): 0.987x ➖ How to read Verdict and Engines
datafusion / vortex-file-compressed (0.987x ➖, 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.012x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.008x ➖, 0↑ 0↓)
datafusion / parquet (0.981x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.014x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.996x ➖, 0↑ 0↓)
duckdb / parquet (0.999x ➖, 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.996x ➖, 0↑ 1↓)
datafusion / vortex-compact (0.998x ➖, 1↑ 1↓)
datafusion / parquet (1.002x ➖, 0↑ 4↓)
duckdb / vortex-file-compressed (0.999x ➖, 1↑ 1↓)
duckdb / vortex-compact (0.995x ➖, 0↑ 2↓)
duckdb / parquet (0.997x ➖, 0↑ 0↓)
duckdb / duckdb (0.987x ➖, 4↑ 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 (0.953x ➖, 0↑ 0↓)
datafusion / vortex-compact (0.948x ➖, 0↑ 0↓)
datafusion / parquet (0.963x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed (0.960x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.954x ➖, 0↑ 0↓)
duckdb / parquet (0.995x ➖, 0↑ 2↓)
duckdb / duckdb (0.984x ➖, 0↑ 0↓)
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.861x ➖, 2↑ 0↓)
datafusion / vortex-compact (1.016x ➖, 0↑ 0↓)
datafusion / parquet (0.917x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.944x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.978x ➖, 0↑ 1↓)
duckdb / parquet (0.999x ➖, 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.017x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.012x ➖, 0↑ 0↓)
duckdb / parquet (1.013x ➖, 0↑ 0↓)
No file size changes detected. |
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.013x ➖, 0↑ 1↓)
datafusion / vortex-compact (1.002x ➖, 0↑ 0↓)
datafusion / parquet (0.999x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.999x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.001x ➖, 0↑ 0↓)
duckdb / parquet (1.003x ➖, 0↑ 0↓)
duckdb / duckdb (1.000x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: Clickbench Sorted on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.973x ➖, 1↑ 1↓)
datafusion / parquet (1.011x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.023x ➖, 0↑ 0↓)
duckdb / parquet (1.008x ➖, 0↑ 0↓)
duckdb / duckdb (1.008x ➖, 0↑ 0↓)
File Size Changes (201 files changed, -0.0% overall, 91↑ 110↓)
Totals:
|
Contributor
Benchmarks: TPC-H SF=1 on S3 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.956x ➖, 1↑ 0↓)
datafusion / vortex-compact (1.013x ➖, 0↑ 0↓)
datafusion / parquet (0.992x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.969x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.976x ➖, 0↑ 0↓)
duckdb / parquet (0.971x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Clickbench on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.039x ➖, 0↑ 2↓)
datafusion / parquet (1.032x ➖, 1↑ 2↓)
duckdb / vortex-file-compressed (1.025x ➖, 1↑ 3↓)
duckdb / parquet (1.013x ➖, 0↑ 1↓)
duckdb / duckdb (1.036x ➖, 1↑ 1↓)
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 (1.003x ➖, 0↑ 0↓)
datafusion / parquet (0.997x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.003x ➖, 0↑ 0↓)
duckdb / parquet (0.999x ➖, 0↑ 0↓)
duckdb / duckdb (0.997x ➖, 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 (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.934x ➖, 0↑ 0↓)
datafusion / vortex-compact (0.963x ➖, 1↑ 0↓)
datafusion / parquet (0.942x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.008x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.007x ➖, 0↑ 0↓)
duckdb / parquet (0.981x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Random Access 📖Vortex (geomean): 0.976x ➖ How to read Verdict and Engines
unknown / unknown (0.986x ➖, 1↑ 0↓)
|
Contributor
Benchmarks: Compression 📖Vortex (geomean): 1.001x ➖ How to read Verdict and Engines
unknown / unknown (1.003x ➖, 0↑ 1↓)
|
Contributor
Benchmarks: Vortex queries 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.017x ➖, 0↑ 0↓)
datafusion / parquet (1.002x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.998x ➖, 0↑ 0↓)
duckdb / parquet (1.000x ➖, 0↑ 0↓)
No file size changes detected. |
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
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
What changes are included in this PR?
What APIs are changed? Are there any user-facing changes?