Repository navigation
[core] Negate predicates like Java so NULL rows stay excluded - #1145
JeonDaehong wants to merge 3 commits into
Conversation
|
@JingsongLi @leaves12138 could you take a look when you have time? Fixes #1143: |
|
Moved to draft until CI is green. I couldn't run the docker fixtures and the Go/Python suites locally. Will mark it ready once they pass. |
|
CI is green, ready for review. |
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS at 0a5bdd9. Fixing NULL rows being selected/deleted by negated predicates has clear end-to-end value. The ordinary NULL and added string-negation scenarios pass, but two additional public-API regression tests fail on this head and pass on base 9568f39: floating bitmap + vector Top-1 returns a row that violates the filter, and a NaN partition is silently excluded. Details are inline. Local verification also ran the core library suite (4146 passed, 6 ignored); after provisioning the actual Spark warehouse, all 358 DataFusion library tests passed. Both new regression tests were also run unchanged against the baseline worktree and passed there.
| Some(match self { | ||
| Self::IsNull => Self::IsNotNull, | ||
| Self::IsNotNull => Self::IsNull, | ||
| Self::Eq => Self::NotEq, |
There was a problem hiding this comment.
[P1] Refine floating bitmap candidates before ranking normalized NOT filters
Turning NOT(f = 1) into NotEq now enters the floating bitmap scalar-index path. BitmapGlobalIndexReader::query deliberately returns all non-null rows for floating NotEq, but matching_ranges_with_exactness labels this leaf exact, so scalar_matched_row_ids skips refinement before vector ranking. I reproduced this through public write/commit/index/search APIs: two rows have f=[1,2], vectors [[1,0],[0,1]], a bitmap index on f, and an ivf-flat index on the vectors. Searching [1,0] with limit 1 and Predicate::negate(equal(f,1)) returns row 0 (f=1), which violates the filter; expected row 1. The same test passes on base 9568f39a, and explicitly constructing raw Predicate::Not still returns row 1 on this head. Propagate floating bitmap candidate inexactness to the scanner and refine before ranking; add this filtered Top-K regression test.
There was a problem hiding this comment.
Thanks, reproduced. Fixed in eff224e in two places:
negatenow keepsNotfor FLOAT/DOUBLE comparisons (see the NaN comment), soNOT(f = 1)stays out of the bitmapNotEqpath.- The same wrong Top-1 also came from a plain
not_equal(f, 1)orgreater_than(f, 1.5), with no negation involved.is_exact_predicatenow treats FLOAT/DOUBLE leaves that the bitmap answers with every non-null row (is_bitmap_floating_residual_sensitive_op) as inexact, so they are refined before ranking.
floating_bitmap_candidates_are_refined_before_top_k covers all three filters. Before the change it returned row 0, now row 1.
| Self::IsNotNull => Self::IsNull, | ||
| Self::Eq => Self::NotEq, | ||
| Self::NotEq => Self::Eq, | ||
| Self::Lt => Self::GtEq, |
There was a problem hiding this comment.
[P2] Preserve NaN rows when negating floating partition comparisons
These operator flips assume complementary ordering, but native datum_cmp uses partial_cmp for FLOAT/DOUBLE, so NaN makes both < and >= false. I reproduced a scan with rows (id,f)=[(0,0.0),(1,NaN),(2,2.0)] and Predicate::negate(less_than(f,1.0)): this head returns [1,2] when f is a data column but only [2] when partitioned by f, because the normalized GtEq prunes the NaN partition. The identical test returns [1,2] in both layouts on base 9568f39a. Java's floating comparison and the Arrow residual path retain the NaN row here. Make native floating comparison compatible with those semantics, or retain NULL-aware NOT until the complements are sound, and cover partitioned/nonpartitioned NaN reads.
There was a problem hiding this comment.
Thanks, reproduced: the partitioned read returned [2]. I kept Not for FLOAT/DOUBLE comparisons in negate rather than changing datum_cmp, since that would also change plain < / > on NaN partitions, which seems like a separate change. Not(leaf) is false only for NULL, and NaN < 1.0 is false, so the NaN row is kept, as in Java.
negated_float_comparison_keeps_nan_rows checks the read and delete_by_predicate, partitioned and not.
0a5bdd9 to
eff224e
Compare
|
@JingsongLi thanks for the careful review. Both are fixed in eff224e, details inline. I also rebased onto main. After #1153, Checked with the ci.yml steps: fmt, clippy ( |
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS at eff224e. The prior floating-bitmap Top-1 and NaN-partition negation regressions are fixed on their covered paths; both added regression tests pass. A new public-API BTree + vector-search probe now returns no rows on this head and passes unchanged on baseline 46e6871. Details inline.
Local validation: 4,250 core library tests, 14 native predicate read/delete tests, 358 DataFusion library tests, 46 SQL read tests, and 5 C API checks passed. The actual Spark warehouse was provisioned. Two additional SQL reads fail only because liblumina_py.dylib is unavailable; lumina-data 0.1.0 supplies only a Linux x86_64 wheel, so these two native-library checks could not be completed on this macOS ARM host. Formatting and clean integration with current main were also checked.
| && !(matches!(data_type, DataType::Float(_) | DataType::Double(_)) | ||
| && super::bitmap_global_index_format::is_bitmap_floating_residual_sensitive_op(*op)) |
There was a problem hiding this comment.
[P2] Keep exact floating BTree answers exact
This bitmap-specific downgrade currently applies to BTree results as well. BTree already implements floating != and ranges using the same bit-preserving total ordering as the residual filter, so those answers are exact. With the supported global-index.filter.refine-from-data=false option, scalar_matched_row_ids excludes all candidates that this method marks inexact. I reproduced the regression using actual Parquet data with finite f=[1.0, 2.0], a scalar BTree on f, an ivf-flat vector index, and filtered Top-1 with f != 1.0: baseline 46e6871 returns id 1, while this head returns no rows. No NaN or predicate negation is involved. Please derive exactness from the contributing index entries/readers and downgrade the floating bitmap fallback specifically, retaining exact BTree results; add a regression with refinement disabled.
There was a problem hiding this comment.
Right, the downgrade was keyed on the predicate, not on the index that answered it. Fixed in ae3660e. evaluate_leaf now marks a leaf inexact only when a bitmap entry answered a floating range or negation, so BTree results stay exact. I reverted the check in is_exact_predicate.
floating_bitmap_candidates_are_refined_before_top_k now also runs on BTree, with and without refine-from-data=false. Before this change, BTree + refine-from-data=false returned [].
I also rebased onto current main.
Predicate::negate wrapped leaves in Not, which was evaluated as !inner, so NOT(p = 'a') selected rows where p is NULL. Reads returned them and delete_by_predicate deleted them. Flip the leaf operator and push through AND/OR like Java Predicate.negate. Leaves without a negated operator stay as Not, which now evaluates to false for NULL input. Closes apache#1143
…ates Negating a FLOAT/DOUBLE comparison keeps Not, since NaN makes the flipped operator non-complementary. Scalar index exactness treats FLOAT/DOUBLE leaves that the bitmap index answers with every non-null row as inexact, so filtered Top-K refines them before ranking.
Only a bitmap answers floating ranges and negations with every non-null row, so mark a leaf inexact only when a bitmap entry answered it.
eff224e to
ae3660e
Compare
Purpose
Linked issue: close #1143
Predicate::negatewrapped leaves inNot, andNotwas evaluated as!inner, so a NULL input madeNOT(p = 'a')true. Reads returned those rows anddelete_by_predicatedeleted them. Java flips the leaf instead (Equal.negate()isNotEqual), which is false for NULL.Brief change log
Predicate::negatenow does what JavaPredicate.negatedoes: flip the leaf operator (=/!=,</>=,<=/>,IS NULL/IS NOT NULL,IN/NOT IN,BETWEEN/NOT BETWEEN) and push through AND/OR with De Morgan.scan.rsnow matches the new shape of the negated filter (x < lo OR x > hiinstead ofNOT(x >= lo AND x <= hi)).STARTS_WITH,ENDS_WITH,CONTAINS,LIKE, array ops) stay asNot(leaf).eval_rowand the residual row filter now return false for those when the input is NULL, same as JavaNotStartsWith/NotLike/... .Tests
negated_predicates_keep_null_rowsandnegated_data_predicate_keeps_null_rowscover the cases from the issue on the partition and DV delete paths and on reads. Both fail on main.negateand for NULL evaluation ineval_rowand the residual filter.test_file_index_nested_not_with_added_column_falls_backnow expects[1]instead of[1, 2].addedis NULL in the old file, so for id 2 the predicate isNOT(true AND unknown). Java drops that row too.test_not/test_display_not, three DataFusion pushdown tests, the global index exactness test and the stats fail-open test asserted the oldNotshape. The last two now buildPredicate::Notdirectly so they keep testing theNotbranch.fmt and clippy (1.98.0,
-D warnings) pass.cargo test --no-fail-fast -p paimon -p paimon-datafusion -p paimon-c: 5804 passed. The 39 failures are allTableNotExist/ table not found for the fixture tables CI provisions, plus 2 that need the lumina library. The ones I checked fail the same way on main.Not covered here: a
Predicate::Notaround AND/OR built by hand (not throughnegate) is still evaluated as!inner. Nothing in core builds that today, but I can add it if you'd like.API and Format
No API change.
Predicate::negatereturns a different but equivalent tree for non-NULL inputs.Documentation
None.