Skip to content

precondition and validity break Kleene AND semantics for Between null bounds #9212

Description

@connortsui20

Between is not strict. is_strict returns false, and execution falls back to lower <= arr AND arr <= upper with Kleene AND (vortex-array/src/scalar_fn/fns/between/mod.rs:148), so a null bound still produces a definite false when the other comparison is false.

SQL-92 defines X BETWEEN Y AND Z as shorthand for X >= Y AND X <= Z under three-valued logic, with no separate null rule. UNKNOWN AND FALSE is FALSE, so that fallback is the reference behavior. is_strict (:309) and BetweenStatsRewrite::falsify (vortex-array/src/stats/rewrite/builtins.rs:179) agree with it. validity and precondition do not, in two different ways.

The BetweenKernel implementations are not involved, since each requires constant bounds and precondition intercepts constant nulls before any of them run (vortex-array/src/arrays/primitive/compute/between.rs:29).

validity declares a strict function

validity (:298) conjoins all three children, so it declares a row null whenever any bound is null, including rows execution resolves to a definite false. With non-strict bounds, arr = [10, 10, 1], lo = [null, null, 0], hi = [5, 50, 5], where lo is a column rather than a literal so that precondition does not short-circuit:

row expected execution expr.validity()
arr=10, lo=null, hi=5 false false false, declares null
arr=10, lo=null, hi=50 null null false
arr=1, lo=0, hi=5 true true true

Execution is correct on every row. Binary returns None for Operator::And precisely because Kleene AND has no derivable validity expression (vortex-array/src/scalar_fn/fns/binary/mod.rs:256). Between desugars to that same And and then asserts a strict conjunction over it.

precondition returns wrong values for a constant null bound

precondition (:108) returns an all-null ConstantArray when either bound is a constant null. A null bound only makes a row null when the other comparison is not already false, so this branch nulls rows that the surviving bound has already falsified.

The defect does not depend on the SQL reading, because it makes the result encoding-dependent. With arr = [10, 10] and hi = [5, 50], the same logical lower column of two nulls:

lower encoding as_constant() result
PrimitiveArray of all nulls None [false, null]
ConstantArray(null) Some [null, null]

Compression encodes an all-null chunk as a ConstantArray, so the same predicate over the same data can produce different values from chunk to chunk.

find_between makes this reachable from a filter

find_between (vortex-array/src/expr/transform/match_between.rs:19) rewrites conjoined comparisons into Between and requires literal bounds (:103), so a null literal reaches precondition through the standard optimizer (vortex-array/src/expr/optimize.rs:130 and :218). Over x = [10, 1] the rewrite is not value-preserving:

expression before rewrite after rewrite
($.x >= null) and ($.x <= 5i32) [false, null] [null, null]
not(($.x >= null) and ($.x <= 5i32)) [true, null] [null, null]

A bare filter masks the first row, since it drops false and null alike. The negated form is not masked, because NOT FALSE is TRUE while NOT UNKNOWN is UNKNOWN. A filter over it retains row 0 before the rewrite and no rows after, so an optimizer pass changes the row count of a query.

Reproduction for the rewrite
let null_lit = lit(Scalar::null(DType::Primitive(PType::I32, Nullability::Nullable)));
let inner = and(gt_eq(col("x"), null_lit), lt_eq(col("x"), lit(5i32)));

// [Some(false), None] then [None, None]
for expr in [inner.clone(), find_between(inner.clone())] {
    let r = data.clone().apply(&expr).unwrap().execute::<BoolArray>(&mut ctx).unwrap();
    println!("{expr} = {:?}", r.opt_bool_vec(&mut ctx));
}

Resolution

validity should return None, matching Binary for Operator::And. Narrowing it to the arr child is also unsound: on the second row of the first table arr is valid but the result is legitimately null. ScalarFnVTable::validity supports None explicitly (vortex-array/src/scalar_fn/vtable.rs:180), since the expression is then evaluated and its mask extracted.

precondition should stop returning all null for a constant null bound. The short-circuit is valid only when no row can resolve to a definite false, which holds when both bounds are null, or when the surviving comparison is never false.

Both need to change together. On the constant path execution currently returns all null, which agrees with the strict validity, so fixing validity alone would make that path start to disagree.

Test gaps: test_constants (:420) asserts only that no row is true, which holds under both the all-null result and the correct [null, null, false, null, null]. match_between.rs has no null-literal case. No test asserts that a declared validity agrees with the mask of the executed result, which would cover this class of defect beyond Between.

The doc comment at :168 states that this expression will shortly be removed in favor of two comparisons combined with a logical AND, which resolves all three sites by construction. Open question: whether to fix precondition and find_between now, or to land only validity returning None and let the removal handle the rest, which leaves the rewrite defect live in the interim.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions