Repository navigation
fix(reader): rewrite NOT out of the read predicate before pruning - #3416
Conversation
The reader fans a FileScanTask's predicate to the row-group metrics evaluator and the page-index evaluator, both conservative (inclusive) filters that cannot soundly negate a NOT node. The row-group evaluator's naive `!inner` flips a MIGHT_MATCH bound to CANT_MATCH. For NOT(x > 5) over a row group spanning [0, 10] that prunes a group which does contain matching rows (silent data loss). The page-index evaluator rejects NOT outright, aborting the read. A FileScanTask built or deserialized outside TableScan carries an un-normalized predicate, so a raw NOT reaches both. Rewrite NOT out of the predicate once, where it is first bound in the read pipeline, so every consumer sees a NOT-free predicate.
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
The fix itself looks right to me — normalizing NOT out of the predicate before the row-group and page-index evaluators run matches what Java's RewriteNot and PyIceberg do, and turning the row-group evaluator's not() into a hard Err instead of Ok(!inner) is the right backstop against the silent mispruning.
My one ask before merge is on the tests. The read test's doc says it exercises both the row-group and page-index evaluators, but with a single row group and default writer properties the row-group stage prunes first and the page-index path almost certainly never runs — so of the two failures this fixes, only the row-group half is actually demonstrated, and a regression in the page-index path would stay green. I'd like a test that genuinely forces the page-index path (and ideally a bit more NOT-shape coverage) before this goes in. Details inline. Once that's covered, I'll take another pass.
| /// A `FileScanTask` built or deserialized outside `TableScan` carries an | ||
| /// un-normalized predicate, so a raw `Not(...)` can reach the reader. Driving | ||
| /// it through a full read with row-group filtering and row selection enabled | ||
| /// exercises both the row-group and page-index evaluators, which negate NOT |
There was a problem hiding this comment.
The doc here says the test exercises both the row-group and page-index evaluators, but with a single row group and default writer props the row-group stage prunes first and the page-index path almost certainly never runs — there may be no column/offset index written at all. So of the two failures this fixes, only the row-group mispruning is actually under test.
Before merge I'd want a second read test that forces the page-index path — row_group_filtering_enabled(false), row_selection_enabled(true), a small data_page_row_count_limit so there are real data pages, and an assertion the file carries a column index. Run against pre-fix code it should fail with the NOT-unsupported error.
While you're in here, a couple more NOT shapes beyond NOT(x > 5) would be worth it — the ones where the negation isn't a simple flip: NOT(a AND b)/NOT(a OR b), NOT(x IS NULL), NOT(StartsWith), and a NOT arriving via the equality-delete combination.
There was a problem hiding this comment.
Added test_not_predicate_read_through_page_index, to test this.
▎ but with a single row group and default writer props the row-group stage prunes first and the page-index path almost certainly never runs
The default Arrow writer does emit a column index, so the page-index path runs whenever a row group survives (the new test exercises exactly that). Agreed it wasn't isolated in the original test, which is why the new one turns row-group filtering off.
▎ NOT(a AND b)/NOT(a OR b), NOT(x IS NULL), NOT(StartsWith), and a NOT arriving via the equality-delete combination
This PR only adds the single rewrite_not() call at the pipeline boundary; it doesn't change rewrite_not. The per-shape behavior lives in rewrite_not and is already unit-tested, so testing each shape end to end here would be at the wrong layer and duplicate it. The equality-delete combination is the one that actually touches reader code (the predicate is AND-ed with the delete predicate before the rewrite), so I can add that one if you'd like.
| iceberg_schema_ref.as_ref(), | ||
| ); | ||
|
|
||
| assert!(result.is_err()); |
There was a problem hiding this comment.
is_err() on its own can pass for the wrong reason — a field-id lookup or bind failure would satisfy it just as well, so this doesn't actually prove the NOT path is what got rejected. I'd pin it:
let err = result.unwrap_err();
assert_eq!(err.kind(), ErrorKind::Unexpected);
assert!(err.message().contains("NOT"));
|
need to rebase once #3425 merged |
2 participants
Which issue does this PR close?
What changes are included in this PR?
The reader fans a
FileScanTask's predicate to the row-group metrics evaluator and the page-index evaluator, both conservative (inclusive) filters that cannot soundly negate aNOTnode. The row-group evaluator's naive!innerflips aMIGHT_MATCHbound toCANT_MATCH. ForNOT(x > 5)over a row group spanning[0, 10]that prunes a group which does contain matching rows (silent data loss). The page-index evaluator rejects NOT outright, aborting the read. A FileScanTask built or deserialized outside TableScan carries an un-normalized predicate, so a raw NOT reaches both.Rewrite NOT out of the predicate once, where it is first bound in the read pipeline, so every consumer sees a NOT-free predicate.
Are these changes tested?
Added tests.