Skip to content

Commit 318a350

Browse files
authored
Rename scalar function strictness contract (#8930)
Changes `is_null_sensitive` to `is_strict` and flips all current implementations (as `is_strict` implies `!is_null_sensitive`). A function is strict only when any null input forces a null output and its return dtype propagates that nullability. Also fixes `Between` and `Dynamic` scalar functions as they were both set to strict when they should have been non-strict, so now they are `is_strict = false`. ## Rationale for this change Dictionary pushdown rewrites `f(dict(codes, values), constant)` as `dict(codes, f(values, constant))`. At a null code, the original evaluates `f(null, constant)`, while the rewritten dictionary is always null. This is only sound when nulling any one argument forces a null result while the other arguments remain unchanged. `is_null_sensitive` documented the unary case but left this multi-argument contract ambiguous. Kleene `AND`/`OR` show the difference: masking both inputs produces null, but `false AND null = false` and `true OR null = true`. That ambiguity caused the bug fixed in #8929. Additionally, we have noticed that a large majority of our scalar functions have strict semantics, and so in the future (hopefully soon) we can lift out a lot of the shared null logic. ## What changes are included in this PR? <details> <summary>Functions that are strict, previously treated as non-strict</summary> <br> These functions are strict, so the old null-sensitivity classification was overly conservative: - `ArrayExpr` and `RowIdx` inherited the old conservative default. These functions take no arguments, so the strictness condition does not apply. - `Mask` inherited the old conservative default, but a null input remains null and its mask argument is required to be non-nullable. - `VariantGet` inherited the old conservative default, but a null Variant input produces a null output. - `GetItem` and `Select` preserve the parent struct validity and propagate its nullability. - `Merge` only accepts non-nullable inputs, so the strictness law is vacuously satisfied for well-typed arguments. </details> <details> <summary>Functions that are non-strict, previously treated as strict</summary> <br> These functions are non-strict, so the old classification allowed unsound dictionary pushdown: - `Between`: its comparisons are combined with Kleene `AND`; for example, `10 BETWEEN null AND 5` is `false`, not null. - `DynamicComparison`: when the dynamic RHS is absent, it returns its configured default even when the LHS is null. </details> <details> <summary>Remaining non-strict functions</summary> <br> - `Binary` for Kleene `AND`/`OR` - `CaseWhen` and `Zip` - `Cast` - `FillNull`, `IsNull`, and `IsNotNull` - `ForeignScalarFnVTable` - `ListContains` - `Pack` - `RowEncode` and `RowSize` - `StatFn` </details> <details> <summary>Remaining strict functions</summary> <br> - `Binary` for all operators except `AND`/`OR` - `ByteLength`, `ExtStorage`, `Like`, `ListLength`, `ListSum`, and `Not` - `Literal`, `Root`, and `RowCount` (nullary) - `JsonToVariant` - `GeoContains`, `GeoDistance`, `GeoEnvelope`, and `GeoIntersects` - `CosineSimilarity`, `InnerProduct`, `L2Denorm`, and `L2Norm` </details> Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
1 parent 1c58493 commit 318a350

53 files changed

Lines changed: 280 additions & 181 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

docs/concepts/expressions.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ to the returned data.
77
## Scalar Functions
88

99
Expressions are defined as abstract scalar functions. These vtables define the signature of the function, properties
10-
such as whether the function is null-sensitive, and the actual logic for executing the function over input arrays.
10+
such as whether the function is strict, and the actual logic for executing the function over input arrays.
1111

1212
The built-in scalar functions can be found in the `vortex-array::expr` module, with additional use-case specific
1313
functions provided by integration and plugin crates.

encodings/parquet-variant/src/json_to_variant_tests.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ fn converts_json_extension_input() -> VortexResult<()> {
145145
#[test]
146146
fn dict_encoded_input_converts_each_row() -> VortexResult<()> {
147147
// A dictionary-encoded JSON column exercises the dict-pushdown / canonicalization path:
148-
// `json_to_variant` is not null-sensitive, so it pushes into the dict values (canonical
148+
// `json_to_variant` is strict, so it pushes into the dict values (canonical
149149
// JSON extension values) where the kernel fires; either way every row must convert correctly.
150150
let values = json_input(VarBinViewArray::from_iter_str(["1", "2"]).into_array())?;
151151
let codes = PrimitiveArray::from_iter([0u8, 1, 0, 1, 0]).into_array();

vortex-array/src/arrays/dict/compute/rules.rs

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -149,17 +149,18 @@ impl ArrayParentReduceRule<Dict> for DictionaryScalarFnValuesPushDownRule {
149149
return Ok(None);
150150
}
151151

152-
// If the scalar function is null-sensitive, then we cannot push it down to values if
153-
// we have any nulls in the codes.
152+
// Before this rewrite, a null code supplies null for this argument while the constant
153+
// arguments retain their values. After the rewrite, the null code masks the function's
154+
// result. Those are equivalent only for a strict function.
154155
if array.codes().dtype().is_nullable()
155156
&& !matches!(
156157
array.codes().validity()?,
157158
Validity::NonNullable | Validity::AllValid
158159
)
159-
&& sig.is_null_sensitive()
160+
&& !sig.is_strict()
160161
{
161162
tracing::trace!(
162-
"Not pushing down null-sensitive scalar function {} over dictionary with null codes {}",
163+
"Not pushing down non-strict scalar function {} over dictionary with null codes {}",
163164
parent.scalar_fn(),
164165
Dict.id(),
165166
);
@@ -182,11 +183,10 @@ impl ArrayParentReduceRule<Dict> for DictionaryScalarFnValuesPushDownRule {
182183
.into_array()
183184
.optimize()?;
184185

185-
// We can only push down null-sensitive functions when we have all-valid codes.
186-
// In these cases, we cannot have the codes influence the nullability of the output DType.
187-
// Therefore, we cast the codes to be non-nullable and then cast the dictionary output
188-
// back to nullable if needed.
189-
if sig.is_null_sensitive() && array.codes().dtype().is_nullable() {
186+
// A non-strict function reaches this point only when the codes are all valid, but their
187+
// dtype may still be nullable. Remove that declared nullability while rebuilding the
188+
// dictionary, then cast its output to the function's declared dtype.
189+
if !sig.is_strict() && array.codes().dtype().is_nullable() {
190190
let new_codes = array.codes().cast(array.codes().dtype().as_nonnullable())?;
191191
let new_dict = unsafe {
192192
DictArray::new_unchecked(new_codes, new_values)

vortex-array/src/arrays/scalar_fn/vtable/mod.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,4 +337,8 @@ impl scalar_fn::ScalarFnVTable for ArrayExpr {
337337
let validity_array = options.0.validity()?.to_array(options.0.len());
338338
Ok(Some(ArrayExpr.new_expr(FakeEq(validity_array), [])))
339339
}
340+
341+
fn is_strict(&self, _options: &Self::Options) -> bool {
342+
true
343+
}
340344
}

vortex-array/src/expr/analysis/labeling.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ use crate::expr::traversal::NodeExt;
1010
use crate::expr::traversal::NodeVisitor;
1111
use crate::expr::traversal::TraversalOrder;
1212

13+
/// Boolean labels keyed by each expression node in a tree.
14+
pub type BooleanLabels<'a> = HashMap<&'a Expression, bool>;
15+
1316
/// Label each node in an expression tree using a bottom-up traversal.
1417
///
1518
/// This function separates tree labeling into two distinct steps:

vortex-array/src/expr/analysis/mod.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,12 @@ pub mod annotation;
55
mod fallible;
66
pub mod immediate_access;
77
mod labeling;
8-
mod null_sensitive;
98
mod referenced_field_paths;
9+
mod strict;
1010

1111
pub use annotation::*;
1212
pub use fallible::label_is_fallible;
1313
pub use immediate_access::*;
1414
pub use labeling::*;
15-
pub use null_sensitive::BooleanLabels;
16-
pub use null_sensitive::label_null_sensitive;
1715
pub use referenced_field_paths::referenced_field_paths;
16+
pub use strict::label_strict;

vortex-array/src/expr/analysis/null_sensitive.rs

Lines changed: 0 additions & 62 deletions
This file was deleted.
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
// SPDX-FileCopyrightText: Copyright the Vortex contributors
3+
4+
use super::BooleanLabels;
5+
use super::labeling::label_tree;
6+
use crate::expr::Expression;
7+
8+
/// Label each expression with whether its entire subtree is strict.
9+
///
10+
/// A subtree is strict only when the node's scalar function and every child subtree are strict.
11+
/// See [`crate::scalar_fn::ScalarFnVTable::is_strict`] for the scalar-function contract.
12+
pub fn label_strict(expr: &Expression) -> BooleanLabels<'_> {
13+
label_tree(
14+
expr,
15+
|expr| expr.signature().is_strict(),
16+
|acc, &child| acc & child,
17+
)
18+
}
19+
20+
#[cfg(test)]
21+
mod tests {
22+
use super::*;
23+
use crate::expr::col;
24+
use crate::expr::eq;
25+
use crate::expr::is_null;
26+
use crate::expr::lit;
27+
28+
#[test]
29+
fn test_non_strict_with_is_null() {
30+
let expr = is_null(col("col1"));
31+
let labels = label_strict(&expr);
32+
33+
assert_eq!(labels.get(&expr), Some(&false));
34+
}
35+
36+
#[test]
37+
fn test_strict_expression() {
38+
let expr = eq(lit(4), lit(5));
39+
let labels = label_strict(&expr);
40+
41+
assert_eq!(labels.get(&expr), Some(&true));
42+
}
43+
44+
#[test]
45+
fn test_non_strict_child_makes_parent_subtree_non_strict() {
46+
let left = eq(lit(4), lit(5));
47+
let right = is_null(col("col2"));
48+
let expr = eq(left.clone(), right.clone());
49+
50+
let labels = label_strict(&expr);
51+
52+
assert_eq!(labels.get(&left), Some(&true));
53+
assert_eq!(labels.get(&right), Some(&false));
54+
assert_eq!(labels.get(&expr), Some(&false));
55+
}
56+
}

vortex-array/src/expr/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
//!
1313
//! Each node references a scalar function defined by a
1414
//! [`ScalarFnVTable`](crate::scalar_fn::ScalarFnVTable). The vtable declares the function signature,
15-
//! properties such as null-sensitivity, and the logic that executes it over input arrays. Built-in
15+
//! properties such as strictness, and the logic that executes it over input arrays. Built-in
1616
//! functions live in [`crate::scalar_fn`]; integration and plugin crates supply additional,
1717
//! use-case-specific functions.
1818
//!

vortex-array/src/scalar_fn/fns/between/mod.rs

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,7 @@ impl ScalarFnVTable for Between {
306306
Ok(Some(and(and(arr, lower), upper)))
307307
}
308308

309-
fn is_null_sensitive(&self, _instance: &Self::Options) -> bool {
309+
fn is_strict(&self, _options: &Self::Options) -> bool {
310310
false
311311
}
312312

@@ -343,6 +343,21 @@ mod tests {
343343

344344
static SESSION: LazyLock<VortexSession> = LazyLock::new(crate::array_session);
345345

346+
#[test]
347+
fn is_not_strict() {
348+
let expr = between(
349+
root(),
350+
lit(0),
351+
lit(100),
352+
BetweenOptions {
353+
lower_strict: StrictComparison::NonStrict,
354+
upper_strict: StrictComparison::NonStrict,
355+
},
356+
);
357+
358+
assert!(!expr.signature().is_strict());
359+
}
360+
346361
#[test]
347362
fn test_display() {
348363
let expr = between(

0 commit comments

Comments
 (0)