feat(expr): partition struct validity as a first-class coordinate#8874
Draft
joseph-isaacs wants to merge 1 commit into
Draft
feat(expr): partition struct validity as a first-class coordinate#8874joseph-isaacs wants to merge 1 commit into
joseph-isaacs wants to merge 1 commit into
Conversation
Partitioning a nullable struct scope decomposes it into n+1 coordinates,
but only the n fields had expression-level names: `replace_root_fields`
rewrote `$` into `pack(f: $.f, ...)`, whose validity is hard-wired
`lit(true)`. The validity coordinate was dropped from the term, then
re-applied by the struct reader as a positional side-channel (projection
only), which:
- constant-folded validity observers (`is_not_null($)` -> `lit(true)`,
`is_null($)` -> `lit(false)`) through pack's always-valid claim,
- let filter predicates match the bytes stored under null rows (the
filter path never read the validity child at all),
- masked validity-independent expressions (`lit(1)` came back null),
- guessed the mask position from the root expression's shape
(`is_pack_merge`), which is wrong for some expressions,
- addressed the validity child by the user-space name "validity",
collidable with a real field of that name.
This change gives the validity coordinate a name in the term and routes
it like any field:
- `replace_root_scope` expands a nullable struct scope with the identity
`$ == mask(pack(f: $.f, ...), is_not_null($))`, so the validity is
read and re-applied exactly where `$` occurred. For non-nullable
scopes the expansion is unchanged, and `mask(x, lit(true))`
simplification collapses the identity back to the plain pack.
- `StructPart { Field, Validity }` annotations (and
`make_struct_part_annotator`) route the coordinate by annotation, not
by field name.
- `is_null`/`is_not_null` absorb a child's derived validity expression
(via the new vtable-level `ScalarFnRef::validity_opt`), so
`is_not_null(mask(pack(..), V))` folds to `V` instead of `lit(true)`.
- `get_item` distributes over `mask`, keeping single-field projections
narrow: `get_item(f, mask(s, m)) == mask(get_item(f, s), m)`.
- `StructReader` routes `Validity` partitions to child 0 and the
projection-time validity side-channel is deleted; the `mask` node in
the partitioned root re-applies the validity at struct level via the
existing Mask kernel, and `null_as_false` already handles the filter
boundary.
The new struct reader tests fail before this change: filters observed
garbage under null rows and folded away null-checks, projections masked
literals and mis-evaluated `is_null($)`, and a field named "validity"
shadowed the struct's own validity.
Follow-ups (tracked separately): generic mask-distribution over
non-null-sensitive infallible fns, `and(m, mask(x, m)) -> and(m, x)` to
restore full filter pushdown, per-fn `is_null_sensitive` audit
(including `GetItem`), and sub-expression dedup in the partition
splitter.
Fixes #1907.
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fo6HoW3WV5UAKwVoymZjyf
Merging this PR will improve performance by 25.72%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
Contributor
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
Contributor
Benchmarks: Vortex queries 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.021x ➖, 0↑ 0↓)
datafusion / parquet (0.943x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.958x ➖, 0↑ 0↓)
duckdb / parquet (0.997x ➖, 0↑ 0↓)
No file size changes detected. |
joseph-isaacs
marked this pull request as draft
July 21, 2026 10:50
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
1 participant
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
Partitioning a nullable struct scope decomposes it into n+1 coordinates (n fields plus the struct's own validity), but only the n fields had expression-level names:
replace_root_fieldsrewrote$intopack(f: $.f, …), whose validity is hard-wiredlit(true). The validity coordinate was dropped from the term and re-applied byStructReaderas a positional side-channel — projection-only, at a syntactically guessed position. Consequences, each covered by a new failing-first test:is_not_null($)filtered to all-true,is_null($)projected wrongly.lit(1)over a nullable struct returned null at null rows.is_pack_merge), which cannot be correct for every expression."validity", collidable with a real field of that name.This PR makes the validity coordinate part of the expression term, so it is routed, read, and re-applied by the same generic partitioning machinery as fields. It is the first part of the plan discussed for #1907; follow-ups (generic mask-distribution over non-null-sensitive infallible fns,
and(m, mask(x, m)) → and(m, x)to restore full filter pushdown over nullable structs, a per-fnis_null_sensitiveaudit includingGetItem, and sub-expression dedup in the partition splitter) restore pushdown depth and are pure optimizations — this PR is the correctness core.What changes are included in this PR?
vortex-arrayreplace_root_scope(expr/transform/replace.rs): expands a nullable struct scope with the identity$ == mask(pack(f: $.f, …), is_not_null($)), so the validity is read and re-applied exactly where$occurred. Non-nullable scopes expand to the plain pack as before, and the existingmask(x, lit(true))simplification collapses the identity for scopes that turn out non-nullable.StructPart { Field(FieldName), Validity }+make_struct_part_annotator(expr/analysis/struct_part.rs): annotatesis_not_null($)as the validity coordinate and fields as before; partitions are routed by annotation, not by field name, removing the"validity"name collision.is_null/is_not_nullabsorb a child's vtable-derived validity expression via the newScalarFnRef::validity_opt(theOption-returning form; the existingvalidity()fallback ofis_not_null(child)would make the rewrite a self-referential no-op). WithMask::validity() = and(child_validity, m),is_not_null(mask(pack(..), V))folds toV.get_itemdistributes overmask(get_item(f, mask(s, m)) == mask(get_item(f, s), m)), keeping single-field projections narrow (they read the field plus the validity child, not every field).vortex-layoutStructReaderpartitions with theStructPartannotator, routesValiditypartitions to child 0, and steps validity partitions into the bool child's scope (is_not_null($) → $). The projection-time validity side-channel (validity_fut+is_pack_mergeguessing) is deleted: themasknode in the partitioned root re-applies validity at struct level through the existingMaskkernel, and the existingnull_as_falseat the filter boundary converts validity-induced nulls to excluded rows. The filter and pruning paths now see the validity coordinate through the same partitioning as projection.Tests (the five new
StructReadertests fail before this change; verified against the parent commit)is_not_null($)over a nullable struct returns the struct validity instead of all-trueeq($.a, 7)does not match a row whose stored bytes satisfy the predicate under a null struct rowis_null($)returns the negated validity instead of a mis-masked constantpack(x: lit(1))is not nulled at null struct rows"validity"projects as the field whileis_not_null($)still routes to the struct's validityPlus expression-level tests for the expansion identity, the absorb rules, the
get_item/maskdistribution, and partitioningand(is_not_null($), $.a > 5)into exactly{Validity}and{Field(a)}coordinates.Checks run:
cargo nextest run -p vortex-array -p vortex-layout -p vortex-file -p vortex-scan -p vortex-datafusion(3611 tests pass),cargo test --doc -p vortex-array,cargo +nightly fmt --all,cargo clippy --all-targetson the affected crate closure (clean). Not run: workspace-wide--all-featuresclippy (the sandbox lacksprotocforlance-encodingand network access for thevortex-duckdbbuild script — failures are environmental and unrelated to this change).What APIs are changed? Are there any user-facing changes?
expr::transform::replace_root_scope,expr::analysis::{StructPart, make_struct_part_annotator},ScalarFnRef::validity_opt. No existing signatures changed (replace_root_fieldsis kept).is_null($)/is_not_null($)observe real validity, and total expressions are no longer masked. Filters over nullable structs currently evaluate at struct level (partitions per referenced field + validity) rather than fully pushed down; pushdown is restored by the follow-up rewrite rules.🤖 Generated with Claude Code
https://claude.ai/code/session_01Fo6HoW3WV5UAKwVoymZjyf
Generated by Claude Code