From 2d531bf77e7629226c92ac2d4d32b9fe04271b37 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Wed, 23 Sep 2026 14:51:21 +0100 Subject: [PATCH 01/22] reduce and/or binary function arrays with regards to constants Signed-off-by: Mikhail Kot --- vortex-array/Cargo.toml | 4 + vortex-array/benches/scalar_fn_probe.rs | 124 +++++++++++++++ vortex-array/src/array/probe/array.rs | 13 +- vortex-array/src/array/probe/repeated.rs | 14 +- vortex-array/src/arrays/scalar_fn/rules.rs | 44 +----- .../src/arrays/scalar_fn/vtable/mod.rs | 142 ++++++++---------- .../src/arrays/scalar_fn/vtable/operations.rs | 57 +++++++ .../src/arrays/scalar_fn/vtable/validity.rs | 84 ----------- vortex-array/src/expr/expression.rs | 7 +- vortex-array/src/expr/exprs.rs | 17 --- vortex-array/src/scalar_fn/erased.rs | 13 +- vortex-array/src/scalar_fn/fns/binary/mod.rs | 51 +++++++ vortex-spatial/src/scalar_fn/collect.rs | 13 +- vortex-spatial/src/scalar_fn/envelope.rs | 11 +- vortex-spatial/src/scalar_fn/length.rs | 13 +- vortex-spatial/src/scalar_fn/make_line.rs | 13 +- vortex-tensor/src/scalar_fns/l2_normalize.rs | 13 +- 17 files changed, 368 insertions(+), 265 deletions(-) create mode 100644 vortex-array/benches/scalar_fn_probe.rs delete mode 100644 vortex-array/src/arrays/scalar_fn/vtable/validity.rs diff --git a/vortex-array/Cargo.toml b/vortex-array/Cargo.toml index 5fed81aec1c..b0ecb9c13d5 100644 --- a/vortex-array/Cargo.toml +++ b/vortex-array/Cargo.toml @@ -232,6 +232,10 @@ harness = false name = "validity_is_valid" harness = false +[[bench]] +name = "scalar_fn_probe" +harness = false + [[bench]] name = "dict_unreferenced_mask" harness = false diff --git a/vortex-array/benches/scalar_fn_probe.rs b/vortex-array/benches/scalar_fn_probe.rs new file mode 100644 index 00000000000..4c605a87eb7 --- /dev/null +++ b/vortex-array/benches/scalar_fn_probe.rs @@ -0,0 +1,124 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +#![expect(clippy::unwrap_used)] + +use std::sync::LazyLock; + +use divan::Bencher; +use divan::black_box; +use rand::RngExt; +use rand::SeedableRng; +use rand::rngs::StdRng; +use vortex_array::ArrayRef; +use vortex_array::IntoArray; +use vortex_array::VortexSessionExecute; +use vortex_array::array_session; +use vortex_array::arrays::BoolArray; +use vortex_array::arrays::PrimitiveArray; +use vortex_array::arrays::ScalarFnArray; +use vortex_array::scalar_fn::TypedScalarFnInstance; +use vortex_array::scalar_fn::fns::binary::Binary; +use vortex_array::scalar_fn::fns::operators::Operator; +use vortex_session::VortexSession; + +fn main() { + LazyLock::force(&SESSION); + divan::main(); +} + +const ARRAY_SIZE: usize = 100_000; +const NUM_ACCESSES: usize = 50; + +static SESSION: LazyLock = LazyLock::new(array_session); + +/// evaluating ADD's validity is cheaper than evaluating ADD +fn binary_add() -> ArrayRef { + let lhs = + PrimitiveArray::from_option_iter((0..ARRAY_SIZE).map(|i| (i % 7 != 0).then_some(i as i64))) + .into_array(); + let rhs = PrimitiveArray::from_iter((0..ARRAY_SIZE).map(|i| i as i64)).into_array(); + let scalar_fn = TypedScalarFnInstance::new(Binary, Operator::Add).erased(); + ScalarFnArray::try_new(scalar_fn, vec![lhs, rhs]) + .unwrap() + .into_array() +} + +/// evaluating AND's validity is equal to evaluating the AND due to +/// Kleene semantics +fn binary_and() -> ArrayRef { + let lhs = BoolArray::from_iter((0..ARRAY_SIZE).map(|i| (i % 7 != 0).then_some(i % 2 == 0))) + .into_array(); + let rhs = BoolArray::from_iter((0..ARRAY_SIZE).map(|i| i % 2 == 0)).into_array(); + let scalar_fn = TypedScalarFnInstance::new(Binary, Operator::And).erased(); + ScalarFnArray::try_new(scalar_fn, vec![lhs, rhs]) + .unwrap() + .into_array() +} + +fn indices() -> Vec { + let mut rng = StdRng::seed_from_u64(0); + (0..NUM_ACCESSES) + .map(|_| rng.random_range(0..ARRAY_SIZE)) + .collect() +} + +#[divan::bench(args = [binary_and(), binary_add()])] +fn probe_scalar_fn_once(bencher: Bencher, array: &ArrayRef) { + let indices = indices(); + bencher + .with_inputs(|| (&array, &indices, SESSION.create_execution_ctx())) + .bench_refs(|(array, indices, ctx)| { + for &index in indices.iter() { + black_box(array.probe().execute_scalar(index, ctx).unwrap()); + } + }); +} + +#[divan::bench(args = [binary_and(), binary_add()])] +fn probe_scalar_fn_repeated(bencher: Bencher, array: &ArrayRef) { + let indices = indices(); + bencher + .with_inputs(|| { + ( + array.repeated_probe(), + &indices, + SESSION.create_execution_ctx(), + ) + }) + .bench_refs(|(probe, indices, ctx)| { + for &index in indices.iter() { + black_box(probe.execute_scalar(index, ctx).unwrap()); + } + }); +} + +#[divan::bench(args = [binary_and(), binary_add()])] +fn probe_scalar_fn_valid_once(bencher: Bencher, array: &ArrayRef) { + let indices = indices(); + bencher + .with_inputs(|| (&array, &indices, SESSION.create_execution_ctx())) + .bench_refs(|(array, indices, ctx)| { + for &index in indices.iter() { + black_box(array.probe().execute_is_valid(index, ctx).unwrap()); + } + }); +} + +#[divan::bench(args = [binary_and(), binary_add()])] +fn probe_scalar_fn_valid_repeated(bencher: Bencher, array: &ArrayRef) { + let indices = indices(); + bencher + .with_inputs(|| { + ( + array.repeated_probe(), + &indices, + SESSION.create_execution_ctx(), + ) + }) + .bench_refs(|(probe, indices, ctx)| { + for &index in indices.iter() { + black_box(probe.execute_is_invalid(index, ctx).unwrap()); + } + }); +} diff --git a/vortex-array/src/array/probe/array.rs b/vortex-array/src/array/probe/array.rs index d53afdbe3e2..7d3c83cb3b8 100644 --- a/vortex-array/src/array/probe/array.rs +++ b/vortex-array/src/array/probe/array.rs @@ -15,6 +15,7 @@ use crate::array::probe::RepeatedArrayProbe; use crate::array::probe::RepeatedState; use crate::array::probe::repeated::child_probe; use crate::arrays::Primitive; +use crate::arrays::ScalarFn; use crate::scalar::Scalar; use crate::vtable::OperationsVTable; @@ -82,7 +83,17 @@ fn execute_scalar_once( index: usize, ctx: &mut ExecutionCtx, ) -> VortexResult { - if !execute_is_valid_once(array, index, ctx)? { + // ScalarFn's validity is lazy, and for some functions evaluating + // validity is equal to evaluating the function. For such functions + // validity() is is_not_null(original array). So we get the chain: + // execute_is_valid_once -> array.validity() -> + // execute_is_valid -> execute_scalar (mask) -> + // mask.probe_scalar_once -> scalar_at -> array.execute_scalar, and as + // "array" is the original array, we get infinite recursion. + // + // For these functions probe_scalar_once gets the nullable scalar anyway. + // See also execute_scalar in probe/repeated.rs + if !array.is::() && !execute_is_valid_once(array, index, ctx)? { return Ok(Scalar::null(array.dtype().clone())); } check_dtype( diff --git a/vortex-array/src/array/probe/repeated.rs b/vortex-array/src/array/probe/repeated.rs index 3a6533b938e..49ed2ff7fb7 100644 --- a/vortex-array/src/array/probe/repeated.rs +++ b/vortex-array/src/array/probe/repeated.rs @@ -12,6 +12,7 @@ use crate::array::probe::ArrayProbe; use crate::array::probe::array::check_bounds; use crate::array::probe::array::check_dtype; use crate::array::probe::array::child_of; +use crate::arrays::ScalarFn; use crate::scalar::Scalar; use crate::validity::Validity; @@ -55,7 +56,18 @@ impl RepeatedArrayProbe { /// Read the scalar at `index`, including its nullness, reusing retained preparation. pub fn execute_scalar(&mut self, index: usize, ctx: &mut ExecutionCtx) -> VortexResult { - if !self.execute_is_valid(index, ctx)? { + // ScalarFn's validity is lazy, and for some functions evaluating + // validity is equal to evaluating the function. For such functions + // validity() is is_not_null(original array). So we get the chain: + // execute_scalar -> array.validity() -> + // execute_is_valid -> execute_scalar (mask) -> + // mask.probe_scalar_once -> scalar_at -> array.execute_scalar, and as + // "array" is the original array, we get infinite recursion. + // + // For these functions probe_scalar_once gets the nullable scalar anyway. + // + // See also execute_scala_once in probe/array.rs + if !self.array.is::() && !self.execute_is_valid(index, ctx)? { return Ok(Scalar::null(self.array.dtype().clone())); } let result = diff --git a/vortex-array/src/arrays/scalar_fn/rules.rs b/vortex-array/src/arrays/scalar_fn/rules.rs index 644f8e8e689..58fa537c510 100644 --- a/vortex-array/src/arrays/scalar_fn/rules.rs +++ b/vortex-array/src/arrays/scalar_fn/rules.rs @@ -16,24 +16,16 @@ use crate::arrays::Slice; use crate::arrays::StructArray; use crate::arrays::filter::prepare_mask_for_reuse; use crate::arrays::scalar_fn::ScalarFnArrayExt; -use crate::builtins::ArrayBuiltins; use crate::optimizer::rules::ArrayParentReduceRule; use crate::optimizer::rules::ArrayReduceRule; use crate::optimizer::rules::ParentRuleSet; use crate::optimizer::rules::ReduceRuleSet; use crate::scalar_fn::ArrayReduceNode; -use crate::scalar_fn::fns::is_not_null::IsNotNull; -use crate::scalar_fn::fns::is_null::IsNull; use crate::scalar_fn::fns::pack::Pack; use crate::validity::Validity; -pub(super) const RULES: ReduceRuleSet = ReduceRuleSet::new(&[ - &ScalarFnPackToStructRule, - &IsNullReduceRule, - // Ordering is important. ScalarFn::reduce() must be called after all other - // optimizations - &ScalarFnAbstractReduceRule, -]); +pub(super) const RULES: ReduceRuleSet = + ReduceRuleSet::new(&[&ScalarFnPackToStructRule, &ScalarFnAbstractReduceRule]); pub(super) const PARENT_RULES: ParentRuleSet = ParentRuleSet::new(&[ ParentRuleSet::lift(&ScalarFilterPushdownRule), @@ -66,38 +58,6 @@ impl ArrayReduceRule for ScalarFnPackToStructRule { } } -/// Reduce IsNull(x) -> lit(false) if !x.nullable or x.validity(). -/// Reduce IsNotNull(x) -> lit(true) if !x.nullable or x.validity() -#[derive(Debug)] -struct IsNullReduceRule; -impl ArrayReduceRule for IsNullReduceRule { - fn reduce(&self, view: ArrayView<'_, ScalarFn>) -> VortexResult> { - let mut is_null = view.scalar_fn().is::(); - if !is_null { - if view.scalar_fn().is::() { - is_null = false; - } else { - return Ok(None); - } - } - - let validity = match view.get_child(0).validity()? { - Validity::NonNullable | Validity::AllValid => { - ConstantArray::new(!is_null, view.len()).into_array() - } - Validity::AllInvalid => ConstantArray::new(is_null, view.len()).into_array(), - Validity::Array(array) => { - if is_null { - array.not()? - } else { - array - } - } - }; - Ok(Some(validity)) - } -} - #[derive(Debug)] struct ScalarFnSliceReduceRule; impl ArrayParentReduceRule for ScalarFnSliceReduceRule { diff --git a/vortex-array/src/arrays/scalar_fn/vtable/mod.rs b/vortex-array/src/arrays/scalar_fn/vtable/mod.rs index c84a36a3169..909e4c79a5d 100644 --- a/vortex-array/src/arrays/scalar_fn/vtable/mod.rs +++ b/vortex-array/src/arrays/scalar_fn/vtable/mod.rs @@ -1,9 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors mod operations; -mod validity; -use std::fmt::Display; -use std::fmt::Formatter; use std::hash::Hash; use std::hash::Hasher; use std::marker::PhantomData; @@ -15,37 +12,40 @@ use vortex_error::vortex_bail; use vortex_error::vortex_ensure; use vortex_error::vortex_panic; use vortex_session::VortexSession; -use vortex_session::registry::CachedId; use crate::ArrayEq; use crate::ArrayHash; use crate::ArrayRef; use crate::EqMode; +use crate::IntoArray; use crate::array::Array; use crate::array::ArrayId; use crate::array::ArrayParts; use crate::array::ArrayView; use crate::array::VTable; +use crate::array::ValidityVTable; use crate::array::with_empty_buffers; +use crate::arrays::StructArray; use crate::arrays::scalar_fn::array::ScalarFnArrayExt; use crate::arrays::scalar_fn::array::ScalarFnData; use crate::arrays::scalar_fn::rules::PARENT_RULES; use crate::arrays::scalar_fn::rules::RULES; use crate::buffer::BufferHandle; use crate::dtype::DType; +use crate::dtype::FieldName; use crate::executor::ExecutionCtx; use crate::executor::ExecutionResult; use crate::expr::Expression; -use crate::expr::display::ExprDisplay; +use crate::expr::get_item; +use crate::expr::is_not_null; +use crate::expr::lit; +use crate::expr::root; use crate::matcher::Matcher; use crate::scalar_fn; -use crate::scalar_fn::Arity; -use crate::scalar_fn::ChildName; -use crate::scalar_fn::ExecutionArgs; use crate::scalar_fn::ScalarFnId; -use crate::scalar_fn::ScalarFnVTableExt; use crate::scalar_fn::VecExecutionArgs; use crate::serde::ArrayChildren; +use crate::validity::Validity; /// A [`ScalarFn`]-encoded Vortex array. pub type ScalarFnArray = Array; @@ -244,79 +244,55 @@ impl Deref for ScalarFnArrayView<'_, F> { } } -// Used only in this method to allow constrained using of Expression evaluate. -#[derive(Clone)] -struct ArrayExpr; - -#[derive(Clone, Debug)] -struct FakeEq(T); - -impl PartialEq for FakeEq { - fn eq(&self, _other: &Self) -> bool { - false - } -} - -impl Eq for FakeEq {} - -impl Hash for FakeEq { - fn hash(&self, _state: &mut H) {} -} - -impl Display for FakeEq { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.0.encoding_id()) - } -} - -impl scalar_fn::ScalarFnVTable for ArrayExpr { - type Options = FakeEq; - - fn id(&self) -> ScalarFnId { - static ID: CachedId = CachedId::new("vortex.array"); - *ID - } - - fn arity(&self, _options: &Self::Options) -> Arity { - Arity::Exact(0) - } - - fn child_name(&self, _options: &Self::Options, _child_idx: usize) -> ChildName { - todo!() - } - - fn fmt_sql( - &self, - options: &Self::Options, - _expr: &dyn ExprDisplay, - f: &mut Formatter<'_>, - ) -> std::fmt::Result { - write!(f, "{}", options.0.encoding_id()) - } - - fn return_dtype(&self, options: &Self::Options, _arg_dtypes: &[DType]) -> VortexResult { - Ok(options.0.dtype().clone()) - } - - fn execute( - &self, - options: &Self::Options, - _args: &dyn ExecutionArgs, - ctx: &mut ExecutionCtx, - ) -> VortexResult { - crate::Executable::execute(options.0.clone(), ctx) - } - - fn validity( - &self, - options: &Self::Options, - _expression: &Expression, - ) -> VortexResult> { - let validity_array = options.0.validity()?.to_array(options.0.len()); - Ok(Some(ArrayExpr.new_expr(FakeEq(validity_array), []))) - } - - fn is_strict(&self, _options: &Self::Options) -> bool { - true +impl ValidityVTable for ScalarFn { + fn validity(view: ArrayView<'_, ScalarFn>) -> VortexResult { + // We want to defer execution of the underlying array. The naïve + // solution for this is to build an all true array and then .apply() an + // Expression referencing parts of root(). This doesn't work because + // in this Expression's evaluation root() is replaced by the original + // array which leads to non-terminating recursion, a stack overflow. So + // we build a Struct array and give the caller (which overrides + // ScalarFn valididy) the ability to reference children with get_item. + // In ScalarFn's overriden validity "expr.child(i)" then translates to + // "view.get_item(i)" which doesn't produce recursion since get_item + // references part of the original array as opposed to root(). + let child_count = view.child_count(); + let names = (0..child_count) + .map(|i| FieldName::from(i.to_string().as_str())) + .collect(); + let fields = view.children(); + + let getters: Vec<_> = view + .children() + .into_iter() + .enumerate() + .map(|(i, child)| { + if let Some(scalar) = child.as_constant() { + lit(scalar) + } else { + get_item(i.to_string(), root()) + } + }) + .collect(); + + let struct_array = StructArray::new(names, fields, view.len(), Validity::NonNullable); + + let scalar_fn = view.scalar_fn(); + let expr = Expression::try_new(scalar_fn.clone(), getters)?; + let expr = scalar_fn + .validity(&expr)? + // However, there is another possible stack overflow if validity() + // isn't overriden. The naïve solution is to do is_not_null(expr) + // which is is_not_null(ScalarFn(get_item(...))). + // Inner ScalarFn(get_item)'s row request will call validity() back + // which will instantiate is_not_null(F( original is_not_null )). + // + // So, to break this recursion, we need to tweak array probing for + // ScalarFn, see execute_scalar in probe/array.rs and in + // probe/repeated.rs + .unwrap_or_else(|| is_not_null(expr.clone())); + + let array = struct_array.into_array().apply(&expr)?; + Ok(Validity::Array(array)) } } diff --git a/vortex-array/src/arrays/scalar_fn/vtable/operations.rs b/vortex-array/src/arrays/scalar_fn/vtable/operations.rs index 21af5728572..cb246451bf4 100644 --- a/vortex-array/src/arrays/scalar_fn/vtable/operations.rs +++ b/vortex-array/src/arrays/scalar_fn/vtable/operations.rs @@ -60,6 +60,7 @@ mod tests { use vortex_buffer::buffer; use vortex_error::VortexResult; + use crate::ArrayRef; use crate::ArraySlots; use crate::Canonical; use crate::IntoArray; @@ -244,4 +245,60 @@ mod tests { Ok(()) } + + /// Mul(Add(a1, a2), a3) + fn scalar_nested_add() -> VortexResult { + let lhs_n = PrimitiveArray::from_option_iter((0..10i64).map(|i| (i % 4 != 0).then_some(i))); + let rhs_r = PrimitiveArray::from_iter(0..10i64); + + let scalar_fn_n = TypedScalarFnInstance::new(Binary, Operator::Add).erased(); + let args_n = vec![lhs_n.into_array(), rhs_r.into_array()]; + + let lhs = ScalarFnArray::try_new(scalar_fn_n, args_n)?.into_array(); + let rhs = PrimitiveArray::from_iter(0..10i64); + + let scalar_fn = TypedScalarFnInstance::new(Binary, Operator::Mul).erased(); + let args = vec![lhs.into_array(), rhs.into_array()]; + + Ok(ScalarFnArray::try_new(scalar_fn, args)?.into_array()) + } + + #[test] + fn scalar_fn_probe() -> VortexResult<()> { + let ctx = &mut array_session().create_execution_ctx(); + let array = scalar_nested_add()?; + let mut probe = array.probe(); + + assert!(!probe.execute_is_valid(0, ctx)?); + assert!(probe.execute_scalar(0, ctx)?.is_null()); + assert!(probe.execute_is_valid(9, ctx)?); + assert_eq!(probe.execute_scalar(9, ctx)?, Scalar::from(Some(162i64))); + + Ok(()) + } + + #[test] + fn scalar_fn_repeated_probe() -> VortexResult<()> { + let ctx = &mut array_session().create_execution_ctx(); + let array = scalar_nested_add()?; + let mut probe = array.repeated_probe(); + + assert!(!probe.execute_is_valid(0, ctx)?); + assert!(probe.execute_scalar(0, ctx)?.is_null()); + assert!(probe.execute_is_valid(9, ctx)?); + assert_eq!(probe.execute_scalar(9, ctx)?, Scalar::from(Some(162i64))); + + Ok(()) + } + + #[test] + fn scalar_fn_all_valid() -> VortexResult<()> { + let ctx = &mut array_session().create_execution_ctx(); + let array = scalar_nested_add()?; + assert!(!array.all_valid(ctx)?); + assert!(!array.all_invalid(ctx)?); + assert_eq!(array.valid_count(ctx)?, 7); + assert_eq!(array.invalid_count(ctx)?, 3); + Ok(()) + } } diff --git a/vortex-array/src/arrays/scalar_fn/vtable/validity.rs b/vortex-array/src/arrays/scalar_fn/vtable/validity.rs deleted file mode 100644 index 4e8ab0c95c0..00000000000 --- a/vortex-array/src/arrays/scalar_fn/vtable/validity.rs +++ /dev/null @@ -1,84 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// SPDX-FileCopyrightText: Copyright the Vortex contributors - -use vortex_error::VortexResult; -use vortex_error::vortex_bail; - -use crate::ArrayRef; -use crate::ExecutionCtx; -use crate::IntoArray; -use crate::VortexSessionExecute; -use crate::array::ArrayView; -use crate::array::ValidityVTable; -use crate::arrays::ConstantArray; -use crate::arrays::scalar_fn::ScalarFnArrayExt; -use crate::arrays::scalar_fn::vtable::ArrayExpr; -use crate::arrays::scalar_fn::vtable::FakeEq; -use crate::arrays::scalar_fn::vtable::ScalarFn; -use crate::expr::Expression; -use crate::expr::lit; -use crate::legacy_session; -use crate::scalar_fn::TypedScalarFnInstance; -use crate::scalar_fn::VecExecutionArgs; -use crate::scalar_fn::fns::literal::Literal; -use crate::validity::Validity; - -/// Execute an expression tree recursively. -/// -/// This assumes all leaf expressions are either ArrayExpr (wrapping actual arrays) or Literals. -fn execute_expr( - expr: &Expression, - row_count: usize, - ctx: &mut ExecutionCtx, -) -> VortexResult { - // Only Expression::Scalar is executable - let Some(scalar_fn) = expr.as_scalar() else { - vortex_bail!("Only Expression::Scalar is executable"); - }; - - // Handle Literal expression - create a constant array - if expr.is::() { - let scalar = expr.as_::(); - return Ok(ConstantArray::new(scalar.clone(), row_count).into_array()); - } - - // Recursively execute child expressions to get input arrays - let inputs: Vec = expr - .children() - .iter() - .map(|child| execute_expr(child, row_count, ctx)) - .collect::>()?; - - let args = VecExecutionArgs::new(inputs, row_count); - - Ok(scalar_fn.execute(&args, ctx)?.into_array()) -} - -impl ValidityVTable for ScalarFn { - fn validity(array: ArrayView<'_, ScalarFn>) -> VortexResult { - let inputs: Vec<_> = array - .iter_children() - .map(|child| { - if let Some(scalar) = child.as_constant() { - return Ok(lit(scalar)); - } - Expression::try_new( - TypedScalarFnInstance::new(ArrayExpr, FakeEq(child.clone())).erased(), - [], - ) - }) - .collect::>()?; - - let expr = Expression::try_new(array.scalar_fn().clone(), inputs)?; - let validity_expr = array.scalar_fn().validity(&expr)?; - - #[allow(clippy::disallowed_methods)] - let ctx = &mut legacy_session().create_execution_ctx(); - // Execute the validity expression. All leaves are ArrayExpr nodes. - Ok(Validity::Array(execute_expr( - &validity_expr, - array.len(), - ctx, - )?)) - } -} diff --git a/vortex-array/src/expr/expression.rs b/vortex-array/src/expr/expression.rs index 3ae9adb72d5..93018dfb367 100644 --- a/vortex-array/src/expr/expression.rs +++ b/vortex-array/src/expr/expression.rs @@ -18,8 +18,11 @@ use crate::dtype::DType; use crate::expr::display::DisplayTreeExpr; use crate::expr::traversal::TraversalOrder; use crate::expr::traversal::pre_order_visit_down; +use crate::scalar_fn::EmptyOptions; use crate::scalar_fn::ScalarFnRef; use crate::scalar_fn::ScalarFnVTable; +use crate::scalar_fn::ScalarFnVTableExt; +use crate::scalar_fn::fns::is_not_null::IsNotNull; /// An empty child slice, returned by [`Expression::children`] for childless variants. const NO_CHILDREN: &[Expression] = &[]; @@ -164,7 +167,9 @@ impl Expression { match self { // The scope is exactly as valid as itself. Self::Root => Ok(Self::Root), - Self::Scalar { scalar_fn, .. } => scalar_fn.validity(self), + Self::Scalar { scalar_fn, .. } => Ok(scalar_fn + .validity(self)? + .unwrap_or_else(|| IsNotNull.new_expr(EmptyOptions, [self.clone()]))), } } diff --git a/vortex-array/src/expr/exprs.rs b/vortex-array/src/expr/exprs.rs index f860c12c040..9a5aca415aa 100644 --- a/vortex-array/src/expr/exprs.rs +++ b/vortex-array/src/expr/exprs.rs @@ -6,7 +6,6 @@ use std::sync::Arc; use vortex_error::VortexExpect; -use vortex_error::VortexResult; use vortex_error::vortex_panic; use vortex_utils::iter::ReduceBalancedIterExt; @@ -598,22 +597,6 @@ where iter.into_iter().reduce_balanced(bound_and) } -/// The conjunction of an expression's child validities — i.e. the validity of a scalar function -/// whose result is null exactly when any operand is null. -/// -/// This is the `ScalarFnVTable::validity` for kernels that propagate nulls and never produce a -/// null from non-null inputs (comparisons, arithmetic, most spatial and tensor operations). Returning it lets -/// the planner derive the output's null mask without executing the kernel. Yields `None` when the -/// expression has no children. -pub fn union_child_validities(expression: &Expression) -> VortexResult> { - let child_validities = expression - .children() - .iter() - .map(Expression::validity) - .collect::>>()?; - Ok(and_collect(child_validities)) -} - /// Create a new [`Binary`] using the [`Add`](Operator::Add) operator. /// /// ## Example usage diff --git a/vortex-array/src/scalar_fn/erased.rs b/vortex-array/src/scalar_fn/erased.rs index 179743c0a78..25b2981ced9 100644 --- a/vortex-array/src/scalar_fn/erased.rs +++ b/vortex-array/src/scalar_fn/erased.rs @@ -23,13 +23,10 @@ use crate::expr::BoundExpression; use crate::expr::Expression; use crate::expr::display::ExprDisplay; use crate::scalar_fn::ArrayReduceNode; -use crate::scalar_fn::EmptyOptions; use crate::scalar_fn::ExecutionArgs; use crate::scalar_fn::ExpressionReduceNode; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; -use crate::scalar_fn::ScalarFnVTableExt; -use crate::scalar_fn::fns::is_not_null::IsNotNull; use crate::scalar_fn::options::ScalarFnOptions; use crate::scalar_fn::signature::ScalarFnSignature; use crate::scalar_fn::typed::DynScalarFn; @@ -125,12 +122,10 @@ impl ScalarFnRef { self.0.return_dtype(arg_types) } - /// Transforms the expression into one representing the validity of this expression. - pub fn validity(&self, expr: &Expression) -> VortexResult { - Ok(self.0.validity(expr)?.unwrap_or_else(|| { - // TODO(ngates): make validity a mandatory method on VTable to avoid this fallback. - IsNotNull.new_expr(EmptyOptions, [expr.clone()]) - })) + /// Some(E) if evaluating validity for this function is faster than + /// evaluating the function itself, None otherwise. + pub fn validity(&self, expr: &Expression) -> VortexResult> { + self.0.validity(expr) } /// Execute the expression given the input arguments. diff --git a/vortex-array/src/scalar_fn/fns/binary/mod.rs b/vortex-array/src/scalar_fn/fns/binary/mod.rs index e6873a663d4..2883859b654 100644 --- a/vortex-array/src/scalar_fn/fns/binary/mod.rs +++ b/vortex-array/src/scalar_fn/fns/binary/mod.rs @@ -341,6 +341,9 @@ mod tests { use crate::arrays::BoolArray; use crate::arrays::ConstantArray; use crate::arrays::PrimitiveArray; + use crate::arrays::ScalarFn; + use crate::arrays::scalar_fn::ExactScalarFn; + use crate::arrays::scalar_fn::ScalarFnArrayExt; use crate::assert_arrays_eq; use crate::builtins::ArrayBuiltins; use crate::dtype::DType; @@ -730,4 +733,52 @@ mod tests { let rhs = PrimitiveArray::from_iter([1i32, 2, 3]).into_array(); assert!(Binary::try_new(lhs, rhs, Operator::And).is_err()); } + + #[test] + fn test_isnull_and_reduce() -> VortexResult<()> { + let mut ctx = array_session().create_execution_ctx(); + let left = BoolArray::from_iter([Some(true), Some(true), None]).into_array(); + let right = BoolArray::from_iter([true, false, true]).into_array(); + + // IsNull(and(x, y)) -> and(IsNull(x), y) -> and(not(x.validity), y) + // ^ nullable + for (lhs, rhs) in [(left.clone(), right.clone()), (right.clone(), left)] { + let array = lhs + .binary(rhs.clone(), Operator::And)? + .is_null()? + .optimize()?; + assert_eq!(*array.as_::>().options, Operator::And); + assert_arrays_eq!( + array.as_::().get_child(0), // not(left.validity()) + BoolArray::from_iter([false, false, true]), + &mut ctx + ); + assert_arrays_eq!(array.as_::().get_child(1), right, &mut ctx); + } + Ok(()) + } + + #[test] + fn test_isnotnull_or_reduce() -> VortexResult<()> { + let mut ctx = array_session().create_execution_ctx(); + let left = BoolArray::from_iter([Some(true), Some(true), None]).into_array(); + let right = BoolArray::from_iter([true, false, true]).into_array(); + + // IsNotNull(or(x, y)) -> or(IsNotNull(x), y) -> or(x.validity, y) + // ^ nullable + for (lhs, rhs) in [(left.clone(), right.clone()), (right.clone(), left)] { + let array = lhs + .binary(rhs.clone(), Operator::Or)? + .is_not_null()? + .optimize()?; + assert_eq!(*array.as_::>().options, Operator::Or); + assert_arrays_eq!( + array.as_::().get_child(0), // left.validity() + BoolArray::from_iter([true, true, false]), + &mut ctx + ); + assert_arrays_eq!(array.as_::().get_child(1), right, &mut ctx); + } + Ok(()) + } } diff --git a/vortex-spatial/src/scalar_fn/collect.rs b/vortex-spatial/src/scalar_fn/collect.rs index a7708f8e8d3..30523fa1124 100644 --- a/vortex-spatial/src/scalar_fn/collect.rs +++ b/vortex-spatial/src/scalar_fn/collect.rs @@ -26,15 +26,16 @@ use vortex_array::dtype::Nullability; use vortex_array::dtype::PType; use vortex_array::dtype::extension::ExtDType; use vortex_array::dtype::extension::ExtDTypeRef; -use vortex_array::expr::Expression; -use vortex_array::expr::union_child_validities; use vortex_array::scalar_fn::Arity; use vortex_array::scalar_fn::ChildName; use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::ExecutionArgs; +use vortex_array::scalar_fn::ReduceNode; +use vortex_array::scalar_fn::ReduceNodeValidity; use vortex_array::scalar_fn::ScalarFnId; use vortex_array::scalar_fn::ScalarFnVTable; use vortex_array::scalar_fn::TypedScalarFnInstance; +use vortex_array::scalar_fn::union_child_validities; use vortex_array::validity::Validity; use vortex_buffer::Buffer; use vortex_buffer::BufferMut; @@ -305,12 +306,12 @@ impl ScalarFnVTable for SpatialCollect { ) } - fn validity( + fn validity( &self, _: &Self::Options, - expression: &Expression, - ) -> VortexResult> { - union_child_validities(expression) + node: &T, + ) -> VortexResult> { + Ok(ReduceNodeValidity::Reduced(union_child_validities(node)?)) } fn is_strict(&self, _: &Self::Options) -> bool { diff --git a/vortex-spatial/src/scalar_fn/envelope.rs b/vortex-spatial/src/scalar_fn/envelope.rs index 553fb415017..1056af8a337 100644 --- a/vortex-spatial/src/scalar_fn/envelope.rs +++ b/vortex-spatial/src/scalar_fn/envelope.rs @@ -18,12 +18,13 @@ use vortex_array::arrays::struct_::StructArrayExt; use vortex_array::dtype::DType; use vortex_array::dtype::Nullability; use vortex_array::dtype::extension::ExtDType; -use vortex_array::expr::Expression; use vortex_array::scalar::Scalar; use vortex_array::scalar_fn::Arity; use vortex_array::scalar_fn::ChildName; use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::ExecutionArgs; +use vortex_array::scalar_fn::ReduceNode; +use vortex_array::scalar_fn::ReduceNodeValidity; use vortex_array::scalar_fn::ScalarFnId; use vortex_array::scalar_fn::ScalarFnVTable; use vortex_array::scalar_fn::TypedScalarFnInstance; @@ -256,10 +257,14 @@ impl ScalarFnVTable for SpatialEnvelope { ) } - fn validity(&self, _: &Self::Options, _: &Expression) -> VortexResult> { + fn validity( + &self, + _: &Self::Options, + _: &T, + ) -> VortexResult> { // The output null mask is not derivable from the operand's validity alone: an empty // geometry yields a null box even where the operand is valid. Let the planner execute. - Ok(None) + Ok(ReduceNodeValidity::Irreducible) } fn is_strict(&self, _: &Self::Options) -> bool { diff --git a/vortex-spatial/src/scalar_fn/length.rs b/vortex-spatial/src/scalar_fn/length.rs index f2c12b72692..3744d2d4b2f 100644 --- a/vortex-spatial/src/scalar_fn/length.rs +++ b/vortex-spatial/src/scalar_fn/length.rs @@ -20,15 +20,16 @@ use vortex_array::builtins::ArrayBuiltins; use vortex_array::dtype::DType; use vortex_array::dtype::Nullability; use vortex_array::dtype::PType; -use vortex_array::expr::Expression; -use vortex_array::expr::union_child_validities; use vortex_array::scalar_fn::Arity; use vortex_array::scalar_fn::ChildName; use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::ExecutionArgs; +use vortex_array::scalar_fn::ReduceNode; +use vortex_array::scalar_fn::ReduceNodeValidity; use vortex_array::scalar_fn::ScalarFnId; use vortex_array::scalar_fn::ScalarFnVTable; use vortex_array::scalar_fn::TypedScalarFnInstance; +use vortex_array::scalar_fn::union_child_validities; use vortex_array::validity::Validity; use vortex_buffer::Buffer; use vortex_error::VortexResult; @@ -205,12 +206,12 @@ impl ScalarFnVTable for SpatialLength { ) } - fn validity( + fn validity( &self, _: &Self::Options, - expression: &Expression, - ) -> VortexResult> { - union_child_validities(expression) + node: &T, + ) -> VortexResult> { + Ok(ReduceNodeValidity::Reduced(union_child_validities(node)?)) } fn is_strict(&self, _: &Self::Options) -> bool { diff --git a/vortex-spatial/src/scalar_fn/make_line.rs b/vortex-spatial/src/scalar_fn/make_line.rs index fdd69676acd..52284a0c948 100644 --- a/vortex-spatial/src/scalar_fn/make_line.rs +++ b/vortex-spatial/src/scalar_fn/make_line.rs @@ -12,15 +12,16 @@ use vortex_array::arrays::StructArray; use vortex_array::dtype::DType; use vortex_array::dtype::Nullability; use vortex_array::dtype::extension::ExtDType; -use vortex_array::expr::Expression; -use vortex_array::expr::union_child_validities; use vortex_array::scalar_fn::Arity; use vortex_array::scalar_fn::ChildName; use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::ExecutionArgs; +use vortex_array::scalar_fn::ReduceNode; +use vortex_array::scalar_fn::ReduceNodeValidity; use vortex_array::scalar_fn::ScalarFnId; use vortex_array::scalar_fn::ScalarFnVTable; use vortex_array::scalar_fn::TypedScalarFnInstance; +use vortex_array::scalar_fn::union_child_validities; use vortex_array::validity::Validity; use vortex_error::VortexResult; use vortex_error::vortex_ensure; @@ -223,12 +224,12 @@ impl ScalarFnVTable for SpatialMakeLine { ) } - fn validity( + fn validity( &self, _: &Self::Options, - expression: &Expression, - ) -> VortexResult> { - union_child_validities(expression) + node: &T, + ) -> VortexResult> { + Ok(ReduceNodeValidity::Reduced(union_child_validities(node)?)) } fn is_strict(&self, _: &Self::Options) -> bool { diff --git a/vortex-tensor/src/scalar_fns/l2_normalize.rs b/vortex-tensor/src/scalar_fns/l2_normalize.rs index 74139a506a2..2e80c44f347 100644 --- a/vortex-tensor/src/scalar_fns/l2_normalize.rs +++ b/vortex-tensor/src/scalar_fns/l2_normalize.rs @@ -26,16 +26,17 @@ use vortex_array::dtype::DType; use vortex_array::dtype::Nullability; use vortex_array::dtype::StructFields; use vortex_array::dtype::proto::dtype as pb; -use vortex_array::expr::Expression; -use vortex_array::expr::union_child_validities; use vortex_array::match_each_float_ptype; use vortex_array::scalar_fn::Arity; use vortex_array::scalar_fn::ChildName; use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::ExecutionArgs; +use vortex_array::scalar_fn::ReduceNode; +use vortex_array::scalar_fn::ReduceNodeValidity; use vortex_array::scalar_fn::ScalarFnId; use vortex_array::scalar_fn::ScalarFnVTable; use vortex_array::scalar_fn::ScalarFnVTableExt; +use vortex_array::scalar_fn::union_child_validities; use vortex_array::serde::ArrayChildren; use vortex_array::validity::Validity; use vortex_buffer::BufferMut; @@ -179,12 +180,12 @@ impl ScalarFnVTable for L2Normalize { }) } - fn validity( + fn validity( &self, _options: &Self::Options, - expression: &Expression, - ) -> VortexResult> { - union_child_validities(expression) + node: &T, + ) -> VortexResult> { + Ok(ReduceNodeValidity::Reduced(union_child_validities(node)?)) } fn is_strict(&self, _options: &Self::Options) -> bool { From 20a6cf0ecfe514e1bf7aa9b36a962d3c27f8ab20 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 25 Sep 2026 15:40:03 +0100 Subject: [PATCH 02/22] replace useless overloads --- vortex-array/benches/scalar_fn_probe.rs | 2 +- vortex-array/src/expr/expression.rs | 23 +++++++++++++++----- vortex-array/src/expr/mod.rs | 1 - vortex-spatial/src/scalar_fn/collect.rs | 11 ---------- vortex-spatial/src/scalar_fn/length.rs | 11 ---------- vortex-spatial/src/scalar_fn/make_line.rs | 11 ---------- vortex-tensor/src/scalar_fns/l2_normalize.rs | 11 ---------- 7 files changed, 19 insertions(+), 51 deletions(-) diff --git a/vortex-array/benches/scalar_fn_probe.rs b/vortex-array/benches/scalar_fn_probe.rs index 4c605a87eb7..720f63414ce 100644 --- a/vortex-array/benches/scalar_fn_probe.rs +++ b/vortex-array/benches/scalar_fn_probe.rs @@ -118,7 +118,7 @@ fn probe_scalar_fn_valid_repeated(bencher: Bencher, array: &ArrayRef) { }) .bench_refs(|(probe, indices, ctx)| { for &index in indices.iter() { - black_box(probe.execute_is_invalid(index, ctx).unwrap()); + black_box(probe.execute_is_valid(index, ctx).unwrap()); } }); } diff --git a/vortex-array/src/expr/expression.rs b/vortex-array/src/expr/expression.rs index 93018dfb367..cd074823fcf 100644 --- a/vortex-array/src/expr/expression.rs +++ b/vortex-array/src/expr/expression.rs @@ -19,6 +19,9 @@ use crate::expr::display::DisplayTreeExpr; use crate::expr::traversal::TraversalOrder; use crate::expr::traversal::pre_order_visit_down; use crate::scalar_fn::EmptyOptions; +use crate::scalar_fn::ExpressionReduceNode; +use crate::scalar_fn::ReduceNode; +use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnRef; use crate::scalar_fn::ScalarFnVTable; use crate::scalar_fn::ScalarFnVTableExt; @@ -162,14 +165,24 @@ impl Expression { /// Returns a new expression representing the validity mask output of this expression. /// - /// The returned expression evaluates to a non-nullable boolean array. - pub fn validity(&self) -> VortexResult { + /// Returned expression evaluates to a non-nullable boolean array. + /// When scalar function's validity is irreducible, returns + /// "is_not_null(self)", which requires evaluating "self". + pub fn validity(&self, scope: &DType) -> VortexResult { match self { // The scope is exactly as valid as itself. Self::Root => Ok(Self::Root), - Self::Scalar { scalar_fn, .. } => Ok(scalar_fn - .validity(self)? - .unwrap_or_else(|| IsNotNull.new_expr(EmptyOptions, [self.clone()]))), + Self::Scalar { .. } => { + let node = ExpressionReduceNode::new(self, scope); + Ok(match node.validity()? { + ReduceNodeValidity::Reduced(reduced) => reduced.into_expression(), + // IsNotNull(x) -> x.validity() symbolic reduction rule works only + // when node's.validity is Reduced to avoid infinite recursion. + ReduceNodeValidity::Irreducible => { + IsNotNull.new_expr(EmptyOptions, [self.clone()]) + } + }) + } } } diff --git a/vortex-array/src/expr/mod.rs b/vortex-array/src/expr/mod.rs index b5634728589..e5d0227b155 100644 --- a/vortex-array/src/expr/mod.rs +++ b/vortex-array/src/expr/mod.rs @@ -105,7 +105,6 @@ pub use exprs::pack; pub use exprs::root; pub use exprs::select; pub use exprs::select_exclude; -pub use exprs::union_child_validities; pub use exprs::variant_get; pub use exprs::zip_expr; pub use scope::*; diff --git a/vortex-spatial/src/scalar_fn/collect.rs b/vortex-spatial/src/scalar_fn/collect.rs index 30523fa1124..5d133ef1f94 100644 --- a/vortex-spatial/src/scalar_fn/collect.rs +++ b/vortex-spatial/src/scalar_fn/collect.rs @@ -30,12 +30,9 @@ use vortex_array::scalar_fn::Arity; use vortex_array::scalar_fn::ChildName; use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::ExecutionArgs; -use vortex_array::scalar_fn::ReduceNode; -use vortex_array::scalar_fn::ReduceNodeValidity; use vortex_array::scalar_fn::ScalarFnId; use vortex_array::scalar_fn::ScalarFnVTable; use vortex_array::scalar_fn::TypedScalarFnInstance; -use vortex_array::scalar_fn::union_child_validities; use vortex_array::validity::Validity; use vortex_buffer::Buffer; use vortex_buffer::BufferMut; @@ -306,14 +303,6 @@ impl ScalarFnVTable for SpatialCollect { ) } - fn validity( - &self, - _: &Self::Options, - node: &T, - ) -> VortexResult> { - Ok(ReduceNodeValidity::Reduced(union_child_validities(node)?)) - } - fn is_strict(&self, _: &Self::Options) -> bool { true } diff --git a/vortex-spatial/src/scalar_fn/length.rs b/vortex-spatial/src/scalar_fn/length.rs index 3744d2d4b2f..3e9938c02af 100644 --- a/vortex-spatial/src/scalar_fn/length.rs +++ b/vortex-spatial/src/scalar_fn/length.rs @@ -24,12 +24,9 @@ use vortex_array::scalar_fn::Arity; use vortex_array::scalar_fn::ChildName; use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::ExecutionArgs; -use vortex_array::scalar_fn::ReduceNode; -use vortex_array::scalar_fn::ReduceNodeValidity; use vortex_array::scalar_fn::ScalarFnId; use vortex_array::scalar_fn::ScalarFnVTable; use vortex_array::scalar_fn::TypedScalarFnInstance; -use vortex_array::scalar_fn::union_child_validities; use vortex_array::validity::Validity; use vortex_buffer::Buffer; use vortex_error::VortexResult; @@ -206,14 +203,6 @@ impl ScalarFnVTable for SpatialLength { ) } - fn validity( - &self, - _: &Self::Options, - node: &T, - ) -> VortexResult> { - Ok(ReduceNodeValidity::Reduced(union_child_validities(node)?)) - } - fn is_strict(&self, _: &Self::Options) -> bool { true } diff --git a/vortex-spatial/src/scalar_fn/make_line.rs b/vortex-spatial/src/scalar_fn/make_line.rs index 52284a0c948..f860a4e5367 100644 --- a/vortex-spatial/src/scalar_fn/make_line.rs +++ b/vortex-spatial/src/scalar_fn/make_line.rs @@ -16,12 +16,9 @@ use vortex_array::scalar_fn::Arity; use vortex_array::scalar_fn::ChildName; use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::ExecutionArgs; -use vortex_array::scalar_fn::ReduceNode; -use vortex_array::scalar_fn::ReduceNodeValidity; use vortex_array::scalar_fn::ScalarFnId; use vortex_array::scalar_fn::ScalarFnVTable; use vortex_array::scalar_fn::TypedScalarFnInstance; -use vortex_array::scalar_fn::union_child_validities; use vortex_array::validity::Validity; use vortex_error::VortexResult; use vortex_error::vortex_ensure; @@ -224,14 +221,6 @@ impl ScalarFnVTable for SpatialMakeLine { ) } - fn validity( - &self, - _: &Self::Options, - node: &T, - ) -> VortexResult> { - Ok(ReduceNodeValidity::Reduced(union_child_validities(node)?)) - } - fn is_strict(&self, _: &Self::Options) -> bool { true } diff --git a/vortex-tensor/src/scalar_fns/l2_normalize.rs b/vortex-tensor/src/scalar_fns/l2_normalize.rs index 2e80c44f347..7e2bfebfa34 100644 --- a/vortex-tensor/src/scalar_fns/l2_normalize.rs +++ b/vortex-tensor/src/scalar_fns/l2_normalize.rs @@ -31,12 +31,9 @@ use vortex_array::scalar_fn::Arity; use vortex_array::scalar_fn::ChildName; use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::ExecutionArgs; -use vortex_array::scalar_fn::ReduceNode; -use vortex_array::scalar_fn::ReduceNodeValidity; use vortex_array::scalar_fn::ScalarFnId; use vortex_array::scalar_fn::ScalarFnVTable; use vortex_array::scalar_fn::ScalarFnVTableExt; -use vortex_array::scalar_fn::union_child_validities; use vortex_array::serde::ArrayChildren; use vortex_array::validity::Validity; use vortex_buffer::BufferMut; @@ -180,14 +177,6 @@ impl ScalarFnVTable for L2Normalize { }) } - fn validity( - &self, - _options: &Self::Options, - node: &T, - ) -> VortexResult> { - Ok(ReduceNodeValidity::Reduced(union_child_validities(node)?)) - } - fn is_strict(&self, _options: &Self::Options) -> bool { true } From 86cb6c13eca6e4b28810ed500cd88c09f6d1ba45 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 25 Sep 2026 15:48:51 +0100 Subject: [PATCH 03/22] better --- vortex-array/src/array/probe/repeated.rs | 14 +--- .../src/arrays/scalar_fn/vtable/mod.rs | 67 ++++--------------- .../src/arrays/scalar_fn/vtable/operations.rs | 2 +- 3 files changed, 16 insertions(+), 67 deletions(-) diff --git a/vortex-array/src/array/probe/repeated.rs b/vortex-array/src/array/probe/repeated.rs index 49ed2ff7fb7..8551bda19c6 100644 --- a/vortex-array/src/array/probe/repeated.rs +++ b/vortex-array/src/array/probe/repeated.rs @@ -56,17 +56,9 @@ impl RepeatedArrayProbe { /// Read the scalar at `index`, including its nullness, reusing retained preparation. pub fn execute_scalar(&mut self, index: usize, ctx: &mut ExecutionCtx) -> VortexResult { - // ScalarFn's validity is lazy, and for some functions evaluating - // validity is equal to evaluating the function. For such functions - // validity() is is_not_null(original array). So we get the chain: - // execute_scalar -> array.validity() -> - // execute_is_valid -> execute_scalar (mask) -> - // mask.probe_scalar_once -> scalar_at -> array.execute_scalar, and as - // "array" is the original array, we get infinite recursion. - // - // For these functions probe_scalar_once gets the nullable scalar anyway. - // - // See also execute_scala_once in probe/array.rs + // Probing validity of a lazy ScalarFn can recurse back into this scalar + // so we need to avoid calling probe_scalar_retained. See + // execute_scalar_once in probe/array.rs. if !self.array.is::() && !self.execute_is_valid(index, ctx)? { return Ok(Scalar::null(self.array.dtype().clone())); } diff --git a/vortex-array/src/arrays/scalar_fn/vtable/mod.rs b/vortex-array/src/arrays/scalar_fn/vtable/mod.rs index 909e4c79a5d..b467dc80004 100644 --- a/vortex-array/src/arrays/scalar_fn/vtable/mod.rs +++ b/vortex-array/src/arrays/scalar_fn/vtable/mod.rs @@ -25,25 +25,22 @@ use crate::array::ArrayView; use crate::array::VTable; use crate::array::ValidityVTable; use crate::array::with_empty_buffers; -use crate::arrays::StructArray; use crate::arrays::scalar_fn::array::ScalarFnArrayExt; use crate::arrays::scalar_fn::array::ScalarFnData; use crate::arrays::scalar_fn::rules::PARENT_RULES; use crate::arrays::scalar_fn::rules::RULES; use crate::buffer::BufferHandle; use crate::dtype::DType; -use crate::dtype::FieldName; use crate::executor::ExecutionCtx; use crate::executor::ExecutionResult; -use crate::expr::Expression; -use crate::expr::get_item; -use crate::expr::is_not_null; -use crate::expr::lit; -use crate::expr::root; use crate::matcher::Matcher; use crate::scalar_fn; +use crate::scalar_fn::ArrayReduceNode; +use crate::scalar_fn::ReduceNode; +use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::VecExecutionArgs; +use crate::scalar_fn::fns::is_not_null::IsNotNull; use crate::serde::ArrayChildren; use crate::validity::Validity; @@ -246,53 +243,13 @@ impl Deref for ScalarFnArrayView<'_, F> { impl ValidityVTable for ScalarFn { fn validity(view: ArrayView<'_, ScalarFn>) -> VortexResult { - // We want to defer execution of the underlying array. The naïve - // solution for this is to build an all true array and then .apply() an - // Expression referencing parts of root(). This doesn't work because - // in this Expression's evaluation root() is replaced by the original - // array which leads to non-terminating recursion, a stack overflow. So - // we build a Struct array and give the caller (which overrides - // ScalarFn valididy) the ability to reference children with get_item. - // In ScalarFn's overriden validity "expr.child(i)" then translates to - // "view.get_item(i)" which doesn't produce recursion since get_item - // references part of the original array as opposed to root(). - let child_count = view.child_count(); - let names = (0..child_count) - .map(|i| FieldName::from(i.to_string().as_str())) - .collect(); - let fields = view.children(); - - let getters: Vec<_> = view - .children() - .into_iter() - .enumerate() - .map(|(i, child)| { - if let Some(scalar) = child.as_constant() { - lit(scalar) - } else { - get_item(i.to_string(), root()) - } - }) - .collect(); - - let struct_array = StructArray::new(names, fields, view.len(), Validity::NonNullable); - - let scalar_fn = view.scalar_fn(); - let expr = Expression::try_new(scalar_fn.clone(), getters)?; - let expr = scalar_fn - .validity(&expr)? - // However, there is another possible stack overflow if validity() - // isn't overriden. The naïve solution is to do is_not_null(expr) - // which is is_not_null(ScalarFn(get_item(...))). - // Inner ScalarFn(get_item)'s row request will call validity() back - // which will instantiate is_not_null(F( original is_not_null )). - // - // So, to break this recursion, we need to tweak array probing for - // ScalarFn, see execute_scalar in probe/array.rs and in - // probe/repeated.rs - .unwrap_or_else(|| is_not_null(expr.clone())); - - let array = struct_array.into_array().apply(&expr)?; - Ok(Validity::Array(array)) + let node = ArrayReduceNode::new(view.as_ref()); + Ok(Validity::Array(match node.validity()? { + ReduceNodeValidity::Reduced(reduced) => reduced.into_array(), + // We get validity only after evaluating this node. To avoid + // infinite recursion, IsNotNull(x) -> x.validity() symbolically + // rewrites only in the Reduced() case. + ReduceNodeValidity::Irreducible => IsNotNull::new(view.as_ref().clone()).into_array(), + })) } } diff --git a/vortex-array/src/arrays/scalar_fn/vtable/operations.rs b/vortex-array/src/arrays/scalar_fn/vtable/operations.rs index cb246451bf4..dbd5a6922c0 100644 --- a/vortex-array/src/arrays/scalar_fn/vtable/operations.rs +++ b/vortex-array/src/arrays/scalar_fn/vtable/operations.rs @@ -258,7 +258,7 @@ mod tests { let rhs = PrimitiveArray::from_iter(0..10i64); let scalar_fn = TypedScalarFnInstance::new(Binary, Operator::Mul).erased(); - let args = vec![lhs.into_array(), rhs.into_array()]; + let args = vec![lhs, rhs.into_array()]; Ok(ScalarFnArray::try_new(scalar_fn, args)?.into_array()) } From 63359e1b2bc00c3683506231eee9556fdb02b2b1 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 25 Sep 2026 15:55:41 +0100 Subject: [PATCH 04/22] rewriting functions --- vortex-array/src/scalar_fn/fns/binary/mod.rs | 17 +------------ vortex-array/src/scalar_fn/fns/byte_length.rs | 9 ------- vortex-array/src/scalar_fn/fns/cast/mod.rs | 14 +++++++---- vortex-array/src/scalar_fn/fns/ext_storage.rs | 9 ------- .../src/scalar_fn/fns/fill_null/mod.rs | 13 ++++++---- vortex-array/src/scalar_fn/fns/like/mod.rs | 13 ---------- vortex-array/src/scalar_fn/fns/list_length.rs | 9 ------- vortex-array/src/scalar_fn/fns/literal.rs | 18 +++++++------- vortex-array/src/scalar_fn/fns/mask/mod.rs | 21 +++++++++------- vortex-array/src/scalar_fn/fns/merge.rs | 10 -------- vortex-array/src/scalar_fn/fns/pack.rs | 12 +++++----- vortex-array/src/scalar_fn/typed.rs | 24 ++++++++++++++++--- .../src/scalar_fn/unstable/row/vtable.rs | 13 +++++----- 13 files changed, 75 insertions(+), 107 deletions(-) diff --git a/vortex-array/src/scalar_fn/fns/binary/mod.rs b/vortex-array/src/scalar_fn/fns/binary/mod.rs index 2883859b654..c86f6141460 100644 --- a/vortex-array/src/scalar_fn/fns/binary/mod.rs +++ b/vortex-array/src/scalar_fn/fns/binary/mod.rs @@ -20,7 +20,6 @@ use crate::arrays::ScalarFnArray; use crate::dtype::DType; use crate::dtype::Nullability; use crate::expr::BoundExpression; -use crate::expr::and; use crate::expr::bound; use crate::expr::display::ExprDisplay; use crate::expr::expression::Expression; @@ -262,19 +261,6 @@ impl ScalarFnVTable for Binary { }) } - fn validity( - &self, - operator: &Operator, - expression: &Expression, - ) -> VortexResult> { - if matches!(operator, Operator::And | Operator::Or) { - return Ok(None); // AND and OR are kleene logic - } - let lhs = expression.child(0).validity()?; - let rhs = expression.child(1).validity()?; - Ok(Some(and(lhs, rhs))) - } - fn reduce(&self, operator: &Operator, node: &T) -> VortexResult> { if !matches!(operator, Operator::And | Operator::Or) { return Ok(None); @@ -307,8 +293,6 @@ impl ScalarFnVTable for Binary { } fn is_strict(&self, operator: &Operator) -> bool { - // Kleene AND/OR is not strict (`false AND null = false`, `true OR null = true`), which is - // consistent with `validity` returning `None` for these operators above. !matches!(operator, Operator::And | Operator::Or) } @@ -350,6 +334,7 @@ mod tests { use crate::dtype::Nullability; use crate::dtype::PType; use crate::expr::Expression; + use crate::expr::and; use crate::expr::and_collect; use crate::expr::col; use crate::expr::eq; diff --git a/vortex-array/src/scalar_fn/fns/byte_length.rs b/vortex-array/src/scalar_fn/fns/byte_length.rs index cc709628bad..91c463c519a 100644 --- a/vortex-array/src/scalar_fn/fns/byte_length.rs +++ b/vortex-array/src/scalar_fn/fns/byte_length.rs @@ -24,7 +24,6 @@ use crate::arrays::varbinview::VarBinViewArrayExt; use crate::dtype::DType; use crate::dtype::Nullability; use crate::dtype::PType; -use crate::expr::Expression; use crate::kernel::ExecuteParentKernel; use crate::scalar::Scalar; use crate::scalar_fn::Arity; @@ -123,14 +122,6 @@ impl ScalarFnVTable for ByteLength { } } - fn validity( - &self, - _: &Self::Options, - expression: &Expression, - ) -> VortexResult> { - Ok(Some(expression.child(0).validity()?)) - } - fn is_strict(&self, _options: &Self::Options) -> bool { true } diff --git a/vortex-array/src/scalar_fn/fns/cast/mod.rs b/vortex-array/src/scalar_fn/fns/cast/mod.rs index ab6f88161b0..4d4c307f6be 100644 --- a/vortex-array/src/scalar_fn/fns/cast/mod.rs +++ b/vortex-array/src/scalar_fn/fns/cast/mod.rs @@ -45,10 +45,12 @@ use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; use crate::scalar_fn::ExecutionArgs; use crate::scalar_fn::ReduceNode; +use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; use crate::scalar_fn::ScalarFnVTableExt; use crate::scalar_fn::fns::literal::Literal; +use crate::scalar_fn::is_not_null_node; /// A cast expression that converts values to a target data type. #[derive(Clone)] @@ -178,11 +180,15 @@ impl ScalarFnVTable for Cast { Ok(scalar.cast(target_dtype).ok().map(bound::lit)) } - fn validity(&self, dtype: &DType, expression: &Expression) -> VortexResult> { - Ok(Some(if dtype.is_nullable() { - expression.child(0).validity()? + fn validity( + &self, + dtype: &DType, + node: &T, + ) -> VortexResult> { + Ok(ReduceNodeValidity::Reduced(if dtype.is_nullable() { + is_not_null_node(&node.child(0))? } else { - lit(true) + node.new_constant(true.into()) })) } diff --git a/vortex-array/src/scalar_fn/fns/ext_storage.rs b/vortex-array/src/scalar_fn/fns/ext_storage.rs index 3d7de98aca8..ad8e77e9fa7 100644 --- a/vortex-array/src/scalar_fn/fns/ext_storage.rs +++ b/vortex-array/src/scalar_fn/fns/ext_storage.rs @@ -13,7 +13,6 @@ use crate::arrays::ConstantArray; use crate::arrays::ExtensionArray; use crate::arrays::extension::ExtensionArrayExt; use crate::dtype::DType; -use crate::expr::Expression; use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; use crate::scalar_fn::EmptyOptions; @@ -85,14 +84,6 @@ impl ScalarFnVTable for ExtStorage { Ok(input.storage_array().clone()) } - fn validity( - &self, - _options: &Self::Options, - expression: &Expression, - ) -> VortexResult> { - Ok(Some(expression.child(0).validity()?)) - } - fn is_strict(&self, _options: &Self::Options) -> bool { true } diff --git a/vortex-array/src/scalar_fn/fns/fill_null/mod.rs b/vortex-array/src/scalar_fn/fns/fill_null/mod.rs index 181e2fd6e95..cdc642d9404 100644 --- a/vortex-array/src/scalar_fn/fns/fill_null/mod.rs +++ b/vortex-array/src/scalar_fn/fns/fill_null/mod.rs @@ -29,9 +29,12 @@ use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; use crate::scalar_fn::EmptyOptions; use crate::scalar_fn::ExecutionArgs; +use crate::scalar_fn::ReduceNode; +use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; use crate::scalar_fn::ScalarFnVTableExt; +use crate::scalar_fn::is_not_null_node; /// An expression that replaces null values in the input with a fill value. #[derive(Clone)] @@ -133,14 +136,16 @@ impl ScalarFnVTable for FillNull { Ok(None) } - fn validity( + fn validity( &self, _options: &Self::Options, - expression: &Expression, - ) -> VortexResult> { + node: &T, + ) -> VortexResult> { // After fill_null, the result validity depends on the fill value's nullability. // If fill_value is non-nullable, the result is always valid. - Ok(Some(expression.child(1).validity()?)) + Ok(ReduceNodeValidity::Reduced(is_not_null_node( + &node.child(1), + )?)) } fn is_strict(&self, _options: &Self::Options) -> bool { diff --git a/vortex-array/src/scalar_fn/fns/like/mod.rs b/vortex-array/src/scalar_fn/fns/like/mod.rs index beb5a554fa3..17885c3cd55 100644 --- a/vortex-array/src/scalar_fn/fns/like/mod.rs +++ b/vortex-array/src/scalar_fn/fns/like/mod.rs @@ -31,8 +31,6 @@ use crate::arrays::VarBinViewArray; use crate::arrays::varbinview::BinaryView; use crate::dtype::DType; use crate::dtype::Nullability; -use crate::expr::Expression; -use crate::expr::and; use crate::expr::display::ExprDisplay; use crate::proto::expr as pb; use crate::scalar::Scalar; @@ -173,17 +171,6 @@ impl ScalarFnVTable for Like { execute_like(&child, &pattern, *options, ctx) } - fn validity( - &self, - _options: &Self::Options, - expression: &Expression, - ) -> VortexResult> { - tracing::warn!("Computing validity for LIKE expression"); - let child_validity = expression.child(0).validity()?; - let pattern_validity = expression.child(1).validity()?; - Ok(Some(and(child_validity, pattern_validity))) - } - fn is_strict(&self, _instance: &Self::Options) -> bool { true } diff --git a/vortex-array/src/scalar_fn/fns/list_length.rs b/vortex-array/src/scalar_fn/fns/list_length.rs index 058cefd1c16..afb03b69c1b 100644 --- a/vortex-array/src/scalar_fn/fns/list_length.rs +++ b/vortex-array/src/scalar_fn/fns/list_length.rs @@ -24,7 +24,6 @@ use crate::builtins::ArrayBuiltins; use crate::dtype::DType; use crate::dtype::Nullability; use crate::dtype::PType; -use crate::expr::Expression; use crate::matcher::Matcher; use crate::scalar::Scalar; use crate::scalar_fn::Arity; @@ -100,14 +99,6 @@ impl ScalarFnVTable for ListLength { list_length(&input, nullability, ctx) } - fn validity( - &self, - _: &Self::Options, - expression: &Expression, - ) -> VortexResult> { - Ok(Some(expression.child(0).validity()?)) - } - fn is_strict(&self, _options: &Self::Options) -> bool { // A null list has a null length, and the length of a valid list is a non-null value // determined by that list alone, with `return_dtype` carrying over the input nullability. diff --git a/vortex-array/src/scalar_fn/fns/literal.rs b/vortex-array/src/scalar_fn/fns/literal.rs index 33507ecef0d..cb6c3284523 100644 --- a/vortex-array/src/scalar_fn/fns/literal.rs +++ b/vortex-array/src/scalar_fn/fns/literal.rs @@ -14,20 +14,16 @@ use crate::ExecutionCtx; use crate::IntoArray; use crate::arrays::ConstantArray; use crate::dtype::DType; -use crate::expr::Expression; use crate::expr::display::ExprDisplay; use crate::proto::expr as pb; use crate::scalar::Scalar; use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; use crate::scalar_fn::ExecutionArgs; +use crate::scalar_fn::ReduceNode; +use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; -use crate::scalar_fn::ScalarFnVTableExt; - -fn lit(value: impl Into) -> Expression { - Literal.new_expr(value.into(), []) -} /// Expression that represents a literal scalar value. #[derive(Clone)] @@ -94,12 +90,14 @@ impl ScalarFnVTable for Literal { Ok(ConstantArray::new(scalar.clone(), args.row_count()).into_array()) } - fn validity( + fn validity( &self, scalar: &Scalar, - _expression: &Expression, - ) -> VortexResult> { - Ok(Some(lit(scalar.is_valid()))) + node: &T, + ) -> VortexResult> { + Ok(ReduceNodeValidity::Reduced( + node.new_constant(scalar.is_valid().into()), + )) } fn is_strict(&self, _instance: &Self::Options) -> bool { diff --git a/vortex-array/src/scalar_fn/fns/mask/mod.rs b/vortex-array/src/scalar_fn/fns/mask/mod.rs index 401a855692c..0d3f953946c 100644 --- a/vortex-array/src/scalar_fn/fns/mask/mod.rs +++ b/vortex-array/src/scalar_fn/fns/mask/mod.rs @@ -24,17 +24,21 @@ use crate::dtype::DType; use crate::dtype::Nullability; use crate::expr::BoundExpression; use crate::expr::Expression; -use crate::expr::and; use crate::expr::bound; use crate::scalar::Scalar; use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; use crate::scalar_fn::EmptyOptions; use crate::scalar_fn::ExecutionArgs; +use crate::scalar_fn::ReduceNode; +use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; use crate::scalar_fn::ScalarFnVTableExt; +use crate::scalar_fn::fns::binary::Binary; use crate::scalar_fn::fns::literal::Literal; +use crate::scalar_fn::fns::operators::Operator; +use crate::scalar_fn::is_not_null_node; /// An expression that masks an input based on a boolean mask. /// @@ -136,15 +140,16 @@ impl ScalarFnVTable for Mask { } } - fn validity( + fn validity( &self, _options: &Self::Options, - expression: &Expression, - ) -> VortexResult> { - Ok(Some(and( - expression.child(0).validity()?, - expression.child(1).clone(), - ))) + node: &T, + ) -> VortexResult> { + let input_validity = is_not_null_node(&node.child(0))?; + Ok(ReduceNodeValidity::Reduced(input_validity.new_node( + Binary.bind(Operator::And), + &[input_validity.clone(), node.child(1)], + )?)) } fn is_strict(&self, _options: &Self::Options) -> bool { diff --git a/vortex-array/src/scalar_fn/fns/merge.rs b/vortex-array/src/scalar_fn/fns/merge.rs index f1c9746e4a2..42f6dc55949 100644 --- a/vortex-array/src/scalar_fn/fns/merge.rs +++ b/vortex-array/src/scalar_fn/fns/merge.rs @@ -23,8 +23,6 @@ use crate::dtype::DType; use crate::dtype::FieldNames; use crate::dtype::Nullability; use crate::dtype::StructFields; -use crate::expr::Expression; -use crate::expr::lit; use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; use crate::scalar_fn::ExecutionArgs; @@ -226,14 +224,6 @@ impl ScalarFnVTable for Merge { Ok(Some(pack_expr)) } - fn validity( - &self, - _options: &Self::Options, - _expression: &Expression, - ) -> VortexResult> { - Ok(Some(lit(true))) - } - fn is_strict(&self, _options: &Self::Options) -> bool { true } diff --git a/vortex-array/src/scalar_fn/fns/pack.rs b/vortex-array/src/scalar_fn/fns/pack.rs index c081b7e6ea5..12775042b45 100644 --- a/vortex-array/src/scalar_fn/fns/pack.rs +++ b/vortex-array/src/scalar_fn/fns/pack.rs @@ -21,13 +21,13 @@ use crate::dtype::FieldName; use crate::dtype::FieldNames; use crate::dtype::Nullability; use crate::dtype::StructFields; -use crate::expr::Expression; use crate::expr::display::ExprDisplay; -use crate::expr::lit; use crate::proto::expr as pb; use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; use crate::scalar_fn::ExecutionArgs; +use crate::scalar_fn::ReduceNode; +use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; use crate::validity::Validity; @@ -127,12 +127,12 @@ impl ScalarFnVTable for Pack { )) } - fn validity( + fn validity( &self, _options: &Self::Options, - _expression: &Expression, - ) -> VortexResult> { - Ok(Some(lit(true))) + node: &T, + ) -> VortexResult> { + Ok(ReduceNodeValidity::Reduced(node.new_constant(true.into()))) } fn execute( diff --git a/vortex-array/src/scalar_fn/typed.rs b/vortex-array/src/scalar_fn/typed.rs index 7061e21ffd7..5f7b53bb138 100644 --- a/vortex-array/src/scalar_fn/typed.rs +++ b/vortex-array/src/scalar_fn/typed.rs @@ -31,6 +31,7 @@ use crate::scalar_fn::ArrayReduceNode; use crate::scalar_fn::ChildName; use crate::scalar_fn::ExecutionArgs; use crate::scalar_fn::ExpressionReduceNode; +use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnRef; use crate::scalar_fn::ScalarFnVTable; @@ -97,7 +98,14 @@ pub(super) trait DynScalarFn: 'static + Send + Sync + super::sealed::Sealed { // Expression methods — take expressions for tree traversal fn fmt_sql(&self, expression: &dyn ExprDisplay, f: &mut Formatter<'_>) -> fmt::Result; fn simplify(&self, expression: &BoundExpression) -> VortexResult>; - fn validity(&self, expression: &Expression) -> VortexResult>; + fn validity_expression<'a>( + &self, + node: &ExpressionReduceNode<'a>, + ) -> VortexResult>>; + fn validity_array<'a>( + &self, + node: &ArrayReduceNode<'a>, + ) -> VortexResult>>; // Options operations — self-contained fn options_serialize(&self) -> VortexResult>>; @@ -201,8 +209,18 @@ impl DynScalarFn for TypedScalarFnInstance { V::simplify(&self.vtable, &self.options, expression) } - fn validity(&self, expression: &Expression) -> VortexResult> { - V::validity(&self.vtable, &self.options, expression) + fn validity_expression<'a>( + &self, + node: &ExpressionReduceNode<'a>, + ) -> VortexResult>> { + V::validity(&self.vtable, &self.options, node) + } + + fn validity_array<'a>( + &self, + node: &ArrayReduceNode<'a>, + ) -> VortexResult>> { + V::validity(&self.vtable, &self.options, node) } fn options_serialize(&self) -> VortexResult>> { diff --git a/vortex-array/src/scalar_fn/unstable/row/vtable.rs b/vortex-array/src/scalar_fn/unstable/row/vtable.rs index 028bf536dde..b68fd24c164 100644 --- a/vortex-array/src/scalar_fn/unstable/row/vtable.rs +++ b/vortex-array/src/scalar_fn/unstable/row/vtable.rs @@ -25,13 +25,14 @@ use super::visitor::ExecuteValidRows; use crate::ArrayRef; use crate::ExecutionCtx; use crate::dtype::DType; -use crate::expr::Expression; -use crate::expr::union_child_validities; use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; use crate::scalar_fn::ExecutionArgs; +use crate::scalar_fn::ReduceNode; +use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; +use crate::scalar_fn::union_child_validities; use crate::scalar_fn::unstable::row::execute::DenseAttempt; impl ScalarFnVTable for F { @@ -70,12 +71,12 @@ impl ScalarFnVTable for F { execute_rows(self, options, args, ctx) } - fn validity( + fn validity( &self, _options: &Self::Options, - expression: &Expression, - ) -> VortexResult> { - union_child_validities(expression) + node: &T, + ) -> VortexResult> { + Ok(ReduceNodeValidity::Reduced(union_child_validities(node)?)) } // `RowFn` is stricter than `ScalarFnVTable::is_strict`: its kernel cannot produce null from From 235abc7d58805fe30549e4f5617b8d017de67c5c Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 25 Sep 2026 16:14:12 +0100 Subject: [PATCH 05/22] better --- vortex-array/src/scalar_fn/erased.rs | 19 ++++- vortex-array/src/scalar_fn/vtable.rs | 123 +++++++++++++++++++++++---- 2 files changed, 123 insertions(+), 19 deletions(-) diff --git a/vortex-array/src/scalar_fn/erased.rs b/vortex-array/src/scalar_fn/erased.rs index 25b2981ced9..8ae96d4d49a 100644 --- a/vortex-array/src/scalar_fn/erased.rs +++ b/vortex-array/src/scalar_fn/erased.rs @@ -25,6 +25,7 @@ use crate::expr::display::ExprDisplay; use crate::scalar_fn::ArrayReduceNode; use crate::scalar_fn::ExecutionArgs; use crate::scalar_fn::ExpressionReduceNode; +use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; use crate::scalar_fn::options::ScalarFnOptions; @@ -122,10 +123,20 @@ impl ScalarFnRef { self.0.return_dtype(arg_types) } - /// Some(E) if evaluating validity for this function is faster than - /// evaluating the function itself, None otherwise. - pub fn validity(&self, expr: &Expression) -> VortexResult> { - self.0.validity(expr) + /// Symbolic validity of this node in an expression tree + pub(crate) fn validity_expression<'a>( + &self, + node: &ExpressionReduceNode<'a>, + ) -> VortexResult>> { + self.0.validity_expression(node) + } + + /// Symbolic validity of this in an array tree + pub(crate) fn validity_array<'a>( + &self, + node: &ArrayReduceNode<'a>, + ) -> VortexResult>> { + self.0.validity_array(node) } /// Execute the expression given the input arguments. diff --git a/vortex-array/src/scalar_fn/vtable.rs b/vortex-array/src/scalar_fn/vtable.rs index 2479f9b49d4..5361d0b8774 100644 --- a/vortex-array/src/scalar_fn/vtable.rs +++ b/vortex-array/src/scalar_fn/vtable.rs @@ -14,6 +14,7 @@ use vortex_error::VortexResult; use vortex_error::vortex_bail; use vortex_error::vortex_err; use vortex_session::VortexSession; +use vortex_utils::iter::ReduceBalancedIterExt; use crate::ArrayRef; use crate::ExecutionCtx; @@ -30,7 +31,53 @@ use crate::scalar::Scalar; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnRef; use crate::scalar_fn::TypedScalarFnInstance; +use crate::scalar_fn::fns::binary::Binary; +use crate::scalar_fn::fns::is_not_null::IsNotNull; use crate::scalar_fn::fns::literal::Literal; +use crate::scalar_fn::fns::operators::Operator; +use crate::validity::Validity; + +// Here and beyond we use validity(x) interchangeably with is_not_null(x). +#[derive(Clone)] +pub enum ReduceNodeValidity { + /// Validity of T can be symbolically reduced (i.e. without evaluating T) + /// to a function over validities of this node's children. For an + /// expression reduce node, one example is byte_length(x). + /// validity(byte_length(x)) can be symbolically reduced to validity(x) + /// since byte_length doesn't change validity. + Reduced(T), + /// Validity of T can't be symbolically reduced to anything, and all + /// further reductions require evaluating T first. For an expression reduce + /// node, one example is list_contains(x, C) since you can't reduce + /// validity(list_contains(x, C)). list_contains([], null) is false, but to + /// know that, you need to evaluate x's offsets. + /// + /// This is also the default case. + Irreducible, +} + +/// IsNotNull(child) as a reducible node +pub fn is_not_null_node(child: &T) -> VortexResult { + child.new_node(IsNotNull.bind(EmptyOptions), std::slice::from_ref(child)) +} + +/// "And" over "node's" non-nullable children +pub fn union_child_validities(node: &T) -> VortexResult { + let mut parts = Vec::with_capacity(node.child_count()); + for i in 0..node.child_count() { + let child = node.child(i); + if child.node_dtype()?.is_nullable() { + parts.push(is_not_null_node(&child)?); + } + } + let parts = parts + .into_iter() + .try_reduce_balanced(|lhs, rhs| { + lhs.new_node(Binary.bind(Operator::And), &[lhs.clone(), rhs]) + })? + .unwrap_or_else(|| node.new_constant(true.into())); + Ok(parts) +} /// This trait defines the interface for scalar function vtables, including methods for /// serialization, deserialization, validation, child naming, return type computation, @@ -142,6 +189,38 @@ pub trait ScalarFnVTable: 'static + Sized + Clone + Send + Sync { Ok(None) } + /// For node, returns node' which is exactly the result of evaluating + /// validity(node). Returned node' is either a lazy computation over + /// children of node, a constant, or Irreducible which means you need to + /// evaluate node to get its validity. + fn validity( + &self, + options: &Self::Options, + node: &T, + ) -> VortexResult> { + if !self.is_strict(options) { + return Ok(ReduceNodeValidity::Irreducible); + } + + let mut dtypes = Vec::with_capacity(node.child_count()); + for i in 0..node.child_count() { + let dtype = node.child(i).node_dtype()?; + if matches!(dtype, DType::Null) { + return Ok(ReduceNodeValidity::Irreducible); + } + dtypes.push(dtype.as_nonnullable()); + } + + let res = if let Ok(dtype) = self.return_dtype(options, &dtypes) + && !dtype.is_nullable() + { + ReduceNodeValidity::Reduced(union_child_validities(node)?) + } else { + ReduceNodeValidity::Irreducible + }; + Ok(res) + } + /// Simplify the bound expression if possible. /// /// Every node of `expr` carries its dtype, so rules read types directly from the tree. @@ -155,21 +234,6 @@ pub trait ScalarFnVTable: 'static + Sized + Clone + Send + Sync { Ok(None) } - /// Returns an expression that evaluates to the validity of the result of this expression. - /// - /// If a validity expression cannot be constructed, returns `None` and the expression will - /// be evaluated as normal before extracting the validity mask from the result. - /// - /// This is essentially a specialized form of a `reduce_parent` - fn validity( - &self, - options: &Self::Options, - expression: &Expression, - ) -> VortexResult> { - _ = (options, expression); - Ok(None) - } - /// Returns whether this scalar function is strict. /// /// A strict function returns null for a row when any argument is null for that row. This @@ -246,6 +310,13 @@ pub trait ReduceNode: Clone { /// Produce a new constant node in the same scope as "self" fn new_constant(&self, value: Scalar) -> Self; + + /// Symbolic validity of this node. Reduced() if you can get from node's + /// validity to validity of its children or a constant without evaluating + /// node. + fn validity(&self) -> VortexResult> + where + Self: Sized; } /// A [`ReduceNode`] over a bound expression tree. @@ -313,6 +384,13 @@ impl ReduceNode for ExpressionReduceNode<'_> { expression: Cow::Owned(bound::lit(value)), } } + + fn validity(&self) -> VortexResult> { + match self.expression.as_scalar() { + Some(scalar_fn) => scalar_fn.validity_expression(self), + None => Ok(ReduceNodeValidity::Irreducible), + } + } } /// A [`ReduceNode`] over an array tree. @@ -393,6 +471,21 @@ impl ReduceNode for ArrayReduceNode<'_> { array: Cow::Owned(array.into_array()), } } + + fn validity(&self) -> VortexResult> { + if let Some(scalar_fn) = self.array.as_opt::() { + return scalar_fn.data().scalar_fn().validity_array(self); + } + Ok(ReduceNodeValidity::Reduced( + match self.array.validity()? { + Validity::NonNullable | Validity::AllValid => self.new_constant(true.into()), + Validity::AllInvalid => self.new_constant(false.into()), + Validity::Array(array) => Self { + array: Cow::Owned(array), + }, + }, + )) + } } /// The arity (number of arguments) of a function. From 2d6a994eab55b2cced362ec5d080ec71f8a3e28a Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 25 Sep 2026 16:24:23 +0100 Subject: [PATCH 06/22] better --- vortex-array/src/scalar_fn/fns/between/mod.rs | 42 ++++++++++++------- vortex-array/src/scalar_fn/fns/is_null.rs | 10 ++++- 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index e1ae042c3ef..717074eaeb5 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -25,7 +25,6 @@ use crate::builtins::ArrayBuiltins; use crate::dtype::DType; use crate::dtype::DType::Bool; use crate::expr::display::ExprDisplay; -use crate::expr::expression::Expression; use crate::proto::expr as pb; use crate::scalar::Scalar; use crate::scalar_fn::Arity; @@ -327,21 +326,8 @@ impl ScalarFnVTable for Between { between_canonical(&arr, &lower, &upper, options, ctx) } - fn validity( - &self, - _options: &Self::Options, - _expression: &Expression, - ) -> VortexResult> { - // `Between` stands for two compares under Kleene `AND`, and `null AND false` is `false`, - // so a null bound does not make a row null. There is no validity expression to derive, - // which is also why `Binary` returns `None` for `Operator::And`. - Ok(None) - } - fn is_strict(&self, _options: &Self::Options) -> bool { - // Not strict for the same reason `validity` returns `None` above: under Kleene `AND` a - // null bound does not force a null row. - false + false // NULL AND false = false } fn is_infallible(&self, _options: &Self::Options) -> bool { @@ -355,6 +341,7 @@ mod tests { use rstest::rstest; use vortex_buffer::buffer; + use vortex_error::vortex_err; use super::*; use crate::IntoArray; @@ -362,6 +349,7 @@ mod tests { use crate::arrays::BoolArray; use crate::arrays::DecimalArray; use crate::arrays::PrimitiveArray; + use crate::arrays::ScalarFn; use crate::arrays::StructArray; use crate::assert_arrays_eq; use crate::dtype::DType; @@ -375,6 +363,7 @@ mod tests { use crate::expr::root; use crate::scalar::DecimalValue; use crate::scalar::Scalar; + use crate::scalar_fn::fns::is_not_null::IsNotNull; use crate::test_harness::to_int_indices; use crate::validity::Validity; @@ -391,6 +380,26 @@ mod tests { ConstantArray::new(null, len).into_array() } + #[test] + fn lazy_validity() -> VortexResult<()> { + let ctx = &mut SESSION.create_execution_ctx(); + + let x = PrimitiveArray::from_option_iter([Some(10), Some(10), Some(1)]).into_array(); + let lo = PrimitiveArray::from_option_iter([None, None, Some(0)]).into_array(); + let hi = PrimitiveArray::from_option_iter([Some(5), Some(50), Some(5)]).into_array(); + + let data = StructArray::from_fields(&[("x", x), ("lo", lo), ("hi", hi)])?.into_array(); + let lazy = data.apply(&between(col("x"), col("lo"), col("hi"), NON_STRICT))?; + + let Validity::Array(validity) = lazy.validity()? else { + vortex_bail!("non-lazy validity"); + }; + let scalar_fn = validity.as_::(); + assert!(scalar_fn.scalar_fn().is::()); + assert_arrays_eq!(validity, BoolArray::from_iter([true, false, true]), ctx); + Ok(()) + } + /// A declared validity expression must agree with the mask of the executed result. /// /// The bounds are columns rather than literals so that a null bound reaches execution @@ -416,8 +425,9 @@ mod tests { .execute::(ctx)? .opt_bool_vec(ctx); + let validity_expr = expr.validity(data.dtype())?; let declared = data - .apply(&expr.validity()?)? + .apply(&validity_expr)? .execute::(ctx)? .bool_vec(ctx); diff --git a/vortex-array/src/scalar_fn/fns/is_null.rs b/vortex-array/src/scalar_fn/fns/is_null.rs index 6242ad48394..e8f21a6293c 100644 --- a/vortex-array/src/scalar_fn/fns/is_null.rs +++ b/vortex-array/src/scalar_fn/fns/is_null.rs @@ -18,9 +18,12 @@ use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; use crate::scalar_fn::EmptyOptions; use crate::scalar_fn::ExecutionArgs; +use crate::scalar_fn::ReduceNode; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; use crate::scalar_fn::ScalarFnVTableExt; +use crate::scalar_fn::fns::is_not_null::lazy_child_or_execute_step; +use crate::scalar_fn::fns::is_not_null::reduce_null; use crate::validity::Validity; /// Expression that checks for null values. @@ -75,13 +78,14 @@ impl ScalarFnVTable for IsNull { &self, _data: &Self::Options, args: &dyn ExecutionArgs, - _ctx: &mut ExecutionCtx, + ctx: &mut ExecutionCtx, ) -> VortexResult { let child = args.get(0)?; if let Some(scalar) = child.as_constant() { return Ok(ConstantArray::new(scalar.is_null(), args.row_count()).into_array()); } + let child = lazy_child_or_execute_step(child, ctx)?; match child.validity()? { Validity::NonNullable | Validity::AllValid => { Ok(ConstantArray::new(false, args.row_count()).into_array()) @@ -91,6 +95,10 @@ impl ScalarFnVTable for IsNull { } } + fn reduce(&self, _options: &Self::Options, node: &T) -> VortexResult> { + reduce_null(true, node) + } + fn is_strict(&self, _instance: &Self::Options) -> bool { // Null input produces the non-null boolean value `true`. false From 01b339214e0e4441d9d931761b1365d28ff8e03b Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 25 Sep 2026 16:42:11 +0100 Subject: [PATCH 07/22] better --- vortex-array/src/scalar_fn/fns/is_not_null.rs | 149 +++++++++++++++++- 1 file changed, 147 insertions(+), 2 deletions(-) diff --git a/vortex-array/src/scalar_fn/fns/is_not_null.rs b/vortex-array/src/scalar_fn/fns/is_not_null.rs index b6a2dcd372f..1fa8e0bb420 100644 --- a/vortex-array/src/scalar_fn/fns/is_not_null.rs +++ b/vortex-array/src/scalar_fn/fns/is_not_null.rs @@ -13,6 +13,7 @@ use crate::ArrayRef; use crate::ExecutionCtx; use crate::IntoArray; use crate::arrays::ConstantArray; +use crate::arrays::ScalarFn; use crate::arrays::ScalarFnArray; use crate::dtype::DType; use crate::dtype::Nullability; @@ -21,11 +22,107 @@ use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; use crate::scalar_fn::EmptyOptions; use crate::scalar_fn::ExecutionArgs; +use crate::scalar_fn::ReduceNode; +use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; use crate::scalar_fn::ScalarFnVTableExt; +use crate::scalar_fn::fns::binary::Binary; +use crate::scalar_fn::fns::is_null::IsNull; +use crate::scalar_fn::fns::not::Not; +use crate::scalar_fn::fns::operators::Operator; +use crate::scalar_fn::is_not_null_node; use crate::validity::Validity; +/// In array context, reduce +/// +/// IsNull(x) -> if !x.nullable lit(false) else not(x.validity()) +/// IsNotNull(x) -> if !x.nullable lit(true) or x.validity() +/// +/// In expression and array contexts for x, y where x is nullable but y is not, +/// reduce +/// +/// 1. IsNull(and(x, y)) -> and(IsNull(x), y) +/// 2. IsNull(or(x, y)) -> and(IsNull(x), not(y)) +/// 3. IsNotNull(and(x, y)) -> or(IsNotNull(x), not(y)) +/// 4. IsNotNull(or(x, y)) -> or(IsNotNull(x), y) +/// +/// Latter optimizations make sense because calculating IsNull(x)/IsNotNull(x) +/// is at most expensive as calculating x, but usually much cheaper. Although +/// in two cases you exchange 4 computations to 4 computations, the latter +/// four are cheaper. +pub(crate) fn reduce_null(is_null: bool, node: &T) -> VortexResult> { + let child = node.child(0); + if !child.node_dtype()?.is_nullable() { + return Ok(Some(node.new_constant((!is_null).into()))); + } + + // Irreducible's validity is IsNotNull(self), if we rewrite it here, we'll + // get into infinite recursion + if let ReduceNodeValidity::Reduced(validity) = child.validity()? { + return Ok(Some(if is_null { + validity.new_node(Not.bind(EmptyOptions), std::slice::from_ref(&validity))? + } else { + validity + })); + } + + let Some(child_fn) = child.scalar_fn() else { + return Ok(None); + }; + let Some(operator) = child_fn.as_opt::() else { + return Ok(None); + }; + let is_and = match operator { + Operator::And => true, + Operator::Or => false, + _ => return Ok(None), + }; + + let left = child.child(0); + let right = child.child(1); + let (nullable, non_nullable) = match ( + left.node_dtype()?.is_nullable(), + right.node_dtype()?.is_nullable(), + ) { + (true, false) => (left, right), + (false, true) => (right, left), + // (false, false) already rewritten by constant folding + _ => return Ok(None), + }; + + // is_null(nullable) (rule 1, 2) + // is_not_null(nullable) (3, 4) + let left = if is_null { + nullable.new_node(IsNull.bind(EmptyOptions), std::slice::from_ref(&nullable))? + } else { + is_not_null_node(&nullable)? + }; + // non_nullable (1, 4) + // not(non_nullable) (2, 3) + let right = if is_null == is_and { + non_nullable + } else { + non_nullable.new_node(Not.bind(EmptyOptions), std::slice::from_ref(&non_nullable))? + }; + let combine = if is_null { Operator::And } else { Operator::Or }; + Ok(Some(node.new_node(Binary.bind(combine), &[left, right])?)) +} + +/// If we get a scalar function child, at this point we have done all symbolic +/// reductions. Execute child one step and return. For every other child, +/// return the child itself. +pub(crate) fn lazy_child_or_execute_step( + child: ArrayRef, + ctx: &mut ExecutionCtx, +) -> VortexResult { + if child.is::() { + child.execute::(ctx) + } else { + Ok(child) + } +} + /// Expression that checks for non-null values. #[derive(Clone)] pub struct IsNotNull; @@ -89,9 +186,9 @@ impl ScalarFnVTable for IsNotNull { &self, _data: &Self::Options, args: &dyn ExecutionArgs, - _ctx: &mut ExecutionCtx, + ctx: &mut ExecutionCtx, ) -> VortexResult { - let child = args.get(0)?; + let child = lazy_child_or_execute_step(args.get(0)?, ctx)?; match child.validity()? { Validity::NonNullable | Validity::AllValid => { Ok(ConstantArray::new(true, args.row_count()).into_array()) @@ -101,6 +198,10 @@ impl ScalarFnVTable for IsNotNull { } } + fn reduce(&self, _options: &Self::Options, node: &T) -> VortexResult> { + reduce_null(false, node) + } + fn is_strict(&self, _instance: &Self::Options) -> bool { // Null input produces the non-null boolean value `false`. false @@ -127,10 +228,15 @@ mod tests { use crate::arrays::StructArray; use crate::dtype::DType; use crate::dtype::Nullability; + use crate::dtype::StructFields; + use crate::expr::and; use crate::expr::col; use crate::expr::eq; use crate::expr::get_item; use crate::expr::is_not_null; + use crate::expr::is_null; + use crate::expr::lit; + use crate::expr::not; use crate::expr::or; use crate::expr::root; use crate::expr::test_harness; @@ -154,6 +260,45 @@ mod tests { ); } + fn bool_dtype() -> DType { + let names = ["a", "b"].into(); + let fields = vec![ + DType::Bool(Nullability::Nullable), + DType::Bool(Nullability::NonNullable), + ]; + DType::Struct(StructFields::new(names, fields), Nullability::NonNullable) + } + + #[test] + fn reduce_to_constant() -> VortexResult<()> { + let dtype = bool_dtype(); + assert_eq!(is_not_null(col("b")).optimize(&dtype)?, lit(true)); + assert_eq!(is_null(col("b")).optimize(&dtype)?, lit(false)); + Ok(()) + } + + #[test] + fn reduce_kleene() -> VortexResult<()> { + let dtype = bool_dtype(); + assert_eq!( + is_not_null(and(col("a"), col("b"))).optimize(&dtype)?, + or(is_not_null(col("a")), not(col("b"))) + ); + assert_eq!( + is_not_null(or(col("a"), col("b"))).optimize(&dtype)?, + or(is_not_null(col("a")), col("b")) + ); + assert_eq!( + is_null(and(col("a"), col("b"))).optimize(&dtype)?, + and(is_null(col("a")), col("b")) + ); + assert_eq!( + is_null(or(col("a"), col("b"))).optimize(&dtype)?, + and(is_null(col("a")), not(col("b"))) + ); + Ok(()) + } + #[test] fn replace_children() { let expr = is_not_null(root()); From 10c123b5219024ac63852b136fcf389fc76bb7f1 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 25 Sep 2026 16:44:44 +0100 Subject: [PATCH 08/22] lint --- vortex-array/src/scalar_fn/fns/between/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index 717074eaeb5..69b07ef62f6 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -341,7 +341,6 @@ mod tests { use rstest::rstest; use vortex_buffer::buffer; - use vortex_error::vortex_err; use super::*; use crate::IntoArray; From 2a615539a94703a688579e7745623cbe1520b971 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 25 Sep 2026 17:05:15 +0100 Subject: [PATCH 09/22] fix --- vortex-array/src/array/probe/repeated.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/vortex-array/src/array/probe/repeated.rs b/vortex-array/src/array/probe/repeated.rs index 8551bda19c6..fced8c2319a 100644 --- a/vortex-array/src/array/probe/repeated.rs +++ b/vortex-array/src/array/probe/repeated.rs @@ -8,10 +8,12 @@ use vortex_error::vortex_err; use crate::ArrayRef; use crate::ExecutionCtx; +use crate::IntoArray; use crate::array::probe::ArrayProbe; use crate::array::probe::array::check_bounds; use crate::array::probe::array::check_dtype; use crate::array::probe::array::child_of; +use crate::arrays::BoolArray; use crate::arrays::ScalarFn; use crate::scalar::Scalar; use crate::validity::Validity; @@ -57,7 +59,7 @@ impl RepeatedArrayProbe { /// Read the scalar at `index`, including its nullness, reusing retained preparation. pub fn execute_scalar(&mut self, index: usize, ctx: &mut ExecutionCtx) -> VortexResult { // Probing validity of a lazy ScalarFn can recurse back into this scalar - // so we need to avoid calling probe_scalar_retained. See + // so we need to avoid calling execute_is_valid. See // execute_scalar_once in probe/array.rs. if !self.array.is::() && !self.execute_is_valid(index, ctx)? { return Ok(Scalar::null(self.array.dtype().clone())); @@ -89,6 +91,12 @@ impl RepeatedArrayProbe { return Ok(false); } Validity::Array(array) => { + // A lazy validity mask would otherwise be reexecuted for every row + let array = if array.is::() { + array.execute::(ctx)?.into_array() + } else { + array + }; self.validity = Some(Box::new(RepeatedArrayProbe::new(array))); } } From 7f656e21b3ff35e5a0299627d54d4553731cd2c0 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 25 Sep 2026 17:39:56 +0100 Subject: [PATCH 10/22] fix scalarfn being the parent --- vortex-array/src/array/probe/repeated.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/vortex-array/src/array/probe/repeated.rs b/vortex-array/src/array/probe/repeated.rs index fced8c2319a..3d6e4618603 100644 --- a/vortex-array/src/array/probe/repeated.rs +++ b/vortex-array/src/array/probe/repeated.rs @@ -91,8 +91,9 @@ impl RepeatedArrayProbe { return Ok(false); } Validity::Array(array) => { - // A lazy validity mask would otherwise be reexecuted for every row - let array = if array.is::() { + // ScalarFn's validity mask is lazy but we don't want to + // reevaluate it for every new probe request + let array = if self.array.is::() { array.execute::(ctx)?.into_array() } else { array From 2fac254903824be3dd3c3ee3e359b2f2f7d66aad Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 25 Sep 2026 17:56:26 +0100 Subject: [PATCH 11/22] rebase --- vortex-array/src/expr/bound_expression.rs | 29 +++++++++++++++++ vortex-array/src/expr/expression.rs | 28 ---------------- vortex-array/src/scalar_fn/erased.rs | 1 - vortex-array/src/scalar_fn/fns/between/mod.rs | 4 +-- vortex-array/src/scalar_fn/fns/binary/mod.rs | 1 - vortex-array/src/scalar_fn/fns/cast/mod.rs | 2 -- .../src/scalar_fn/fns/fill_null/mod.rs | 1 - vortex-array/src/scalar_fn/fns/is_not_null.rs | 32 +++++++++++++------ vortex-array/src/scalar_fn/fns/mask/mod.rs | 1 - vortex-array/src/scalar_fn/typed.rs | 1 - 10 files changed, 53 insertions(+), 47 deletions(-) diff --git a/vortex-array/src/expr/bound_expression.rs b/vortex-array/src/expr/bound_expression.rs index 631ffe4a1d3..85a4d40d158 100644 --- a/vortex-array/src/expr/bound_expression.rs +++ b/vortex-array/src/expr/bound_expression.rs @@ -22,6 +22,12 @@ use crate::expr::traversal::TraversalOrder; use crate::expr::traversal::pre_order_visit_down; use crate::scalar_fn::ScalarFnRef; use crate::scalar_fn::ScalarFnVTable; +use crate::scalar_fn::EmptyOptions; +use crate::scalar_fn::ExpressionReduceNode; +use crate::scalar_fn::ReduceNode; +use crate::scalar_fn::ReduceNodeValidity; +use crate::scalar_fn::ScalarFnVTableExt; +use crate::scalar_fn::fns::is_not_null::IsNotNull; use crate::stats::rewrite::falsify; use crate::stats::rewrite::satisfy; @@ -241,6 +247,29 @@ impl BoundExpression { is_bound_to } + /// Returns a new expression representing the validity mask output of this expression. + /// + /// Returned expression evaluates to a non-nullable boolean array. + /// When scalar function's validity is irreducible, returns + /// "is_not_null(self)", which requires evaluating "self". + pub fn validity(&self) -> VortexResult { + match self { + // The scope is exactly as valid as itself. + Self::Root { .. } => Ok(self.clone()), + Self::Scalar { .. } => { + let node = ExpressionReduceNode::new(self); + Ok(match node.validity()? { + ReduceNodeValidity::Reduced(reduced) => reduced.into_expression(), + // IsNotNull(x) -> x.validity() symbolic reduction rule works only + // when node's.validity is Reduced to avoid infinite recursion. + ReduceNodeValidity::Irreducible => { + IsNotNull.try_new_bound_expr(EmptyOptions, [self.clone()])? + } + }) + } + } + } + /// Return an expression that proves this predicate is definitely false from statistics. pub fn falsify(&self, session: &VortexSession) -> VortexResult> { falsify(self, session) diff --git a/vortex-array/src/expr/expression.rs b/vortex-array/src/expr/expression.rs index cd074823fcf..af21524a84d 100644 --- a/vortex-array/src/expr/expression.rs +++ b/vortex-array/src/expr/expression.rs @@ -18,14 +18,9 @@ use crate::dtype::DType; use crate::expr::display::DisplayTreeExpr; use crate::expr::traversal::TraversalOrder; use crate::expr::traversal::pre_order_visit_down; -use crate::scalar_fn::EmptyOptions; -use crate::scalar_fn::ExpressionReduceNode; -use crate::scalar_fn::ReduceNode; -use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnRef; use crate::scalar_fn::ScalarFnVTable; use crate::scalar_fn::ScalarFnVTableExt; -use crate::scalar_fn::fns::is_not_null::IsNotNull; /// An empty child slice, returned by [`Expression::children`] for childless variants. const NO_CHILDREN: &[Expression] = &[]; @@ -163,29 +158,6 @@ impl Expression { } } - /// Returns a new expression representing the validity mask output of this expression. - /// - /// Returned expression evaluates to a non-nullable boolean array. - /// When scalar function's validity is irreducible, returns - /// "is_not_null(self)", which requires evaluating "self". - pub fn validity(&self, scope: &DType) -> VortexResult { - match self { - // The scope is exactly as valid as itself. - Self::Root => Ok(Self::Root), - Self::Scalar { .. } => { - let node = ExpressionReduceNode::new(self, scope); - Ok(match node.validity()? { - ReduceNodeValidity::Reduced(reduced) => reduced.into_expression(), - // IsNotNull(x) -> x.validity() symbolic reduction rule works only - // when node's.validity is Reduced to avoid infinite recursion. - ReduceNodeValidity::Irreducible => { - IsNotNull.new_expr(EmptyOptions, [self.clone()]) - } - }) - } - } - } - /// Format the expression as a compact string. /// /// Since this is a recursive formatter, it is exposed on the public Expression type. diff --git a/vortex-array/src/scalar_fn/erased.rs b/vortex-array/src/scalar_fn/erased.rs index 8ae96d4d49a..da5c14de539 100644 --- a/vortex-array/src/scalar_fn/erased.rs +++ b/vortex-array/src/scalar_fn/erased.rs @@ -20,7 +20,6 @@ use crate::ArrayRef; use crate::ExecutionCtx; use crate::dtype::DType; use crate::expr::BoundExpression; -use crate::expr::Expression; use crate::expr::display::ExprDisplay; use crate::scalar_fn::ArrayReduceNode; use crate::scalar_fn::ExecutionArgs; diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index 69b07ef62f6..705747622c1 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -424,9 +424,9 @@ mod tests { .execute::(ctx)? .opt_bool_vec(ctx); - let validity_expr = expr.validity(data.dtype())?; + let validity_expr = expr.bind(data.dtype())?.validity()?; let declared = data - .apply(&validity_expr)? + .apply_bound(&validity_expr)? .execute::(ctx)? .bool_vec(ctx); diff --git a/vortex-array/src/scalar_fn/fns/binary/mod.rs b/vortex-array/src/scalar_fn/fns/binary/mod.rs index c86f6141460..b99604544e8 100644 --- a/vortex-array/src/scalar_fn/fns/binary/mod.rs +++ b/vortex-array/src/scalar_fn/fns/binary/mod.rs @@ -22,7 +22,6 @@ use crate::dtype::Nullability; use crate::expr::BoundExpression; use crate::expr::bound; use crate::expr::display::ExprDisplay; -use crate::expr::expression::Expression; use crate::proto::expr as pb; use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; diff --git a/vortex-array/src/scalar_fn/fns/cast/mod.rs b/vortex-array/src/scalar_fn/fns/cast/mod.rs index 4d4c307f6be..fe593df7623 100644 --- a/vortex-array/src/scalar_fn/fns/cast/mod.rs +++ b/vortex-array/src/scalar_fn/fns/cast/mod.rs @@ -38,8 +38,6 @@ use crate::dtype::DType; use crate::expr::BoundExpression; use crate::expr::bound; use crate::expr::display::ExprDisplay; -use crate::expr::expression::Expression; -use crate::expr::lit; use crate::proto::expr as pb; use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; diff --git a/vortex-array/src/scalar_fn/fns/fill_null/mod.rs b/vortex-array/src/scalar_fn/fns/fill_null/mod.rs index cdc642d9404..88056429558 100644 --- a/vortex-array/src/scalar_fn/fns/fill_null/mod.rs +++ b/vortex-array/src/scalar_fn/fns/fill_null/mod.rs @@ -23,7 +23,6 @@ use crate::arrays::ScalarFnArray; use crate::builtins::ArrayBuiltins; use crate::dtype::DType; use crate::expr::BoundExpression; -use crate::expr::Expression; use crate::scalar::Scalar; use crate::scalar_fn::Arity; use crate::scalar_fn::ChildName; diff --git a/vortex-array/src/scalar_fn/fns/is_not_null.rs b/vortex-array/src/scalar_fn/fns/is_not_null.rs index 1fa8e0bb420..b0df24d11ec 100644 --- a/vortex-array/src/scalar_fn/fns/is_not_null.rs +++ b/vortex-array/src/scalar_fn/fns/is_not_null.rs @@ -230,6 +230,8 @@ mod tests { use crate::dtype::Nullability; use crate::dtype::StructFields; use crate::expr::and; + use crate::expr::BoundExpression; + use crate::expr::Expression; use crate::expr::col; use crate::expr::eq; use crate::expr::get_item; @@ -269,11 +271,21 @@ mod tests { DType::Struct(StructFields::new(names, fields), Nullability::NonNullable) } + fn optimized(expr: Expression, scope: &DType) -> VortexResult { + expr.bind(scope)?.optimize() + } + #[test] fn reduce_to_constant() -> VortexResult<()> { let dtype = bool_dtype(); - assert_eq!(is_not_null(col("b")).optimize(&dtype)?, lit(true)); - assert_eq!(is_null(col("b")).optimize(&dtype)?, lit(false)); + assert_eq!( + optimized(is_not_null(col("b")), &dtype)?, + lit(true).bind(&dtype)? + ); + assert_eq!( + optimized(is_null(col("b")), &dtype)?, + lit(false).bind(&dtype)? + ); Ok(()) } @@ -281,20 +293,20 @@ mod tests { fn reduce_kleene() -> VortexResult<()> { let dtype = bool_dtype(); assert_eq!( - is_not_null(and(col("a"), col("b"))).optimize(&dtype)?, - or(is_not_null(col("a")), not(col("b"))) + optimized(is_not_null(and(col("a"), col("b"))), &dtype)?, + or(is_not_null(col("a")), not(col("b"))).bind(&dtype)? ); assert_eq!( - is_not_null(or(col("a"), col("b"))).optimize(&dtype)?, - or(is_not_null(col("a")), col("b")) + optimized(is_not_null(or(col("a"), col("b"))), &dtype)?, + or(is_not_null(col("a")), col("b")).bind(&dtype)? ); assert_eq!( - is_null(and(col("a"), col("b"))).optimize(&dtype)?, - and(is_null(col("a")), col("b")) + optimized(is_null(and(col("a"), col("b"))), &dtype)?, + and(is_null(col("a")), col("b")).bind(&dtype)? ); assert_eq!( - is_null(or(col("a"), col("b"))).optimize(&dtype)?, - and(is_null(col("a")), not(col("b"))) + optimized(is_null(or(col("a"), col("b"))), &dtype)?, + and(is_null(col("a")), not(col("b"))).bind(&dtype)? ); Ok(()) } diff --git a/vortex-array/src/scalar_fn/fns/mask/mod.rs b/vortex-array/src/scalar_fn/fns/mask/mod.rs index 0d3f953946c..42f1441059d 100644 --- a/vortex-array/src/scalar_fn/fns/mask/mod.rs +++ b/vortex-array/src/scalar_fn/fns/mask/mod.rs @@ -23,7 +23,6 @@ use crate::child_to_validity; use crate::dtype::DType; use crate::dtype::Nullability; use crate::expr::BoundExpression; -use crate::expr::Expression; use crate::expr::bound; use crate::scalar::Scalar; use crate::scalar_fn::Arity; diff --git a/vortex-array/src/scalar_fn/typed.rs b/vortex-array/src/scalar_fn/typed.rs index 5f7b53bb138..f2c3c479ad5 100644 --- a/vortex-array/src/scalar_fn/typed.rs +++ b/vortex-array/src/scalar_fn/typed.rs @@ -24,7 +24,6 @@ use crate::ArrayRef; use crate::ExecutionCtx; use crate::dtype::DType; use crate::expr::BoundExpression; -use crate::expr::Expression; use crate::expr::display::ExprDisplay; use crate::scalar_fn::Arity; use crate::scalar_fn::ArrayReduceNode; From 878780d0ada6c285771b2c6ff10edb5136d21e7f Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 25 Sep 2026 17:57:47 +0100 Subject: [PATCH 12/22] falsify stats based on literals --- vortex-array/src/stats/rewrite.rs | 23 +++++++------- vortex-array/src/stats/rewrite/builtins.rs | 37 ++++++++++++++++++++++ 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/vortex-array/src/stats/rewrite.rs b/vortex-array/src/stats/rewrite.rs index 453115c2005..24ba509a364 100644 --- a/vortex-array/src/stats/rewrite.rs +++ b/vortex-array/src/stats/rewrite.rs @@ -152,21 +152,22 @@ mod tests { use crate::dtype::PType; use crate::expr::BoundExpression; use crate::expr::lit; + use crate::expr::not; use crate::expr::or; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; - use crate::scalar_fn::fns::literal::Literal; + use crate::scalar_fn::fns::not::Not; use crate::stats::session::StatsSessionExt; #[derive(Debug)] - struct StaticLiteralRule { + struct StaticNotRule { falsifier: Option, satisfier: Option, } - impl StatsRewriteRule for StaticLiteralRule { + impl StatsRewriteRule for StaticNotRule { fn scalar_fn_id(&self) -> ScalarFnId { - Literal.id() + Not.id() } fn falsify( @@ -190,17 +191,17 @@ mod tests { fn combines_multiple_falsifiers_with_or() -> VortexResult<()> { let session = crate::array_session(); let dtype = DType::Primitive(PType::I32, Nullability::NonNullable); - session.stats().register_rewrite(StaticLiteralRule { + session.stats().register_rewrite(StaticNotRule { falsifier: Some(lit(false).bind(&dtype)?), satisfier: None, }); - session.stats().register_rewrite(StaticLiteralRule { + session.stats().register_rewrite(StaticNotRule { falsifier: Some(lit(true).bind(&dtype)?), satisfier: None, }); assert_eq!( - lit(true).bind(&dtype)?.falsify(&session)?, + not(lit(true)).bind(&dtype)?.falsify(&session)?, Some(or(lit(false), lit(true)).bind(&dtype)?) ); Ok(()) @@ -210,17 +211,17 @@ mod tests { fn combines_multiple_satisfiers_with_or() -> VortexResult<()> { let session = crate::array_session(); let dtype = DType::Primitive(PType::I32, Nullability::NonNullable); - session.stats().register_rewrite(StaticLiteralRule { + session.stats().register_rewrite(StaticNotRule { falsifier: None, satisfier: Some(lit(false).bind(&dtype)?), }); - session.stats().register_rewrite(StaticLiteralRule { + session.stats().register_rewrite(StaticNotRule { falsifier: None, satisfier: Some(lit(true).bind(&dtype)?), }); assert_eq!( - lit(true).bind(&dtype)?.satisfy(&session)?, + not(lit(true)).bind(&dtype)?.satisfy(&session)?, Some(or(lit(false), lit(true)).bind(&dtype)?) ); Ok(()) @@ -231,7 +232,7 @@ mod tests { let session = crate::array_session(); let dtype = DType::Primitive(PType::I32, Nullability::NonNullable); - let expr = lit(true).bind(&dtype)?; + let expr = not(lit(true)).bind(&dtype)?; assert_eq!(expr.falsify(&session)?, None); assert_eq!(expr.satisfy(&session)?, None); Ok(()) diff --git a/vortex-array/src/stats/rewrite/builtins.rs b/vortex-array/src/stats/rewrite/builtins.rs index 116e6f0cdae..872aef6e33c 100644 --- a/vortex-array/src/stats/rewrite/builtins.rs +++ b/vortex-array/src/stats/rewrite/builtins.rs @@ -60,6 +60,7 @@ pub(crate) fn register_builtins(session: &StatsSession) { session.register_rewrite(BinaryNanCountStatsRewrite); session.register_rewrite(BinaryAllNonNanStatsRewrite); session.register_rewrite(BetweenStatsRewrite); + session.register_rewrite(LiteralStatsRewrite); session.register_rewrite(IsNullNullCountStatsRewrite); session.register_rewrite(IsNullAllNonNullStatsRewrite); session.register_rewrite(IsNullAllNullStatsRewrite); @@ -82,6 +83,42 @@ fn row_count() -> BoundExpression { .vortex_expect("row-count expressions are always well-typed") } +#[derive(Debug)] +struct LiteralStatsRewrite; + +impl StatsRewriteRule for LiteralStatsRewrite { + fn scalar_fn_id(&self) -> ScalarFnId { + Literal.id() + } + + fn falsify( + &self, + expr: &BoundExpression, + _session: &VortexSession, + ) -> VortexResult> { + Ok(match literal_truth(expr) { + Some(true) => None, + _ => Some(lit(true)), + }) + } + + fn satisfy( + &self, + expr: &BoundExpression, + _session: &VortexSession, + ) -> VortexResult> { + Ok(match literal_truth(expr) { + Some(true) => Some(lit(true)), + _ => None, + }) + } +} + +/// Bool literal predicate value +fn literal_truth(expr: &BoundExpression) -> Option { + expr.as_::().as_bool_opt().and_then(|b| b.value()) +} + #[derive(Debug)] struct BinaryNanCountStatsRewrite; From 24c1a23a7928fca5e296660fdc5699e1cb16054e Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 25 Sep 2026 18:04:55 +0100 Subject: [PATCH 13/22] fix --- vortex-array/src/expr/bound_expression.rs | 4 ++-- vortex-array/src/expr/expression.rs | 1 - vortex-array/src/scalar_fn/fns/is_not_null.rs | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/vortex-array/src/expr/bound_expression.rs b/vortex-array/src/expr/bound_expression.rs index 85a4d40d158..4ae3bd23547 100644 --- a/vortex-array/src/expr/bound_expression.rs +++ b/vortex-array/src/expr/bound_expression.rs @@ -20,12 +20,12 @@ use crate::expr::display::DisplayTreeExpr; use crate::expr::scope::Scope; use crate::expr::traversal::TraversalOrder; use crate::expr::traversal::pre_order_visit_down; -use crate::scalar_fn::ScalarFnRef; -use crate::scalar_fn::ScalarFnVTable; use crate::scalar_fn::EmptyOptions; use crate::scalar_fn::ExpressionReduceNode; use crate::scalar_fn::ReduceNode; use crate::scalar_fn::ReduceNodeValidity; +use crate::scalar_fn::ScalarFnRef; +use crate::scalar_fn::ScalarFnVTable; use crate::scalar_fn::ScalarFnVTableExt; use crate::scalar_fn::fns::is_not_null::IsNotNull; use crate::stats::rewrite::falsify; diff --git a/vortex-array/src/expr/expression.rs b/vortex-array/src/expr/expression.rs index af21524a84d..4a00516cc4b 100644 --- a/vortex-array/src/expr/expression.rs +++ b/vortex-array/src/expr/expression.rs @@ -20,7 +20,6 @@ use crate::expr::traversal::TraversalOrder; use crate::expr::traversal::pre_order_visit_down; use crate::scalar_fn::ScalarFnRef; use crate::scalar_fn::ScalarFnVTable; -use crate::scalar_fn::ScalarFnVTableExt; /// An empty child slice, returned by [`Expression::children`] for childless variants. const NO_CHILDREN: &[Expression] = &[]; diff --git a/vortex-array/src/scalar_fn/fns/is_not_null.rs b/vortex-array/src/scalar_fn/fns/is_not_null.rs index b0df24d11ec..8eb2731a675 100644 --- a/vortex-array/src/scalar_fn/fns/is_not_null.rs +++ b/vortex-array/src/scalar_fn/fns/is_not_null.rs @@ -229,9 +229,9 @@ mod tests { use crate::dtype::DType; use crate::dtype::Nullability; use crate::dtype::StructFields; - use crate::expr::and; use crate::expr::BoundExpression; use crate::expr::Expression; + use crate::expr::and; use crate::expr::col; use crate::expr::eq; use crate::expr::get_item; From 8162779a73ec83bb139382ffb5fc295a056c35a7 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Mon, 28 Sep 2026 10:23:58 +0100 Subject: [PATCH 14/22] add is_null pushdown test Signed-off-by: Mikhail Kot --- vortex-array/src/scalar_fn/erased.rs | 2 +- vortex-sqllogictest/slt/is_null.slt | 33 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 vortex-sqllogictest/slt/is_null.slt diff --git a/vortex-array/src/scalar_fn/erased.rs b/vortex-array/src/scalar_fn/erased.rs index da5c14de539..d1b09ce5f1f 100644 --- a/vortex-array/src/scalar_fn/erased.rs +++ b/vortex-array/src/scalar_fn/erased.rs @@ -35,7 +35,7 @@ use crate::scalar_fn::typed::TypedScalarFnInstance; /// A type-erased scalar function, pairing a vtable with bound options behind a trait object. /// /// This stores a [`ScalarFnVTable`] and its options behind an `Arc`, allowing -/// heterogeneous storage inside [`Expression`] and [`crate::arrays::ScalarFnArray`]. +/// heterogeneous storage inside [`crate::arrays::ScalarFnArray`]. /// /// Use [`super::TypedScalarFnInstance::new()`] to construct, and [`super::TypedScalarFnInstance::erased()`] to /// obtain a [`ScalarFnRef`]. diff --git a/vortex-sqllogictest/slt/is_null.slt b/vortex-sqllogictest/slt/is_null.slt new file mode 100644 index 00000000000..ee9dc5224d9 --- /dev/null +++ b/vortex-sqllogictest/slt/is_null.slt @@ -0,0 +1,33 @@ +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: Copyright the Vortex contributors + +include ./setup.slt.no + +statement ok +COPY (SELECT * FROM (VALUES (1),(2),(CAST(NULL AS BIGINT))) AS t(x)) +TO '${WORK_DIR}/is_null.vortex'; + +onlyif duckdb +query TT +EXPLAIN (FORMAT JSON) +SELECT x FROM '${WORK_DIR}/is_null.vortex' WHERE x IS NULL; +---- +:.*FILTER.* + +query I +SELECT x FROM '${WORK_DIR}/is_null.vortex' WHERE x IS NULL; +---- +NULL + +onlyif duckdb +query TT +EXPLAIN (FORMAT JSON) +SELECT x FROM '${WORK_DIR}/is_null.vortex' WHERE x IS NOT NULL; +---- +:.*FILTER.* + +query I +SELECT x FROM '${WORK_DIR}/is_null.vortex' WHERE x IS NOT NULL; +---- +1 +2 From e93031d13945939f3857e2241b8ca056574d03cb Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Mon, 28 Sep 2026 11:36:38 +0100 Subject: [PATCH 15/22] don't execute fill_null array on short circuit Signed-off-by: Mikhail Kot --- vortex-array/src/scalar_fn/fns/fill_null/mod.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/vortex-array/src/scalar_fn/fns/fill_null/mod.rs b/vortex-array/src/scalar_fn/fns/fill_null/mod.rs index 88056429558..8299360bcf9 100644 --- a/vortex-array/src/scalar_fn/fns/fill_null/mod.rs +++ b/vortex-array/src/scalar_fn/fns/fill_null/mod.rs @@ -167,10 +167,7 @@ fn fill_null_canonical( ) -> VortexResult { let arr = canonical.to_array_ref(); if let Some(result) = short_circuit(&arr, fill_value)? { - // The short circuit can return a lazy `ScalarFn`, so this forces it for now. - // TODO(aduffy): Remove this once we have better driver check. We're also implicitly - // relying on the fact that Cast execution will do an optimize on its result. - return result.execute::(ctx); + return Ok(result); } match canonical { CanonicalView::Bool(a) => ::fill_null(a, fill_value, ctx)? From 1a5ba5d24975d8b731d2fbe69b0b1ec9492ce791 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Mon, 28 Sep 2026 13:21:39 +0100 Subject: [PATCH 16/22] remove extra comment in nullasfalse Signed-off-by: Mikhail Kot --- vortex-array/src/mask.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/vortex-array/src/mask.rs b/vortex-array/src/mask.rs index ee55abc9875..e3985da2272 100644 --- a/vortex-array/src/mask.rs +++ b/vortex-array/src/mask.rs @@ -47,10 +47,6 @@ impl Executable for Mask { /// /// Use for filter and pruning predicates over nullable data, where SQL semantics treat `NULL` as /// not matching. -/// -/// Prefer `array.null_as_false().execute(ctx)` over `array.fill_null(false)?.execute::(ctx)`: -/// `fill_null` on a lazy `ScalarFn` array (e.g. the result of `apply()`) is currently -/// slow because its `validity()` executes the predicate expression. pub struct NullAsFalse(ArrayRef); impl ArrayRef { From d4aa321f082129bde7c218bf9f884f9f8b8d77fb Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Mon, 28 Sep 2026 17:41:29 +0100 Subject: [PATCH 17/22] review Signed-off-by: Mikhail Kot --- vortex-array/src/array/vtable/operations.rs | 5 +++-- vortex-array/src/scalar_fn/vtable.rs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/vortex-array/src/array/vtable/operations.rs b/vortex-array/src/array/vtable/operations.rs index 471ea6e3f88..69dd7de9c0a 100644 --- a/vortex-array/src/array/vtable/operations.rs +++ b/vortex-array/src/array/vtable/operations.rs @@ -27,8 +27,9 @@ pub trait OperationsVTable { /// Read the non-null scalar at `index` of the array in `state`. /// - /// Bounds and validity have been checked; the row is non-null. `state` carries the typed - /// view of the array and, for a read through a + /// Caller must check bounds and validity before calling this function and + /// ensure target row is non-NULL. + /// `state` carries the typed view of the array and, for a read through a /// [`RepeatedArrayProbe`](crate::RepeatedArrayProbe), the state that probe keeps. Read /// children through [`ProbeState::slot`], which follows the read's policy without the /// encoding having to know it. Take encoding state from [`ProbeState::retained`]. The scalar must retain the source's diff --git a/vortex-array/src/scalar_fn/vtable.rs b/vortex-array/src/scalar_fn/vtable.rs index 5361d0b8774..331b11881f3 100644 --- a/vortex-array/src/scalar_fn/vtable.rs +++ b/vortex-array/src/scalar_fn/vtable.rs @@ -57,7 +57,7 @@ pub enum ReduceNodeValidity { } /// IsNotNull(child) as a reducible node -pub fn is_not_null_node(child: &T) -> VortexResult { +pub(crate) fn is_not_null_node(child: &T) -> VortexResult { child.new_node(IsNotNull.bind(EmptyOptions), std::slice::from_ref(child)) } From b36849d3f0a910c751a72520f7e46bf3e745a6d0 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Tue, 29 Sep 2026 18:09:17 +0100 Subject: [PATCH 18/22] use a special inner array instead of isnotnull for execution Signed-off-by: Mikhail Kot --- vortex-array/src/array/probe/array.rs | 10 -- vortex-array/src/array/probe/repeated.rs | 27 +++- .../src/arrays/scalar_fn/vtable/mod.rs | 9 +- .../src/arrays/scalar_fn/vtable/validity.rs | 144 ++++++++++++++++++ vortex-array/src/scalar_fn/fns/between/mod.rs | 8 +- vortex-array/src/scalar_fn/fns/is_not_null.rs | 29 +--- vortex-array/src/scalar_fn/fns/is_null.rs | 4 +- 7 files changed, 178 insertions(+), 53 deletions(-) create mode 100644 vortex-array/src/arrays/scalar_fn/vtable/validity.rs diff --git a/vortex-array/src/array/probe/array.rs b/vortex-array/src/array/probe/array.rs index 7d3c83cb3b8..53418709abe 100644 --- a/vortex-array/src/array/probe/array.rs +++ b/vortex-array/src/array/probe/array.rs @@ -83,16 +83,6 @@ fn execute_scalar_once( index: usize, ctx: &mut ExecutionCtx, ) -> VortexResult { - // ScalarFn's validity is lazy, and for some functions evaluating - // validity is equal to evaluating the function. For such functions - // validity() is is_not_null(original array). So we get the chain: - // execute_is_valid_once -> array.validity() -> - // execute_is_valid -> execute_scalar (mask) -> - // mask.probe_scalar_once -> scalar_at -> array.execute_scalar, and as - // "array" is the original array, we get infinite recursion. - // - // For these functions probe_scalar_once gets the nullable scalar anyway. - // See also execute_scalar in probe/repeated.rs if !array.is::() && !execute_is_valid_once(array, index, ctx)? { return Ok(Scalar::null(array.dtype().clone())); } diff --git a/vortex-array/src/array/probe/repeated.rs b/vortex-array/src/array/probe/repeated.rs index 3d6e4618603..3bf315ebc9f 100644 --- a/vortex-array/src/array/probe/repeated.rs +++ b/vortex-array/src/array/probe/repeated.rs @@ -16,6 +16,9 @@ use crate::array::probe::array::child_of; use crate::arrays::BoolArray; use crate::arrays::ScalarFn; use crate::scalar::Scalar; +use crate::scalar_fn::ArrayReduceNode; +use crate::scalar_fn::ReduceNode; +use crate::scalar_fn::ReduceNodeValidity; use crate::validity::Validity; /// A row accessor that owns its array and keeps preparation between reads. @@ -58,10 +61,12 @@ impl RepeatedArrayProbe { /// Read the scalar at `index`, including its nullness, reusing retained preparation. pub fn execute_scalar(&mut self, index: usize, ctx: &mut ExecutionCtx) -> VortexResult { - // Probing validity of a lazy ScalarFn can recurse back into this scalar - // so we need to avoid calling execute_is_valid. See - // execute_scalar_once in probe/array.rs. - if !self.array.is::() && !self.execute_is_valid(index, ctx)? { + // For some scalar functions executing validity is equal to executing the + // function itself. Thus execute_is_valid + probe_scalar_retained do + // two evaluations instead of one. probe_scalar_once for such functions + // already gives you the nullable scalar, so skip the first check + // TODO(myrrc) this should be removed once we no longer probe validity here + if !self.irreducible_validity()? && !self.execute_is_valid(index, ctx)? { return Ok(Scalar::null(self.array.dtype().clone())); } let result = @@ -71,6 +76,18 @@ impl RepeatedArrayProbe { check_dtype(&self.array, result) } + fn irreducible_validity(&self) -> VortexResult { + if self.uniform_validity.is_some() || self.validity.is_some() { + return Ok(false); + } + Ok(self.array.dtype().is_nullable() + && self.array.is::() + && matches!( + ArrayReduceNode::new(&self.array).validity()?, + ReduceNodeValidity::Irreducible + )) + } + /// Whether the row at `index` is valid, through the retained validity. pub fn execute_is_valid(&mut self, index: usize, ctx: &mut ExecutionCtx) -> VortexResult { check_bounds(&self.array, index)?; @@ -93,6 +110,8 @@ impl RepeatedArrayProbe { Validity::Array(array) => { // ScalarFn's validity mask is lazy but we don't want to // reevaluate it for every new probe request + // TODO(myrrc) remove this once probing api allows us + // to specify validity let array = if self.array.is::() { array.execute::(ctx)?.into_array() } else { diff --git a/vortex-array/src/arrays/scalar_fn/vtable/mod.rs b/vortex-array/src/arrays/scalar_fn/vtable/mod.rs index b467dc80004..d2f45f19330 100644 --- a/vortex-array/src/arrays/scalar_fn/vtable/mod.rs +++ b/vortex-array/src/arrays/scalar_fn/vtable/mod.rs @@ -1,12 +1,15 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors mod operations; +mod validity; + use std::hash::Hash; use std::hash::Hasher; use std::marker::PhantomData; use std::ops::Deref; use itertools::Itertools; +use validity::ValidityArray; use vortex_error::VortexResult; use vortex_error::vortex_bail; use vortex_error::vortex_ensure; @@ -40,7 +43,6 @@ use crate::scalar_fn::ReduceNode; use crate::scalar_fn::ReduceNodeValidity; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::VecExecutionArgs; -use crate::scalar_fn::fns::is_not_null::IsNotNull; use crate::serde::ArrayChildren; use crate::validity::Validity; @@ -246,10 +248,7 @@ impl ValidityVTable for ScalarFn { let node = ArrayReduceNode::new(view.as_ref()); Ok(Validity::Array(match node.validity()? { ReduceNodeValidity::Reduced(reduced) => reduced.into_array(), - // We get validity only after evaluating this node. To avoid - // infinite recursion, IsNotNull(x) -> x.validity() symbolically - // rewrites only in the Reduced() case. - ReduceNodeValidity::Irreducible => IsNotNull::new(view.as_ref().clone()).into_array(), + ReduceNodeValidity::Irreducible => ValidityArray::new(node.into_array()).into_array(), })) } } diff --git a/vortex-array/src/arrays/scalar_fn/vtable/validity.rs b/vortex-array/src/arrays/scalar_fn/vtable/validity.rs new file mode 100644 index 00000000000..bd0f8774900 --- /dev/null +++ b/vortex-array/src/arrays/scalar_fn/vtable/validity.rs @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +use vortex_error::VortexResult; +use vortex_error::vortex_bail; +use vortex_error::vortex_panic; +use vortex_session::VortexSession; +use vortex_session::registry::CachedId; + +use crate::ArrayRef; +use crate::array::Array; +use crate::array::ArrayId; +use crate::array::ArrayParts; +use crate::array::ArrayView; +use crate::array::EmptyArrayData; +use crate::array::OperationsVTable; +use crate::array::VTable; +use crate::array::ValidityVTable; +use crate::array::with_empty_buffers; +use crate::array_slots; +use crate::buffer::BufferHandle; +use crate::dtype::DType; +use crate::dtype::Nullability; +use crate::executor::ExecutionCtx; +use crate::executor::ExecutionResult; +use crate::scalar::Scalar; +use crate::serde::ArrayChildren; +use crate::validity::Validity; + +#[array_slots(ScalarFnValidity)] +struct ValiditySlots { + #[slot(0)] + child: ArrayRef, +} + +pub(crate) type ValidityArray = Array; + +/// Special case for scalar functions whose validity is Irreducible. +/// Does a single execute step of a child. +#[derive(Clone, Debug)] +pub(crate) struct ScalarFnValidity; + +impl Array { + pub(crate) fn new(child: ArrayRef) -> Self { + let len = child.len(); + unsafe { + Array::from_parts_unchecked( + ArrayParts::new(ScalarFnValidity, Validity::DTYPE, len, EmptyArrayData) + .with_slots(ValiditySlots { child }.into_slots()), + ) + } + } +} + +impl VTable for ScalarFnValidity { + type TypedArrayData = EmptyArrayData; + type OperationsVTable = Self; + type ValidityVTable = Self; + + fn id(&self) -> ArrayId { + static ID: CachedId = CachedId::new("vortex.scalar_fn_validity"); + *ID + } + + fn validate( + &self, + _data: &Self::TypedArrayData, + _dtype: &DType, + _len: usize, + _slots: &[Option], + ) -> VortexResult<()> { + // We construct it internally, valid by definition + Ok(()) + } + + fn nbuffers(_array: ArrayView<'_, Self>) -> usize { + 0 + } + + fn buffer(_array: ArrayView<'_, Self>, _idx: usize) -> BufferHandle { + vortex_panic!("ScalarFnValidity has no buffers") + } + + fn buffer_name(_array: ArrayView<'_, Self>, _idx: usize) -> Option { + None + } + + fn with_buffers( + &self, + array: ArrayView<'_, Self>, + buffers: &[BufferHandle], + ) -> VortexResult> { + with_empty_buffers(self, array, buffers) + } + + fn serialize( + _array: ArrayView<'_, Self>, + _session: &VortexSession, + ) -> VortexResult>> { + Ok(None) + } + + fn deserialize( + &self, + _dtype: &DType, + _len: usize, + _metadata: &[u8], + _buffers: &[BufferHandle], + _children: &dyn ArrayChildren, + _session: &VortexSession, + ) -> VortexResult> { + vortex_bail!("ScalarFnValidity deserialize not supported"); + } + + fn slot_name(_array: ArrayView<'_, Self>, idx: usize) -> String { + ValiditySlots::NAMES[idx].to_string() + } + + fn execute(array: Array, ctx: &mut ExecutionCtx) -> VortexResult { + let len = array.len(); + let child = array.child().clone().execute::(ctx)?; + Ok(ExecutionResult::done(child.validity()?.to_array(len))) + } +} + +impl OperationsVTable for ScalarFnValidity { + type ProbeState = (); + + fn scalar_at( + array: ArrayView<'_, ScalarFnValidity>, + index: usize, + ctx: &mut ExecutionCtx, + ) -> VortexResult { + let child = array.child(); + let value = child.dyn_array().probe_scalar_once(child, index, ctx)?; + Ok(Scalar::bool(!value.is_null(), Nullability::NonNullable)) + } +} + +impl ValidityVTable for ScalarFnValidity { + fn validity(_array: ArrayView<'_, ScalarFnValidity>) -> VortexResult { + Ok(Validity::NonNullable) + } +} diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index 705747622c1..943d13db2eb 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -348,7 +348,6 @@ mod tests { use crate::arrays::BoolArray; use crate::arrays::DecimalArray; use crate::arrays::PrimitiveArray; - use crate::arrays::ScalarFn; use crate::arrays::StructArray; use crate::assert_arrays_eq; use crate::dtype::DType; @@ -362,7 +361,6 @@ mod tests { use crate::expr::root; use crate::scalar::DecimalValue; use crate::scalar::Scalar; - use crate::scalar_fn::fns::is_not_null::IsNotNull; use crate::test_harness::to_int_indices; use crate::validity::Validity; @@ -393,8 +391,10 @@ mod tests { let Validity::Array(validity) = lazy.validity()? else { vortex_bail!("non-lazy validity"); }; - let scalar_fn = validity.as_::(); - assert!(scalar_fn.scalar_fn().is::()); + assert_eq!( + validity.encoding_id().to_string(), + "vortex.scalar_fn_validity" + ); assert_arrays_eq!(validity, BoolArray::from_iter([true, false, true]), ctx); Ok(()) } diff --git a/vortex-array/src/scalar_fn/fns/is_not_null.rs b/vortex-array/src/scalar_fn/fns/is_not_null.rs index 8eb2731a675..a57118222a3 100644 --- a/vortex-array/src/scalar_fn/fns/is_not_null.rs +++ b/vortex-array/src/scalar_fn/fns/is_not_null.rs @@ -11,9 +11,6 @@ use vortex_session::registry::CachedId; use crate::ArrayRef; use crate::ExecutionCtx; -use crate::IntoArray; -use crate::arrays::ConstantArray; -use crate::arrays::ScalarFn; use crate::arrays::ScalarFnArray; use crate::dtype::DType; use crate::dtype::Nullability; @@ -32,7 +29,6 @@ use crate::scalar_fn::fns::is_null::IsNull; use crate::scalar_fn::fns::not::Not; use crate::scalar_fn::fns::operators::Operator; use crate::scalar_fn::is_not_null_node; -use crate::validity::Validity; /// In array context, reduce /// @@ -109,20 +105,6 @@ pub(crate) fn reduce_null(is_null: bool, node: &T) -> VortexResul Ok(Some(node.new_node(Binary.bind(combine), &[left, right])?)) } -/// If we get a scalar function child, at this point we have done all symbolic -/// reductions. Execute child one step and return. For every other child, -/// return the child itself. -pub(crate) fn lazy_child_or_execute_step( - child: ArrayRef, - ctx: &mut ExecutionCtx, -) -> VortexResult { - if child.is::() { - child.execute::(ctx) - } else { - Ok(child) - } -} - /// Expression that checks for non-null values. #[derive(Clone)] pub struct IsNotNull; @@ -186,16 +168,9 @@ impl ScalarFnVTable for IsNotNull { &self, _data: &Self::Options, args: &dyn ExecutionArgs, - ctx: &mut ExecutionCtx, + _ctx: &mut ExecutionCtx, ) -> VortexResult { - let child = lazy_child_or_execute_step(args.get(0)?, ctx)?; - match child.validity()? { - Validity::NonNullable | Validity::AllValid => { - Ok(ConstantArray::new(true, args.row_count()).into_array()) - } - Validity::AllInvalid => Ok(ConstantArray::new(false, args.row_count()).into_array()), - Validity::Array(a) => Ok(a), - } + Ok(args.get(0)?.validity()?.to_array(args.row_count())) } fn reduce(&self, _options: &Self::Options, node: &T) -> VortexResult> { diff --git a/vortex-array/src/scalar_fn/fns/is_null.rs b/vortex-array/src/scalar_fn/fns/is_null.rs index e8f21a6293c..87eefb90e60 100644 --- a/vortex-array/src/scalar_fn/fns/is_null.rs +++ b/vortex-array/src/scalar_fn/fns/is_null.rs @@ -22,7 +22,6 @@ use crate::scalar_fn::ReduceNode; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; use crate::scalar_fn::ScalarFnVTableExt; -use crate::scalar_fn::fns::is_not_null::lazy_child_or_execute_step; use crate::scalar_fn::fns::is_not_null::reduce_null; use crate::validity::Validity; @@ -78,14 +77,13 @@ impl ScalarFnVTable for IsNull { &self, _data: &Self::Options, args: &dyn ExecutionArgs, - ctx: &mut ExecutionCtx, + _ctx: &mut ExecutionCtx, ) -> VortexResult { let child = args.get(0)?; if let Some(scalar) = child.as_constant() { return Ok(ConstantArray::new(scalar.is_null(), args.row_count()).into_array()); } - let child = lazy_child_or_execute_step(child, ctx)?; match child.validity()? { Validity::NonNullable | Validity::AllValid => { Ok(ConstantArray::new(false, args.row_count()).into_array()) From 8a789a2b06ffcd22e30c7123536471e8d44d0344 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Wed, 30 Sep 2026 10:33:31 +0100 Subject: [PATCH 19/22] comments Signed-off-by: Mikhail Kot --- vortex-array/src/array/probe/array.rs | 5 +++++ vortex-array/src/scalar_fn/fns/is_not_null.rs | 2 -- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/vortex-array/src/array/probe/array.rs b/vortex-array/src/array/probe/array.rs index 53418709abe..1afa12bb34d 100644 --- a/vortex-array/src/array/probe/array.rs +++ b/vortex-array/src/array/probe/array.rs @@ -83,6 +83,11 @@ fn execute_scalar_once( index: usize, ctx: &mut ExecutionCtx, ) -> VortexResult { + // For some scalar functions executing validity is equal to executing the + // function itself. Thus execute_is_valid_once + probe_scalar_once do + // two evaluations instead of one. probe_scalar_once for such functions + // already gives you the nullable scalar, so skip the first check + // TODO(myrrc) this should be removed once we no longer probe validity here if !array.is::() && !execute_is_valid_once(array, index, ctx)? { return Ok(Scalar::null(array.dtype().clone())); } diff --git a/vortex-array/src/scalar_fn/fns/is_not_null.rs b/vortex-array/src/scalar_fn/fns/is_not_null.rs index a57118222a3..1e7c1b6960a 100644 --- a/vortex-array/src/scalar_fn/fns/is_not_null.rs +++ b/vortex-array/src/scalar_fn/fns/is_not_null.rs @@ -53,8 +53,6 @@ pub(crate) fn reduce_null(is_null: bool, node: &T) -> VortexResul return Ok(Some(node.new_constant((!is_null).into()))); } - // Irreducible's validity is IsNotNull(self), if we rewrite it here, we'll - // get into infinite recursion if let ReduceNodeValidity::Reduced(validity) = child.validity()? { return Ok(Some(if is_null { validity.new_node(Not.bind(EmptyOptions), std::slice::from_ref(&validity))? From 4d1a7729a2cb03128d23a19c511c6426e303fc03 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Wed, 30 Sep 2026 11:42:58 +0100 Subject: [PATCH 20/22] more correct literal falsifier Signed-off-by: Mikhail Kot --- vortex-array/src/stats/rewrite/builtins.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/vortex-array/src/stats/rewrite/builtins.rs b/vortex-array/src/stats/rewrite/builtins.rs index 872aef6e33c..226615541f5 100644 --- a/vortex-array/src/stats/rewrite/builtins.rs +++ b/vortex-array/src/stats/rewrite/builtins.rs @@ -97,8 +97,8 @@ impl StatsRewriteRule for LiteralStatsRewrite { _session: &VortexSession, ) -> VortexResult> { Ok(match literal_truth(expr) { - Some(true) => None, - _ => Some(lit(true)), + Some(value) => Some(lit(!value)), + _ => None }) } @@ -108,7 +108,7 @@ impl StatsRewriteRule for LiteralStatsRewrite { _session: &VortexSession, ) -> VortexResult> { Ok(match literal_truth(expr) { - Some(true) => Some(lit(true)), + Some(value) => Some(lit(value)), _ => None, }) } From 6ae72b7cdfc350ba04d85e7987e59bae2ab9ba2e Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Wed, 30 Sep 2026 11:47:40 +0100 Subject: [PATCH 21/22] fix Signed-off-by: Mikhail Kot --- vortex-array/src/stats/rewrite/builtins.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/vortex-array/src/stats/rewrite/builtins.rs b/vortex-array/src/stats/rewrite/builtins.rs index 226615541f5..be87b79d266 100644 --- a/vortex-array/src/stats/rewrite/builtins.rs +++ b/vortex-array/src/stats/rewrite/builtins.rs @@ -96,10 +96,7 @@ impl StatsRewriteRule for LiteralStatsRewrite { expr: &BoundExpression, _session: &VortexSession, ) -> VortexResult> { - Ok(match literal_truth(expr) { - Some(value) => Some(lit(!value)), - _ => None - }) + Ok(literal_truth(expr).map(|value| lit(!value))) } fn satisfy( @@ -107,10 +104,7 @@ impl StatsRewriteRule for LiteralStatsRewrite { expr: &BoundExpression, _session: &VortexSession, ) -> VortexResult> { - Ok(match literal_truth(expr) { - Some(value) => Some(lit(value)), - _ => None, - }) + Ok(literal_truth(expr).map(|value| lit(value))) } } From 2b9f8c642d868c8a533fa4bd7b0eb862394ab360 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Wed, 30 Sep 2026 11:52:06 +0100 Subject: [PATCH 22/22] fix Signed-off-by: Mikhail Kot --- vortex-array/src/stats/rewrite/builtins.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vortex-array/src/stats/rewrite/builtins.rs b/vortex-array/src/stats/rewrite/builtins.rs index be87b79d266..f6ad2aca6cf 100644 --- a/vortex-array/src/stats/rewrite/builtins.rs +++ b/vortex-array/src/stats/rewrite/builtins.rs @@ -104,7 +104,7 @@ impl StatsRewriteRule for LiteralStatsRewrite { expr: &BoundExpression, _session: &VortexSession, ) -> VortexResult> { - Ok(literal_truth(expr).map(|value| lit(value))) + Ok(literal_truth(expr).map(lit)) } }