Refactor struct layout expression partitioning into dedicated module - #9106
Draft
joseph-isaacs wants to merge 10 commits into
Draft
Refactor struct layout expression partitioning into dedicated module#9106joseph-isaacs wants to merge 10 commits into
joseph-isaacs wants to merge 10 commits into
Conversation
A struct layout stores n + 1 children: the struct's own validity bitmap (when nullable) followed by one child per field. Expression partitioning only ever split over the fields, so `StructReader` had to reapply the struct's validity to the partitioned result afterwards. That is unsound: the validity was pushed down into the projected children, so the DType did not round trip, `is_null($)` never consulted the struct's validity, and `select` over a nullable struct lost the struct's own nullability. Partition over all n + 1 children instead. This adds a struct-specific copy of the partitioning logic under `layouts/struct_/partition`, which extends it in three ways: * Expressions are first flattened into a scope holding one field per child of the layout, making the struct's nullability explicit in the expression itself (`$.a` becomes `mask($.a, $.__validity)`). `root`, `get_item`, `select`, `is_null` and `is_not_null` over the root scope each have a rule; `root` is the sound catch-all, so anything else applied to the scope still partitions correctly. * The splitter partitions over slots (validity or field) rather than fields, deduplicating identical sub-expressions and leaving a slot with a single sub-expression unpacked. * Stepping a partition down into its child's scope is now a total rewrite that errors if a partition reaches the struct scope, rather than a best-effort substitution of `$.a` with `$`. The reader no longer post-applies validity: the partitioned root expression places it where the expression's semantics require. Signed-off-by: "Claude" <noreply@anthropic.com>
joseph-isaacs
added
the
action/benchmark
github-actions
Bot
removed
the
action/benchmark
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 (1.036x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.164x ➖, 0↑ 4↓)
datafusion / parquet (1.045x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed (1.006x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.024x ➖, 0↑ 0↓)
duckdb / parquet (1.110x ➖, 0↑ 2↓)
|
Contributor
Benchmarks: Statistical and Population Genetics 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
duckdb / vortex-file-compressed (0.941x ➖, 5↑ 2↓)
duckdb / vortex-compact (1.028x ➖, 1↑ 2↓)
duckdb / parquet (0.997x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Polar Signals Profiling ResultsLatest Run
Previous Runs (15)
Powered by Polar Signals Cloud |
Contributor
Benchmarks: PolarSignals Profiling 📖Vortex (geomean): 0.984x ➖ How to read Verdict and Engines
datafusion / vortex-file-compressed (0.984x ➖, 1↑ 0↓)
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.006x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.009x ➖, 0↑ 0↓)
datafusion / parquet (1.008x ➖, 1↑ 2↓)
duckdb / vortex-file-compressed (1.007x ➖, 1↑ 1↓)
duckdb / vortex-compact (0.995x ➖, 0↑ 0↓)
duckdb / parquet (1.003x ➖, 0↑ 0↓)
duckdb / duckdb (1.003x ➖, 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 (0.971x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.009x ➖, 0↑ 0↓)
datafusion / parquet (0.992x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.986x ➖, 2↑ 1↓)
duckdb / vortex-compact (0.972x ➖, 1↑ 0↓)
duckdb / parquet (0.992x ➖, 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↑ 3↓)
datafusion / vortex-compact (0.992x ➖, 0↑ 2↓)
datafusion / parquet (1.006x ➖, 0↑ 2↓)
duckdb / vortex-file-compressed (0.999x ➖, 3↑ 7↓)
duckdb / vortex-compact (1.007x ➖, 4↑ 7↓)
duckdb / parquet (1.004x ➖, 2↑ 6↓)
duckdb / duckdb (1.003x ➖, 3↑ 1↓)
No file size changes detected. |
Contributor
Benchmarks: Clickbench on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.018x ➖, 1↑ 2↓)
datafusion / parquet (1.006x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed (1.018x ➖, 1↑ 3↓)
duckdb / parquet (0.996x ➖, 0↑ 0↓)
duckdb / duckdb (1.015x ➖, 0↑ 0↓)
File Size Changes (1 files changed, -0.0% overall, 0↑ 1↓)
Totals:
|
joseph-isaacs
added
the
action/benchmark
github-actions
Bot
removed
the
action/benchmark
Contributor
Benchmarks: FineWeb S3 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.031x ➖, 0↑ 0↓)
datafusion / vortex-compact (0.992x ➖, 0↑ 0↓)
datafusion / parquet (0.905x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.997x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.978x ➖, 0↑ 0↓)
duckdb / parquet (0.998x ➖, 0↑ 0↓)
|
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.979x ➖, 0↑ 0↓)
datafusion / vortex-compact (0.948x ➖, 0↑ 0↓)
datafusion / parquet (0.985x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.048x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.080x ➖, 0↑ 1↓)
duckdb / parquet (1.010x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Appian on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.006x ➖, 0↑ 0↓)
datafusion / parquet (1.011x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.996x ➖, 0↑ 0↓)
duckdb / parquet (0.998x ➖, 0↑ 0↓)
duckdb / duckdb (1.002x ➖, 0↑ 0↓)
File Size Changes (1 files changed, -0.0% overall, 0↑ 1↓)
Totals:
|
Contributor
Benchmarks: Clickbench Sorted on NVME 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.054x ➖, 1↑ 3↓)
datafusion / parquet (0.969x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed (0.962x ➖, 3↑ 0↓)
duckdb / parquet (0.992x ➖, 0↑ 0↓)
duckdb / duckdb (0.987x ➖, 0↑ 0↓)
File Size Changes (201 files changed, -0.0% overall, 96↑ 105↓)
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.010x ➖, 1↑ 1↓)
datafusion / vortex-compact (1.006x ➖, 0↑ 0↓)
datafusion / parquet (1.008x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.979x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.987x ➖, 0↑ 0↓)
duckdb / parquet (1.003x ➖, 0↑ 0↓)
duckdb / duckdb (0.977x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: Random Access 📖Vortex (geomean): 0.978x ➖ How to read Verdict and Engines
unknown / unknown (0.994x ➖, 2↑ 1↓)
|
Contributor
Benchmarks: Vortex queries 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.989x ➖, 0↑ 0↓)
datafusion / parquet (1.007x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.972x ➖, 0↑ 0↓)
duckdb / parquet (0.954x ➖, 0↑ 0↓)
No file size changes detected. |
Two changes on top of the struct partitioning fix. Address partition slots by index rather than by name. The flat scope that expressions are flattened into now names each child by its logical slot index — slot 0 is validity, slot `i + 1` is field `i` — which is the same numbering the layout already uses in `slot_to_child`. Field names are arbitrary user data, so any name reserved for validity could be taken by a real field; an index cannot clash, which removes the name-mangling loop that guarded against it. It also lets `StructReader` resolve a slot to its child in `O(1)`, so the name lookup it kept for wide schemas is gone. Move the semantics tests out of the layout crate and up to the file level. They now write a real Vortex file to an in-memory buffer and scan it back, so they cover the whole stack — writer, footer, layouts, and the readers that partition the scan's expressions — rather than expression evaluation alone. The layout-level suite stays where it is. The two are deliberately independent, sharing no fixtures or expectations, so that a change to one side cannot silently move both. Signed-off-by: "Claude" <noreply@anthropic.com>
The other projection tests wrap the nullable struct column in a `pack` or `select`. That wrapper gave the old reader somewhere to hang the struct's validity, so those cases came out right for the wrong reason and passed even before the partitioning fix. Projecting the column on its own has no such wrapper and does fail against the old reader, which returned a non-nullable struct whose fields had absorbed the nulls. Signed-off-by: "Claude" <noreply@anthropic.com>
Merging this PR will improve performance by 23.06%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
Contributor
Benchmarks: Compression 📖Vortex (geomean): 0.990x ➖ How to read Verdict and Engines
unknown / unknown (0.988x ➖, 7↑ 2↓)
|
joseph-isaacs
added
the
action/benchmark
github-actions
Bot
removed
the
action/benchmark
The shared partitioner in vortex-array has six tests, all over a single non-nullable scope. Nothing pins the properties that matter when folding the struct layout's copy back into it: that partitioning preserves an expression's dtype, that each partition type-checks in the scope it is dispatched to, and that partitions are packed by annotation. Add those, plus a characterization test for the nullable scope. That one asserts today's *unsound* behaviour — the struct's own validity is dropped and its nullability reappears on the fields — so that unifying with the struct partitioner has to change it visibly rather than silently. See #1907. Signed-off-by: "Claude" <noreply@anthropic.com>
The row index reader partitions expressions into a row-index part and a child part, but nothing tested that split directly — the only coverage was three end-to-end evaluations, which pass regardless of how the expression is divided up. Add tests for each outcome: an expression that only touches the child, one that only touches the row index (and is stepped down into its own scope by replacing `row_idx()` with `$`), and one spanning both. The spanning case also pins the packed partition shape, since that is what gives partitions a struct dtype and so decides whether `PartitionedExprEval` evaluates them as masks or as arrays. Signed-off-by: "Claude" <noreply@anthropic.com>
The dict reader's pushdown tests only ever split expressions with a single pushed sub-expression, so nothing covered the case the packed partition shape exists for: several sub-expressions under one annotation, read back by index. Nothing pinned the repeated-reference case either. Add both, and give them the same apply-and-compare check the existing multi-stage tests use, so they assert the semantics of the split and not just its syntax. That check is now a helper taking the array under test, since these cases need a struct rather than a string array. Signed-off-by: "Claude" <noreply@anthropic.com>
Partitioning an expression is the same job everywhere it is done: annotate each node with the slots it touches, cut out the maximal subtrees that touch exactly one slot, and leave a root expression that re-assembles them. What differs between callers is only how slots are named, whether the expression needs rewriting into the scope's slots first, and how a finished partition is stepped into the scope that evaluates it. Introduce a `Partitioner` trait for exactly those three, all defaulted to the identity, and rebuild the splitter around it. `partition` and `partition_annotations` keep their signatures and become thin wrappers over a partitioner that names slots by converting the annotation to a field name and does nothing else, so their behaviour is unchanged. Two details worth noting. The splitter now emits partitions in the order slots were first encountered rather than in hash order, so partitioning the same expression twice gives the same plan. And the shared code does not optimize the expression before annotating it: where an expression is simplified changes how finely it splits, so that belongs to `flatten`. This is preparation for folding the struct layout's copy of the partitioner back in; no caller changes behaviour yet. Signed-off-by: "Claude" <noreply@anthropic.com>
The struct layout's partitioner keeps only what is specific to a struct layout — flattening the scope into slot accesses, naming slots by index, and stepping a partition down into its child's scope — and drives vortex-array's splitter through the `Partitioner` trait for the rest. The duplicated splitter, sub-expression naming and root fix-up are gone. Two behaviours that were previously baked into the copy become explicit opt-ins on the trait, so no other caller inherits them: unwrapping a slot's single sub-expression out of its pack, and sharing identical sub-expressions rather than evaluating one per reference. The struct partitioner enables both, which is what keeps projecting a struct asking each child for its own root and reads the validity once per expression. Behaviour is unchanged: the partitioner, layout and file tests all pass untouched. Signed-off-by: "Claude" <noreply@anthropic.com>
The row index reader stepped its row-index partition down by hand, after partitioning, by walking the finished partitions and replacing `row_idx()` with `$`. That is exactly what `Partitioner::step_into` is for, so express it there instead: the reader now declares a partitioner and the shared code applies the step-down as each partition is built. Signed-off-by: "Claude" <noreply@anthropic.com>
It is a `VTable` trait method rather than an inherent one, so the link did not resolve. Signed-off-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
This PR extracts the expression partitioning logic for struct layouts into a dedicated
partitionmodule, improving code organization and maintainability. The originalreader.rsfile contained ~900 lines mixing reader implementation with complex partitioning logic. By separating concerns, we make the partitioning algorithm more testable and easier to understand.The new
StructPartitioneris a specialized implementation that handles partitioning expressions over struct layout children (including the struct's own validity bitmap as a distinct partition slot), which differs from the genericvortex_array::expr::transform::partitionthat only handles field partitioning.What changes are included in this PR?
New
partitionmodule (vortex-layout/src/layouts/struct_/partition/mod.rs):StructPartitioner: Partitions expressions over struct layout childrenStructSlot: Enum representing either the validity bitmap or a fieldStructPartitioned: Result type for single vs. multi-partition expressionsRefactored
readermodule (vortex-layout/src/layouts/struct_/reader/mod.rs):StructPartitionerStructSlotfor uniform handling of validity and field readersReorganized tests:
reader/tests/mod.rs: Test module organizationreader/tests/in_memory.rs: Reference semantics tests (in-memory struct arrays)reader/tests/layout.rs: Layout-based expression evaluation testspartition/tests.rs: Unit tests for the partitioning algorithm itselfWhat APIs are changed? Are there any user-facing changes?
No user-facing API changes. This is an internal refactoring of
vortex-layout. TheStructReaderpublic interface remains unchanged; only its internal implementation is reorganized.The new
StructPartitioneris internal to the layout module and not exposed publicly.https://claude.ai/code/session_01U3JLmrxBcAKz6yTkWYii21