From bc198617d5539c1e6a0170a1434caddd81f0bb17 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 19:50:14 +0000 Subject: [PATCH 1/8] fix: `Between::validity` must not declare a strict conjunction `Between` is not strict. Execution falls back to `lower <= arr AND arr <= upper` under Kleene `AND`, so a null bound still yields a definite `false` when the other comparison is false. `validity` conjoined all three children, declaring a row null whenever any bound was null, including rows that execution resolves to `false`. `Binary` returns `None` for `Operator::And` precisely because Kleene `AND` has no derivable validity expression, and `Between` desugars to that same `And`. Return `None` so the expression is evaluated and its mask extracted. Narrowing to the `arr` child would also be unsound, since a row can be legitimately null while `arr` is valid. Add a test that a declared validity agrees with the mask of the executed result, which covers this class of defect beyond `Between`. Signed-off-by: "Connor" Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36 --- vortex-array/src/scalar_fn/fns/between/mod.rs | 62 +++++++++++++++++-- 1 file changed, 56 insertions(+), 6 deletions(-) diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index 9e8c4649b5d..809309dd33e 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -8,7 +8,6 @@ use std::fmt::Formatter; pub use kernel::*; use prost::Message; -use vortex_array::expr::and; use vortex_error::VortexResult; use vortex_error::vortex_bail; use vortex_proto::expr as pb; @@ -299,12 +298,12 @@ impl ScalarFnVTable for Between { fn validity( &self, _options: &Self::Options, - expression: &Expression, + _expression: &Expression, ) -> VortexResult> { - let arr = expression.child(0).validity()?; - let lower = expression.child(1).validity()?; - let upper = expression.child(2).validity()?; - Ok(Some(and(and(arr, lower), upper))) + // `Between` desugars to two comparisons combined with Kleene `AND`, which has no + // derivable validity expression: `null AND false` is `false`, so a null bound does not + // make the row null. `Binary` returns `None` for `Operator::And` for the same reason. + Ok(None) } fn is_strict(&self, _options: &Self::Options) -> bool { @@ -328,12 +327,15 @@ mod tests { use crate::VortexSessionExecute; use crate::arrays::BoolArray; use crate::arrays::DecimalArray; + use crate::arrays::PrimitiveArray; + use crate::arrays::StructArray; use crate::assert_arrays_eq; use crate::dtype::DType; use crate::dtype::DecimalDType; use crate::dtype::Nullability; use crate::dtype::PType; use crate::expr::between; + use crate::expr::col; use crate::expr::get_item; use crate::expr::lit; use crate::expr::root; @@ -344,6 +346,54 @@ mod tests { static SESSION: LazyLock = LazyLock::new(crate::array_session); + const NON_STRICT: BetweenOptions = BetweenOptions { + lower_strict: StrictComparison::NonStrict, + upper_strict: StrictComparison::NonStrict, + }; + + /// 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 + /// instead of being intercepted as a constant. + #[test] + fn validity_agrees_with_execution() -> VortexResult<()> { + let ctx = &mut SESSION.create_execution_ctx(); + let data = StructArray::from_fields(&[ + ( + "x", + PrimitiveArray::from_option_iter([Some(10), Some(10), Some(1)]).into_array(), + ), + ( + "lo", + PrimitiveArray::from_option_iter([None, None, Some(0)]).into_array(), + ), + ( + "hi", + PrimitiveArray::from_option_iter([Some(5), Some(50), Some(5)]).into_array(), + ), + ])? + .into_array(); + + let expr = between(col("x"), col("lo"), col("hi"), NON_STRICT); + + let executed = data + .clone() + .apply(&expr)? + .execute::(ctx)? + .opt_bool_vec(ctx); + let declared = data + .apply(&expr.validity()?)? + .execute::(ctx)? + .bool_vec(ctx); + + assert_eq!(executed, [Some(false), None, Some(true)]); + assert_eq!( + executed.iter().map(Option::is_some).collect::>(), + declared + ); + Ok(()) + } + #[test] fn is_not_strict() { let expr = between( From 317d4ed1b4d7e648ad2bdf600e1caaa52715ce6d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 19:55:33 +0000 Subject: [PATCH 2/8] fix: a constant null `Between` bound must not null already-false rows `precondition` returned an all-null `ConstantArray` whenever either bound was a constant null. Under Kleene `AND` a null bound only makes a row null when the other comparison is not already false, so this nulled rows the surviving bound had already falsified. Because the branch keys off `as_constant()`, the result was also encoding-dependent: an all-null chunk stored as a `PrimitiveArray` produced `[false, null]` while the same chunk compressed to a `ConstantArray` produced `[null, null]`. Compression encodes all-null chunks as constants, so the same predicate over the same data could disagree from chunk to chunk. This also made `find_between` non-value-preserving. It rewrites conjoined comparisons with literal bounds into `Between`, so a null literal reached `precondition` through the standard optimizer. Under `not(...)` the rewrite changed a query's row count, since `NOT FALSE` is `TRUE` while `NOT UNKNOWN` is `UNKNOWN`. Short-circuit to all null only when both bounds are null, which is the one case where no comparison can falsify a row. With a single null bound, desugar into the two comparisons combined with Kleene `AND`, since the kernels all require non-null constant bounds. Reuse that desugaring for the existing fallback in `between_canonical`. `test_constants` asserted only that no row was `true`, which held under both the old and the correct result, so tighten it to the exact values. Signed-off-by: "Connor" Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36 --- .../src/expr/transform/match_between.rs | 41 +++++++ .../src/scalar_fn/fns/between/kernel.rs | 8 +- vortex-array/src/scalar_fn/fns/between/mod.rs | 103 +++++++++++++++--- 3 files changed, 131 insertions(+), 21 deletions(-) diff --git a/vortex-array/src/expr/transform/match_between.rs b/vortex-array/src/expr/transform/match_between.rs index 56f03dbac4d..ca6c1cf6b2d 100644 --- a/vortex-array/src/expr/transform/match_between.rs +++ b/vortex-array/src/expr/transform/match_between.rs @@ -125,7 +125,18 @@ fn is_strict_comparison(op: Operator) -> Option { #[cfg(test)] mod tests { + use vortex_buffer::buffer; + use vortex_error::VortexResult; + use super::find_between; + use crate::IntoArray; + use crate::VortexSessionExecute; + use crate::array_session; + use crate::arrays::BoolArray; + use crate::arrays::StructArray; + use crate::dtype::DType; + use crate::dtype::Nullability; + use crate::dtype::PType; use crate::expr::and; use crate::expr::between; use crate::expr::col; @@ -134,9 +145,39 @@ mod tests { use crate::expr::lit; use crate::expr::lt; use crate::expr::lt_eq; + use crate::scalar::Scalar; use crate::scalar_fn::fns::between::BetweenOptions; use crate::scalar_fn::fns::between::StrictComparison; + /// A null literal bound must not change the values of the rewritten expression. Kleene `AND` + /// keeps a row false when the surviving comparison is false, so the rewrite cannot null it. + #[test] + fn test_null_literal_bound_is_value_preserving() -> VortexResult<()> { + let session = array_session(); + let ctx = &mut session.create_execution_ctx(); + let data = StructArray::from_fields(&[("x", buffer![10, 1].into_array())])?.into_array(); + + let null_lit = lit(Scalar::null(DType::Primitive( + PType::I32, + Nullability::Nullable, + ))); + let expr = and(gt_eq(col("x"), null_lit), lt_eq(col("x"), lit(5i32))); + + let before = data + .clone() + .apply(&expr)? + .execute::(ctx)? + .opt_bool_vec(ctx); + let after = data + .apply(&find_between(expr))? + .execute::(ctx)? + .opt_bool_vec(ctx); + + assert_eq!(before, [Some(false), None]); + assert_eq!(before, after); + Ok(()) + } + #[test] fn test_bad_match() { // An impossible expression diff --git a/vortex-array/src/scalar_fn/fns/between/kernel.rs b/vortex-array/src/scalar_fn/fns/between/kernel.rs index ee4fb688982..275ac9fe09f 100644 --- a/vortex-array/src/scalar_fn/fns/between/kernel.rs +++ b/vortex-array/src/scalar_fn/fns/between/kernel.rs @@ -70,7 +70,7 @@ where let lower = &children[1]; let upper = &children[2]; let arr = array.array().clone(); - if let Some(result) = precondition(&arr, lower, upper)? { + if let Some(result) = precondition(&arr, lower, upper, parent.options)? { return Ok(Some(result)); } ::between(array, lower, upper, parent.options) @@ -105,8 +105,10 @@ where let lower = &children[1]; let upper = &children[2]; let arr = array.array().clone(); - if let Some(result) = precondition(&arr, lower, upper)? { - return Ok(Some(result)); + if let Some(result) = precondition(&arr, lower, upper, parent.options)? { + // `precondition` may return a lazy `ScalarFn` array, which callers of the execution + // kernels do not expect, so apply it immediately. + return result.execute::(ctx).map(Some); } ::between(array, lower, upper, parent.options, ctx) } diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index 809309dd33e..dbb4aee596e 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -32,7 +32,6 @@ use crate::scalar_fn::ChildName; use crate::scalar_fn::ExecutionArgs; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; -use crate::scalar_fn::fns::binary::execute_boolean; use crate::scalar_fn::fns::operators::CompareOperator; use crate::scalar_fn::fns::operators::Operator; @@ -91,11 +90,16 @@ impl StrictComparison { /// /// Returns `Some(result)` if the precondition short-circuits the between operation /// (empty array, null bounds), or `None` if between should proceed with the -/// encoding-specific implementation. +/// encoding-specific implementation. Kernels can therefore rely on both bounds being +/// non-null. +/// +/// The returned array may be a lazy [`crate::arrays::ScalarFn`] array, which the caller must +/// execute if it needs a computed result. pub(super) fn precondition( arr: &ArrayRef, lower: &ArrayRef, upper: &ArrayRef, + options: &BetweenOptions, ) -> VortexResult> { let return_dtype = Bool(arr.dtype().nullability() | lower.dtype().nullability() | upper.dtype().nullability()); @@ -105,17 +109,42 @@ pub(super) fn precondition( return Ok(Some(Canonical::empty(&return_dtype).into_array())); } - if lower.as_constant().is_some_and(|v| v.is_null()) - || upper.as_constant().is_some_and(|v| v.is_null()) - { + let lower_null = lower.as_constant().is_some_and(|v| v.is_null()); + let upper_null = upper.as_constant().is_some_and(|v| v.is_null()); + + // A null bound falsifies nothing on its own: `Between` is not strict, and Kleene `AND` gives + // `null AND false = false`. So a row is null only when the surviving comparison is not + // already false, which means every row is null only when both bounds are null. + if lower_null && upper_null { return Ok(Some( ConstantArray::new(Scalar::null(return_dtype), arr.len()).into_array(), )); } + // With one null bound there is nothing for the kernels to do, since they all require + // non-null constant bounds. Hand back the two comparisons that `Between` stands for so that + // the surviving one can still falsify rows. + if lower_null || upper_null { + return desugar(arr, lower, upper, options).map(Some); + } + Ok(None) } +/// `Between` rewritten as the two comparisons it stands for, combined with Kleene `AND`. +/// +/// Returns a lazy array, so this is safe to call from a reduce rule. +fn desugar( + arr: &ArrayRef, + lower: &ArrayRef, + upper: &ArrayRef, + options: &BetweenOptions, +) -> VortexResult { + let lower_cmp = lower.binary(arr.clone(), options.lower_strict.to_operator())?; + let upper_cmp = arr.binary(upper.clone(), options.upper_strict.to_operator())?; + lower_cmp.binary(upper_cmp, Operator::And) +} + /// Between on a canonical array by directly dispatching to the appropriate kernel. /// /// Falls back to compare + boolean and if no kernel handles the input. @@ -126,8 +155,10 @@ fn between_canonical( options: &BetweenOptions, ctx: &mut ExecutionCtx, ) -> VortexResult { - if let Some(result) = precondition(arr, lower, upper)? { - return Ok(result); + if let Some(result) = precondition(arr, lower, upper, options)? { + // `precondition` may return a lazy `ScalarFn` array, which callers of `execute` do not + // expect, so apply it immediately. + return result.execute::(ctx); } // Try type-specific kernels @@ -145,15 +176,7 @@ fn between_canonical( // TODO(joe): return lazy compare once the executor supports this // Fall back to compare + boolean and - let lower_cmp = lower.clone().binary( - arr.clone(), - Operator::from(options.lower_strict.to_compare_operator()), - )?; - let upper_cmp = arr.clone().binary( - upper.clone(), - Operator::from(options.upper_strict.to_compare_operator()), - )?; - execute_boolean(lower_cmp, upper_cmp, Operator::And, ctx) + desugar(arr, lower, upper, options)?.execute::(ctx) } /// An optimized scalar expression to compute whether values fall between two bounds. @@ -494,8 +517,11 @@ mod tests { .execute::(ctx) .unwrap(); - let indices = to_int_indices(matches, ctx).unwrap(); - assert!(indices.is_empty()); + // The rows the lower bound already falsified stay false rather than becoming null. + assert_eq!( + matches.opt_bool_vec(ctx), + [None, None, Some(false), None, Some(false)] + ); // upper is a fixed constant let upper = ConstantArray::new(Scalar::from(2), 5).into_array(); @@ -535,6 +561,47 @@ mod tests { assert_eq!(indices, vec![0, 1, 2, 3, 4]); } + /// `Between` is not strict, so a null bound only makes a row null when the surviving + /// comparison is not already false. This must not depend on how the bound is encoded. + #[rstest] + #[case::primitive_nulls(PrimitiveArray::from_option_iter([None::, None]).into_array())] + #[case::constant_null( + ConstantArray::new( + Scalar::null(DType::Primitive(PType::I32, Nullability::Nullable)), + 2, + ) + .into_array() + )] + fn null_lower_bound(#[case] lower: ArrayRef) -> VortexResult<()> { + let ctx = &mut SESSION.create_execution_ctx(); + let array = buffer![10, 10].into_array(); + let upper = buffer![5, 50].into_array(); + + let result = between_canonical(&array, &lower, &upper, &NON_STRICT, ctx)? + .execute::(ctx)?; + + assert_eq!(result.opt_bool_vec(ctx), [Some(false), None]); + Ok(()) + } + + /// With both bounds null no comparison can falsify a row, so every row is null. + #[test] + fn both_bounds_null() -> VortexResult<()> { + let ctx = &mut SESSION.create_execution_ctx(); + let array = buffer![10, 10].into_array(); + let bound = ConstantArray::new( + Scalar::null(DType::Primitive(PType::I32, Nullability::Nullable)), + 2, + ) + .into_array(); + + let result = between_canonical(&array, &bound, &bound, &NON_STRICT, ctx)? + .execute::(ctx)?; + + assert_eq!(result.opt_bool_vec(ctx), [None, None]); + Ok(()) + } + #[test] fn test_between_decimal() { let ctx = &mut SESSION.create_execution_ctx(); From 1b9c015e8039b93632439669971f0a1a58c26ec7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 21:11:53 +0000 Subject: [PATCH 3/8] style: tighten the `Between` null-bound comments and tests Removes a contract stated three times. `precondition` documents that its result can be lazy, so the two call sites no longer repeat that in a comment. Replaces the copied null `ConstantArray` construction with a `null_i32s` test helper, and names why that encoding matters. Presents the `validity` test data as an annotated truth table rather than three columns, since the row is the unit the test reasons about. Renames `lower_null` to `lower_is_null` to read as the boolean it is, drops "may" and "should" from the docs, and adds the missing blank lines before returns. Signed-off-by: "Connor" Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36 --- .../src/expr/transform/match_between.rs | 3 + .../src/scalar_fn/fns/between/kernel.rs | 2 - vortex-array/src/scalar_fn/fns/between/mod.rs | 88 +++++++++---------- 3 files changed, 44 insertions(+), 49 deletions(-) diff --git a/vortex-array/src/expr/transform/match_between.rs b/vortex-array/src/expr/transform/match_between.rs index ca6c1cf6b2d..7cb427596f4 100644 --- a/vortex-array/src/expr/transform/match_between.rs +++ b/vortex-array/src/expr/transform/match_between.rs @@ -168,13 +168,16 @@ mod tests { .apply(&expr)? .execute::(ctx)? .opt_bool_vec(ctx); + let after = data .apply(&find_between(expr))? .execute::(ctx)? .opt_bool_vec(ctx); + // Row 0 is false rather than null because `$.x <= 5` falsifies it on its own. assert_eq!(before, [Some(false), None]); assert_eq!(before, after); + Ok(()) } diff --git a/vortex-array/src/scalar_fn/fns/between/kernel.rs b/vortex-array/src/scalar_fn/fns/between/kernel.rs index 275ac9fe09f..f74aeca61ed 100644 --- a/vortex-array/src/scalar_fn/fns/between/kernel.rs +++ b/vortex-array/src/scalar_fn/fns/between/kernel.rs @@ -106,8 +106,6 @@ where let upper = &children[2]; let arr = array.array().clone(); if let Some(result) = precondition(&arr, lower, upper, parent.options)? { - // `precondition` may return a lazy `ScalarFn` array, which callers of the execution - // kernels do not expect, so apply it immediately. return result.execute::(ctx).map(Some); } ::between(array, lower, upper, parent.options, ctx) diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index dbb4aee596e..e79dadaef4a 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -89,12 +89,14 @@ impl StrictComparison { /// Common preconditions for between operations that apply to all arrays. /// /// Returns `Some(result)` if the precondition short-circuits the between operation -/// (empty array, null bounds), or `None` if between should proceed with the +/// (empty array, null bounds), or `None` if between must proceed with the /// encoding-specific implementation. Kernels can therefore rely on both bounds being /// non-null. /// -/// The returned array may be a lazy [`crate::arrays::ScalarFn`] array, which the caller must -/// execute if it needs a computed result. +/// The result can be a lazy [`ScalarFn`] array, so a caller that needs a computed array +/// **must** execute it. +/// +/// [`ScalarFn`]: crate::arrays::ScalarFn pub(super) fn precondition( arr: &ArrayRef, lower: &ArrayRef, @@ -109,22 +111,20 @@ pub(super) fn precondition( return Ok(Some(Canonical::empty(&return_dtype).into_array())); } - let lower_null = lower.as_constant().is_some_and(|v| v.is_null()); - let upper_null = upper.as_constant().is_some_and(|v| v.is_null()); + let lower_is_null = lower.as_constant().is_some_and(|v| v.is_null()); + let upper_is_null = upper.as_constant().is_some_and(|v| v.is_null()); - // A null bound falsifies nothing on its own: `Between` is not strict, and Kleene `AND` gives - // `null AND false = false`. So a row is null only when the surviving comparison is not - // already false, which means every row is null only when both bounds are null. - if lower_null && upper_null { + // `Between` is not strict, and Kleene `AND` gives `null AND false = false`, so a null bound + // cannot falsify a row on its own. Every row is null only when both bounds are null. + if lower_is_null && upper_is_null { return Ok(Some( ConstantArray::new(Scalar::null(return_dtype), arr.len()).into_array(), )); } - // With one null bound there is nothing for the kernels to do, since they all require - // non-null constant bounds. Hand back the two comparisons that `Between` stands for so that - // the surviving one can still falsify rows. - if lower_null || upper_null { + // Every kernel requires non-null constant bounds, so a single null bound leaves nothing to + // dispatch to. Desugaring keeps the surviving comparison, which can still falsify rows. + if lower_is_null || upper_is_null { return desugar(arr, lower, upper, options).map(Some); } @@ -133,7 +133,7 @@ pub(super) fn precondition( /// `Between` rewritten as the two comparisons it stands for, combined with Kleene `AND`. /// -/// Returns a lazy array, so this is safe to call from a reduce rule. +/// The returned array is lazy, so a reduce rule can call this function. fn desugar( arr: &ArrayRef, lower: &ArrayRef, @@ -156,8 +156,6 @@ fn between_canonical( ctx: &mut ExecutionCtx, ) -> VortexResult { if let Some(result) = precondition(arr, lower, upper, options)? { - // `precondition` may return a lazy `ScalarFn` array, which callers of `execute` do not - // expect, so apply it immediately. return result.execute::(ctx); } @@ -323,9 +321,9 @@ impl ScalarFnVTable for Between { _options: &Self::Options, _expression: &Expression, ) -> VortexResult> { - // `Between` desugars to two comparisons combined with Kleene `AND`, which has no - // derivable validity expression: `null AND false` is `false`, so a null bound does not - // make the row null. `Binary` returns `None` for `Operator::And` for the same reason. + // `Between` desugars to two comparisons 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) } @@ -374,6 +372,12 @@ mod tests { upper_strict: StrictComparison::NonStrict, }; + /// `len` null `i32` values held as a [`ConstantArray`], which is what `as_constant` sees. + fn null_i32s(len: usize) -> ArrayRef { + let null = Scalar::null(DType::Primitive(PType::I32, Nullability::Nullable)); + ConstantArray::new(null, len).into_array() + } + /// 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 @@ -381,21 +385,15 @@ mod tests { #[test] fn validity_agrees_with_execution() -> VortexResult<()> { let ctx = &mut SESSION.create_execution_ctx(); - let data = StructArray::from_fields(&[ - ( - "x", - PrimitiveArray::from_option_iter([Some(10), Some(10), Some(1)]).into_array(), - ), - ( - "lo", - PrimitiveArray::from_option_iter([None, None, Some(0)]).into_array(), - ), - ( - "hi", - PrimitiveArray::from_option_iter([Some(5), Some(50), Some(5)]).into_array(), - ), - ])? - .into_array(); + + // x lo hi expected + // 10 null 5 false, since the upper bound alone falsifies the row + // 10 null 50 null, since neither bound falsifies the row + // 1 0 5 true + 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 expr = between(col("x"), col("lo"), col("hi"), NON_STRICT); @@ -404,6 +402,7 @@ mod tests { .apply(&expr)? .execute::(ctx)? .opt_bool_vec(ctx); + let declared = data .apply(&expr.validity()?)? .execute::(ctx)? @@ -414,6 +413,7 @@ mod tests { executed.iter().map(Option::is_some).collect::>(), declared ); + Ok(()) } @@ -562,16 +562,11 @@ mod tests { } /// `Between` is not strict, so a null bound only makes a row null when the surviving - /// comparison is not already false. This must not depend on how the bound is encoded. + /// comparison is not already false. This must not depend on how the bound is encoded, and + /// compression stores an all-null chunk as a [`ConstantArray`]. #[rstest] #[case::primitive_nulls(PrimitiveArray::from_option_iter([None::, None]).into_array())] - #[case::constant_null( - ConstantArray::new( - Scalar::null(DType::Primitive(PType::I32, Nullability::Nullable)), - 2, - ) - .into_array() - )] + #[case::constant_null(null_i32s(2))] fn null_lower_bound(#[case] lower: ArrayRef) -> VortexResult<()> { let ctx = &mut SESSION.create_execution_ctx(); let array = buffer![10, 10].into_array(); @@ -580,7 +575,9 @@ mod tests { let result = between_canonical(&array, &lower, &upper, &NON_STRICT, ctx)? .execute::(ctx)?; + // Row 0 stays false because the upper bound falsifies it on its own. assert_eq!(result.opt_bool_vec(ctx), [Some(false), None]); + Ok(()) } @@ -589,16 +586,13 @@ mod tests { fn both_bounds_null() -> VortexResult<()> { let ctx = &mut SESSION.create_execution_ctx(); let array = buffer![10, 10].into_array(); - let bound = ConstantArray::new( - Scalar::null(DType::Primitive(PType::I32, Nullability::Nullable)), - 2, - ) - .into_array(); + let bound = null_i32s(2); let result = between_canonical(&array, &bound, &bound, &NON_STRICT, ctx)? .execute::(ctx)?; assert_eq!(result.opt_bool_vec(ctx), [None, None]); + Ok(()) } From 99d03c9aa3f60d43a9410e9e36e10891fa852a52 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 21:18:49 +0000 Subject: [PATCH 4/8] style: rename `desugar` to `as_two_compares` `desugar` never said what it desugars into, and the term is off-register for an array compute crate. `Between` stands for two compares combined with Kleene `AND`, so the name now says that directly. Signed-off-by: "Connor" Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36 --- vortex-array/src/scalar_fn/fns/between/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index e79dadaef4a..86abff901dd 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -123,18 +123,18 @@ pub(super) fn precondition( } // Every kernel requires non-null constant bounds, so a single null bound leaves nothing to - // dispatch to. Desugaring keeps the surviving comparison, which can still falsify rows. + // dispatch to. The two compares keep the surviving bound, which can still falsify rows. if lower_is_null || upper_is_null { - return desugar(arr, lower, upper, options).map(Some); + return as_two_compares(arr, lower, upper, options).map(Some); } Ok(None) } -/// `Between` rewritten as the two comparisons it stands for, combined with Kleene `AND`. +/// The two compares that `Between` stands for, combined with Kleene `AND`. /// /// The returned array is lazy, so a reduce rule can call this function. -fn desugar( +fn as_two_compares( arr: &ArrayRef, lower: &ArrayRef, upper: &ArrayRef, @@ -174,7 +174,7 @@ fn between_canonical( // TODO(joe): return lazy compare once the executor supports this // Fall back to compare + boolean and - desugar(arr, lower, upper, options)?.execute::(ctx) + as_two_compares(arr, lower, upper, options)?.execute::(ctx) } /// An optimized scalar expression to compute whether values fall between two bounds. @@ -321,7 +321,7 @@ impl ScalarFnVTable for Between { _options: &Self::Options, _expression: &Expression, ) -> VortexResult> { - // `Between` desugars to two comparisons under Kleene `AND`, and `null AND false` is + // `Between` stands for two comparisons 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) From bbcc430e57cce932371abea7460d31bc4673b76a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 21:35:39 +0000 Subject: [PATCH 5/8] Mark the two sites that force the lazy `Between` fallback `precondition` can now return a lazy array, and the two execution paths force it because their callers expect a computed array. Both carry the same debt as the existing fallback TODO in `between_canonical`, so record it where the forcing happens. The reduce adaptor needs no marker, since a reduce rule can return a lazy array. Signed-off-by: "Connor" Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36 --- vortex-array/src/scalar_fn/fns/between/kernel.rs | 3 +++ vortex-array/src/scalar_fn/fns/between/mod.rs | 2 ++ 2 files changed, 5 insertions(+) diff --git a/vortex-array/src/scalar_fn/fns/between/kernel.rs b/vortex-array/src/scalar_fn/fns/between/kernel.rs index f74aeca61ed..a4e03af623f 100644 --- a/vortex-array/src/scalar_fn/fns/between/kernel.rs +++ b/vortex-array/src/scalar_fn/fns/between/kernel.rs @@ -106,6 +106,9 @@ where let upper = &children[2]; let arr = array.array().clone(); if let Some(result) = precondition(&arr, lower, upper, parent.options)? { + // TODO(joe): return the lazy array directly, blocked on the same executor support as + // the fallback in `between_canonical`. The reduce adaptor above already passes it + // through unexecuted, since a reduce rule can return a lazy array. return result.execute::(ctx).map(Some); } ::between(array, lower, upper, parent.options, ctx) diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index 86abff901dd..d7fc26312e2 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -156,6 +156,8 @@ fn between_canonical( ctx: &mut ExecutionCtx, ) -> VortexResult { if let Some(result) = precondition(arr, lower, upper, options)? { + // TODO(joe): return the lazy array directly, blocked on the same executor support as the + // fallback below. Only the single-null-bound case is lazy, so this forces it for now. return result.execute::(ctx); } From 581504f1d8e84419da4026bead34033f49d9302e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 21:40:15 +0000 Subject: [PATCH 6/8] Justify `Between::is_strict` returning `false` Every other non-strict scalar fn in this family explains its `false`, and the trait default is also `false`, so a bare override recorded no intent. The comment states the reason and points at `validity`, matching `Binary`. Signed-off-by: "Connor" Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36 --- vortex-array/src/scalar_fn/fns/between/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index d7fc26312e2..34a6c16cf6a 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -330,6 +330,8 @@ impl ScalarFnVTable for Between { } fn is_strict(&self, _options: &Self::Options) -> bool { + // `Between` stands for two compares under Kleene `AND`, so a null bound does not force a + // null row, which is consistent with `validity` returning `None` above. false } From 33cbc33133cf8162d14bdcbc91300bd50e8952a7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 21:46:23 +0000 Subject: [PATCH 7/8] style: sync the `Between` docs with the renamed fallback `between_canonical` still described the old `execute_boolean` fallback, so it now links [`as_two_compares`]. Trims the `is_strict` comment to point at `validity` instead of restating its reasoning, matching `Binary`, and settles on "compares" in both. Signed-off-by: "Connor" Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36 --- vortex-array/src/scalar_fn/fns/between/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index 34a6c16cf6a..0039d9e91ec 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -147,7 +147,7 @@ fn as_two_compares( /// Between on a canonical array by directly dispatching to the appropriate kernel. /// -/// Falls back to compare + boolean and if no kernel handles the input. +/// Falls back to [`as_two_compares`] if no kernel handles the input. fn between_canonical( arr: &ArrayRef, lower: &ArrayRef, @@ -323,15 +323,15 @@ impl ScalarFnVTable for Between { _options: &Self::Options, _expression: &Expression, ) -> VortexResult> { - // `Between` stands for two comparisons 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`. + // `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 { - // `Between` stands for two compares under Kleene `AND`, so a null bound does not force a - // null row, which is consistent with `validity` returning `None` above. + // Not strict for the same reason `validity` returns `None` above: under Kleene `AND` a + // null bound does not force a null row. false } From 41a7eaa5ab5740ffbf6992d4ee002ec93fc90197 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 22:04:15 +0000 Subject: [PATCH 8/8] Remove the unused `StrictComparison::to_compare_operator` Its last caller was the `between_canonical` fallback, which now builds its operators with `to_operator`. Nothing else in the workspace called it. Removing it also drops the `CompareOperator` import from the module. This is a public API removal on a type that eight crates use, so it is split out from the fix that orphaned it. Signed-off-by: "Connor" Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36 --- vortex-array/src/scalar_fn/fns/between/mod.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index 0039d9e91ec..a8df0214ae0 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -32,7 +32,6 @@ use crate::scalar_fn::ChildName; use crate::scalar_fn::ExecutionArgs; use crate::scalar_fn::ScalarFnId; use crate::scalar_fn::ScalarFnVTable; -use crate::scalar_fn::fns::operators::CompareOperator; use crate::scalar_fn::fns::operators::Operator; #[derive(Debug, Clone, PartialEq, Eq, Hash)] @@ -67,13 +66,6 @@ pub enum StrictComparison { } impl StrictComparison { - pub const fn to_compare_operator(&self) -> CompareOperator { - match self { - StrictComparison::Strict => CompareOperator::Lt, - StrictComparison::NonStrict => CompareOperator::Lte, - } - } - pub const fn to_operator(&self) -> Operator { match self { StrictComparison::Strict => Operator::Lt,