From 8ad62d47af08b7becd755bad7404833795489b31 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Fri, 23 Oct 2026 12:05:57 -0400 Subject: [PATCH 1/4] Attach batch validity directly to canonical RowFn output Signed-off-by: Connor Tsui --- .../unstable/row/batch/execute/dense.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs b/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs index cd5ab2aeea1..1cd7fba843b 100644 --- a/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs +++ b/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs @@ -9,8 +9,12 @@ use vortex_mask::MaskValuesRef; use super::super::RowFnExecutionArgs; use super::super::args::BorrowedRowFnArgs; use crate::ArrayRef; +use crate::Canonical; use crate::ExecutionCtx; +use crate::IntoArray; +use crate::arrays::masked::mask_validity_canonical; use crate::builtins::ArrayBuiltins; +use crate::canonical::AnyCanonical; use crate::scalar_fn::unstable::row::execute::DenseAttempt; use crate::validity::Validity; @@ -107,7 +111,19 @@ impl RowFnExecutionArgs { Validity::NonNullable | Validity::AllValid => { self.finalize_output(values, self.row_count) } - Validity::Array(valid) => self.finalize_output(values.mask(valid)?, self.row_count), + Validity::Array(valid) => { + // Canonical output can carry validity directly, without a second mask execution. + let values = match values.as_opt::() { + Some(canonical) => mask_validity_canonical( + Canonical::from(canonical), + Validity::Array(valid), + ctx, + )? + .into_array(), + None => values.mask(valid)?, + }; + self.finalize_output(values, self.row_count) + } Validity::AllInvalid => { unreachable!("all-invalid validity is handled before dense row execution") } From 7546753e2b711d50bb6b17e985a311408ced1de7 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Mon, 26 Oct 2026 10:51:51 -0400 Subject: [PATCH 2/4] Document the benchmark-driven RowFn validity fast path Signed-off-by: Connor Tsui --- .../src/scalar_fn/unstable/row/batch/execute/dense.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs b/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs index 1cd7fba843b..a636ad88b36 100644 --- a/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs +++ b/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs @@ -112,7 +112,12 @@ impl RowFnExecutionArgs { self.finalize_output(values, self.row_count) } Validity::Array(valid) => { - // Canonical output can carry validity directly, without a second mask execution. + // This internal micro-optimization skips the temporary ScalarFnArray and optimizer + // pass in `values.mask(valid)`, even when its reduction attaches validity directly. + // Isolated benchmarks measured about 15-20% less time for nullable u64 addition on + // 64-row batches with a right-hand constant, not a general speedup. Use `.mask()` + // here if it gains an equivalent direct dispatch path. Revisions and limitations: + // https://github.com/vortex-data/vortex/pull/10742 let values = match values.as_opt::() { Some(canonical) => mask_validity_canonical( Canonical::from(canonical), From b756862d95bd95d604ef57a2579473c08650c12a Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Mon, 26 Oct 2026 10:55:32 -0400 Subject: [PATCH 3/4] Explain why RowFn bypasses generic mask dispatch Signed-off-by: Connor Tsui --- .../scalar_fn/unstable/row/batch/execute/dense.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs b/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs index a636ad88b36..63f14eabd91 100644 --- a/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs +++ b/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs @@ -112,11 +112,14 @@ impl RowFnExecutionArgs { self.finalize_output(values, self.row_count) } Validity::Array(valid) => { - // This internal micro-optimization skips the temporary ScalarFnArray and optimizer - // pass in `values.mask(valid)`, even when its reduction attaches validity directly. - // Isolated benchmarks measured about 15-20% less time for nullable u64 addition on - // 64-row batches with a right-hand constant, not a general speedup. Use `.mask()` - // here if it gains an equivalent direct dispatch path. Revisions and limitations: + // `values.mask(valid)` currently reaches encoding-specific masking through + // parent-reduce rules, which need a ScalarFnArray parent to match. Even a successful + // reduction pays for constructing and optimizing that temporary node. Benchmarks + // motivated skipping this setup in RowFn finalization. Doing so inside `.mask()` + // would require direct encoding dispatch or a canonical-specific branch in the + // generic builtin. Keep this micro-optimization local to the internal executor, + // reusing the existing helper instead of changing generic masking dispatch for this + // hot path. Remove this bypass if `.mask()` gains equivalent direct dispatch. // https://github.com/vortex-data/vortex/pull/10742 let values = match values.as_opt::() { Some(canonical) => mask_validity_canonical( From 41b6e8b83bb3419bd7298b4d086e4c8ee0e124c0 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Mon, 26 Oct 2026 10:56:44 -0400 Subject: [PATCH 4/4] Shorten the RowFn masking rationale Signed-off-by: Connor Tsui --- .../scalar_fn/unstable/row/batch/execute/dense.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs b/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs index 63f14eabd91..50835c0ded5 100644 --- a/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs +++ b/vortex-array/src/scalar_fn/unstable/row/batch/execute/dense.rs @@ -112,14 +112,10 @@ impl RowFnExecutionArgs { self.finalize_output(values, self.row_count) } Validity::Array(valid) => { - // `values.mask(valid)` currently reaches encoding-specific masking through - // parent-reduce rules, which need a ScalarFnArray parent to match. Even a successful - // reduction pays for constructing and optimizing that temporary node. Benchmarks - // motivated skipping this setup in RowFn finalization. Doing so inside `.mask()` - // would require direct encoding dispatch or a canonical-specific branch in the - // generic builtin. Keep this micro-optimization local to the internal executor, - // reusing the existing helper instead of changing generic masking dispatch for this - // hot path. Remove this bypass if `.mask()` gains equivalent direct dispatch. + // `values.mask(valid)` builds and optimizes a ScalarFnArray to dispatch through + // parent-reduce rules. Benchmarks motivated bypassing that setup here, keeping the + // exception internal instead of adding canonical-specific dispatch to the generic + // builtin. Remove this bypass if `.mask()` gains equivalent direct dispatch. // https://github.com/vortex-data/vortex/pull/10742 let values = match values.as_opt::() { Some(canonical) => mask_validity_canonical(