Repository navigation
fix(array): between over a constant null array is null, not false - #10744
Open
gitRasheed wants to merge 1 commit into
Open
gitRasheed wants to merge 1 commit into
gitRasheed wants to merge 1 commit into
Conversation
The Constant between reduce rule compared the constant scalar with the bounds directly. Scalar orders null below every value, so an all-null chunk (which compression stores as a ConstantArray) gave a non-null false, and not(between(x, a, b)) over it selected every row instead of none. Return a null constant when the array constant is null. Signed-off-by: gitRasheed <99349713+gitRasheed@users.noreply.github.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In DataFusion,
a NOT IN (1, 2, 3, 4, 5)counted null rows that sit in an all-null chunk.Data:
idInt32 0..999.aInt32 nullable, null for rows 0..499 andid % 20for rows 500..999. Written as ten 100-row chunks (WriteStrategyBuilderwithwith_row_block_size(100)andwith_data_block_target_bytes(None)), so rows 0..499 are five all-null chunks.A DataFusion
MemTablewith the same data returns 375. On develop Vortex returns 875, so it counts all 500 null rows. With this change it returns 375.vortex-datafusion turns a NOT IN of consecutive integers into
not(between(...))(in_list_ofinvortex-array/src/expr/exprs.rs) and reports the filter as pushed, so DataFusion does not check it again. Each all-null chunk is aConstantArray. TheConstantBetweenReducerule compared it with the constant bounds usingScalar's ordering, which puts null below every value. Sobetweenreturnedfalseinstead of null, andnotturned that intotrue. In normal dispatch null bounds are handled earlier byshort_circuit, so a null array value is the case that got through.Changes
When the array constant is null, the
ConstantBetweenReducerule now returns a nullBoolconstant with the combined nullability of the array and both bounds.The new test
scalar_fn::fns::between::tests::constant_null_arraychecks thatbetweenandnot(between)are all null forf64andi32.Checked locally:
cargo test -p vortex-array --lib between(46 passed, and the new test fails on develop), the DataFusion query above (875 on develop, 375 with this change), clippy with-D warnings, and nightly fmt.I used AI tools to help write and review this change.