From 615e61c70ab32b47b1034c45b0f8df8e9ca52d3e Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Thu, 17 Sep 2026 16:20:18 +0100 Subject: [PATCH 1/3] tests Signed-off-by: Mikhail Kot --- vortex-array/Cargo.toml | 4 ++ vortex-array/benches/scalar_fn_probe.rs | 83 +++++++++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 vortex-array/benches/scalar_fn_probe.rs diff --git a/vortex-array/Cargo.toml b/vortex-array/Cargo.toml index cc5d869aaa1..b1ca29ceddf 100644 --- a/vortex-array/Cargo.toml +++ b/vortex-array/Cargo.toml @@ -228,6 +228,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..481ede71924 --- /dev/null +++ b/vortex-array/benches/scalar_fn_probe.rs @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +#![expect(clippy::unwrap_used)] + +use divan::Bencher; +use divan::black_box; +use rand::RngExt; +use rand::SeedableRng; +use rand::rngs::StdRng; +use std::sync::LazyLock; +use vortex_array::ArrayRef; +use vortex_array::IntoArray; +use vortex_array::VortexSessionExecute; +use vortex_array::array_session; +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 = 100; + +static SESSION: LazyLock = LazyLock::new(array_session); + +fn array() -> 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() +} + +fn indices() -> Vec { + let mut rng = StdRng::seed_from_u64(0); + (0..NUM_ACCESSES) + .map(|_| rng.random_range(0..ARRAY_SIZE)) + .collect() +} + +#[divan::bench] +fn probe_scalar_fn_once(bencher: Bencher) { + let array = array(); + 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.execute_scalar(index, ctx).unwrap()); + } + }); +} + +#[divan::bench] +fn probe_scalar_fn_repeated(bencher: Bencher) { + let array = array(); + 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()); + } + }); +} From 60940fc9e2f5ad75b416439e49c7f78e6aef4a1d Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Fri, 18 Sep 2026 12:20:10 +0100 Subject: [PATCH 2/3] more benchmark tests Signed-off-by: Mikhail Kot --- vortex-array/benches/scalar_fn_probe.rs | 65 +++++++++++++++---- .../src/arrays/scalar_fn/vtable/operations.rs | 57 ++++++++++++++++ 2 files changed, 110 insertions(+), 12 deletions(-) diff --git a/vortex-array/benches/scalar_fn_probe.rs b/vortex-array/benches/scalar_fn_probe.rs index 481ede71924..4c605a87eb7 100644 --- a/vortex-array/benches/scalar_fn_probe.rs +++ b/vortex-array/benches/scalar_fn_probe.rs @@ -3,16 +3,18 @@ #![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 std::sync::LazyLock; 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; @@ -26,11 +28,12 @@ fn main() { } const ARRAY_SIZE: usize = 100_000; -const NUM_ACCESSES: usize = 100; +const NUM_ACCESSES: usize = 50; static SESSION: LazyLock = LazyLock::new(array_session); -fn array() -> ArrayRef { +/// 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(); @@ -41,6 +44,18 @@ fn array() -> ArrayRef { .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) @@ -48,25 +63,21 @@ fn indices() -> Vec { .collect() } -#[divan::bench] -fn probe_scalar_fn_once(bencher: Bencher) { - let array = array(); +#[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.execute_scalar(index, ctx).unwrap()); + black_box(array.probe().execute_scalar(index, ctx).unwrap()); } }); } -#[divan::bench] -fn probe_scalar_fn_repeated(bencher: Bencher) { - let array = array(); +#[divan::bench(args = [binary_and(), binary_add()])] +fn probe_scalar_fn_repeated(bencher: Bencher, array: &ArrayRef) { let indices = indices(); - bencher .with_inputs(|| { ( @@ -81,3 +92,33 @@ fn probe_scalar_fn_repeated(bencher: Bencher) { } }); } + +#[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/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(()) + } } From 4ca7b44705b7ff12fefd05ed796b847df4565156 Mon Sep 17 00:00:00 2001 From: Mikhail Kot Date: Mon, 21 Sep 2026 11:42:54 +0100 Subject: [PATCH 3/3] Lazy ScalarFn validity --- vortex-array/src/array/probe/array.rs | 13 +- vortex-array/src/array/probe/repeated.rs | 14 +- .../src/arrays/scalar_fn/vtable/mod.rs | 142 ++++++++---------- .../src/arrays/scalar_fn/vtable/validity.rs | 84 ----------- vortex-array/src/expr/expression.rs | 7 +- vortex-array/src/scalar_fn/erased.rs | 13 +- vortex-array/src/scalar_fn/fns/binary/mod.rs | 14 +- 7 files changed, 98 insertions(+), 189 deletions(-) delete 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 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/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/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/scalar_fn/erased.rs b/vortex-array/src/scalar_fn/erased.rs index 8b25398c324..7b56887b6e8 100644 --- a/vortex-array/src/scalar_fn/erased.rs +++ b/vortex-array/src/scalar_fn/erased.rs @@ -22,14 +22,11 @@ use crate::dtype::DType; 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::SimplifyCtx; -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 e790bc4d18f..3e12da7e3bf 100644 --- a/vortex-array/src/scalar_fn/fns/binary/mod.rs +++ b/vortex-array/src/scalar_fn/fns/binary/mod.rs @@ -254,18 +254,12 @@ impl ScalarFnVTable for Binary { 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(match operator { - // AND and OR are kleene logic. - Operator::And => None, - Operator::Or => None, - _ => { - // All other binary operators are null if either side is null. - Some(and(lhs, rhs)) - } - }) + Ok(Some(and(lhs, rhs))) } fn is_strict(&self, operator: &Operator) -> bool {