From 66a2395ce20ec301f66fae5be846b553e11a93b4 Mon Sep 17 00:00:00 2001 From: Joe Isaacs Date: Tue, 21 Jul 2026 10:18:12 +0000 Subject: [PATCH] feat(expr): partition struct validity as a first-class coordinate 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 Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01Fo6HoW3WV5UAKwVoymZjyf --- vortex-array/src/expr/analysis/mod.rs | 2 + vortex-array/src/expr/analysis/struct_part.rs | 109 ++++++ vortex-array/src/expr/transform/partition.rs | 45 ++- vortex-array/src/expr/transform/replace.rs | 84 ++++- vortex-array/src/scalar_fn/erased.rs | 9 + vortex-array/src/scalar_fn/fns/get_item.rs | 25 ++ vortex-array/src/scalar_fn/fns/is_not_null.rs | 26 ++ vortex-array/src/scalar_fn/fns/is_null.rs | 32 ++ vortex-layout/src/layouts/struct_/reader.rs | 355 +++++++++++++----- 9 files changed, 575 insertions(+), 112 deletions(-) create mode 100644 vortex-array/src/expr/analysis/struct_part.rs diff --git a/vortex-array/src/expr/analysis/mod.rs b/vortex-array/src/expr/analysis/mod.rs index f5208a31be8..c181b52d18c 100644 --- a/vortex-array/src/expr/analysis/mod.rs +++ b/vortex-array/src/expr/analysis/mod.rs @@ -7,6 +7,7 @@ pub mod immediate_access; mod labeling; mod null_sensitive; mod referenced_field_paths; +pub mod struct_part; pub use annotation::*; pub use fallible::label_is_fallible; @@ -15,3 +16,4 @@ pub use labeling::*; pub use null_sensitive::BooleanLabels; pub use null_sensitive::label_null_sensitive; pub use referenced_field_paths::referenced_field_paths; +pub use struct_part::*; diff --git a/vortex-array/src/expr/analysis/struct_part.rs b/vortex-array/src/expr/analysis/struct_part.rs new file mode 100644 index 00000000000..7195cb2745e --- /dev/null +++ b/vortex-array/src/expr/analysis/struct_part.rs @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +use std::fmt::Display; +use std::fmt::Formatter; + +use vortex_error::VortexExpect; + +use crate::dtype::FieldName; +use crate::dtype::Nullability; +use crate::dtype::StructFields; +use crate::expr::Expression; +use crate::expr::analysis::AnnotationFn; +use crate::scalar_fn::fns::get_item::GetItem; +use crate::scalar_fn::fns::is_not_null::IsNotNull; +use crate::scalar_fn::fns::root::Root; +use crate::scalar_fn::fns::select::Select; + +/// A coordinate of a struct scope: either one of its fields, or — for a nullable struct — +/// the struct's own validity. +/// +/// A nullable struct with `n` fields decomposes into `n + 1` coordinates. Annotating +/// expressions with `StructPart` instead of a bare [`FieldName`] lets the validity +/// coordinate be routed like any field, and cannot collide with a real field that happens +/// to be named "validity". +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum StructPart { + /// A top-level field of the struct scope. + Field(FieldName), + /// The validity of the struct scope itself, referenced as `is_not_null($)`. + Validity, +} + +impl Display for StructPart { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + StructPart::Field(name) => write!(f, "{name}"), + StructPart::Validity => write!(f, "$validity"), + } + } +} + +impl From for FieldName { + fn from(part: StructPart) -> Self { + match part { + StructPart::Field(name) => name, + // NOTE: this name is only used in the synthetic scope that recombines + // partitions. It is not distinguishable from a real field literally named + // "$validity"; routing must use the `StructPart` annotation, never this name. + StructPart::Validity => FieldName::from("$validity"), + } + } +} + +/// Returns the free [`StructPart`] coordinates for an expression node. +/// +/// This extends [`super::make_free_field_annotator`] with the validity coordinate of a +/// nullable scope: +/// +/// - **`is_not_null(root())`**: Returns `[Validity]` — the canonical reference to the +/// scope's validity coordinate (it is also what [`crate::scalar_fn::fns::mask::Mask`]'s +/// validity derivation produces for the expanded root, see +/// [`crate::expr::transform::replace_root_scope`]). +/// - **[`Select`] / [`GetItem`] on [`Root`]**: Returns the referenced fields, as +/// `Field(..)`. These reference the field coordinates *without* the scope validity. +/// - **[`Root`]**: Returns every field, plus `Validity` if the scope is nullable +/// (conservative over-approximation: a bare `$` observes all coordinates). +/// - **Everything else**: Returns empty (annotations aggregate from children). +pub fn make_struct_part_annotator( + scope: &StructFields, + scope_nullability: Nullability, +) -> impl AnnotationFn { + move |expr: &Expression| { + if expr.is::() && expr.child(0).is::() { + if scope_nullability.is_nullable() { + return vec![StructPart::Validity]; + } + // A non-nullable scope has no validity coordinate; `is_not_null($)` is a + // constant. Leave it unannotated so it does not force a partition. + return vec![]; + } else if let Some(selection) = expr.as_opt::