Guard pushed-down parquet filters against NULL rows (#728) - #730
Conversation
`whereToParquetFilter` emitted a bare relational operator, e.g.
`{ ts: { $lte: 300n } }`. hyparquet's `matchFilter` evaluates that with a
raw JavaScript comparison, and `null <= 300n` is true (null coerces to 0),
so every NULL row matched. `$ne` and `$nin` leak the same way by negating a
failed comparison. Because `parquetDataSource` sets `appliedWhere` from the
presence of a filter, the engine never re-filters and the NULL rows reach
the caller: a silent wrong answer on any `<`, `<=`, `!=`, `<>`, or `NOT IN`
predicate over a nullable column.
`>` and `>=` leak too. They only look correct in the issue's table because
a coerced NULL reads as 0 and loses to a positive bound; with a negative
bound (`neg > -400`) they return every NULL row.
Every operator whose evaluation can admit NULL now carries a `$ne: null`
conjunct. It rides inside the same condition object rather than an outer
`$and`, which also recovers pruning the bug was costing: `canSkipRowGroup`
and `filterPageRanges` refuse to skip on statistics for any condition a
NULL could satisfy, so a bare bound read the whole file. On a 5-row-group
fixture, `ts <= 500` went from 5 reads / 64791 bytes to 1 read / 12933.
The `$and` shape would have kept the unpruned behavior. `$ne` is the one
operator whose guard key collides with its own, so it takes the `$and`
form, which measurement shows costs no pruning.
Predicates that are UNKNOWN for every row under three-valued logic
(`col = NULL`, `col < NULL`, `col NOT IN (…, NULL)`) are no longer pushed
down at all: there is no hyparquet operator for "never match", and pushing
`$eq: null` made `col = NULL` behave like `IS NULL`.
The regression test drives a real `parquetDataSource` over a nullable
fixture and reports every mismatching predicate at once. Against the
pre-fix converter it lists 11 leaking predicates plus 3 wrong NULL-literal
results; after the fix the list is empty.
The NULL guard added for #728 stops at the leaves. `NOT (a OR b)` compiled to a `$nor` wrapper, and hyparquet's `matchFilter` evaluates `$nor` as a two-valued complement: a row that is UNKNOWN for every disjunct reads as "no child matched, therefore true" and is returned. SQL excludes it. The scan claims `appliedWhere` for any convertible predicate, so the engine never re-filters and those rows reach the caller. Pre-fix the bug hid itself: a bare `{ts: {$lt: 100n}}` matched the NULL row by coercion (`null < 100n` is `0 < 100`), which flipped `$nor` to exclude it. Adding `$ne: null` removed the accident and the NULL row escaped. Use De Morgan instead, as the AND branch already does: `NOT (a OR b)` is `(NOT a) AND (NOT b)` in Kleene 3VL, so each leaf carries its own guard. Nothing conservative is lost (a non-convertible child already forced `undefined` on both paths), and it is a pruning win: `canSkipRowGroup` and `filterPageRanges` bail out unconditionally on `$nor`, while `$and` prunes if any branch prunes. Also: note in the guardNulls JSDoc that per-leaf guards are only sound while every negation reaches a leaf; correct the stated reason for declining `NOT IN (..., NULL)` (it is FALSE for a listed value and UNKNOWN elsewhere, not UNKNOWN everywhere); and cite LLP 0098 as a whole rather than its `#wrapper-duties` section, which enumerates kernel scanColumn wrapper duties and does not cover this converter. Extends the #728 census with seven `NOT (a OR b)` cases over nullable columns; six of them fail on the previous commit.
|
Review round 1 of The guard is right, the compact shape is right, and the pruning claim reproduces. But the fix stopped at the leaves and turned three previously-correct queries into wrong-row queries. 1. blocker -
|
`col NOT IN (..., NULL)` matches no row under three-valued logic, and the
converter declined it on the grounds that no hyparquet operator says
"never match". That premise was wrong twice over.
It is expressible. `{col: {$in: []}}` never matches: `matchesIn` folds an
empty target list to `[].some(...)`, which is false. It also prunes
perfectly: `canSkipStats`'s `$in` branch folds to `[].every(...)`, which
is true, so every row group is skipped on statistics alone. Measured on a
1000-row / 5-row-group fixture, `$in: []` reads the footer and nothing
else (rows=0, slices=1, bytes=4144) against rows=1000, slices=6,
bytes=7879 unfiltered.
And declining was not safe. squirreling's own WHERE evaluation is
two-valued for NULLs, so handing the predicate back reproduced the very
leak #728 is about: `ts NOT IN (300, NULL)` returned [1,5] where SQL
returns []. Push the never-match filter instead.
The docstring's "let the SQL engine filter the rows itself" read as if
declining were a correctness guarantee. It is not, on a nullable column;
say so. Those remaining divergences are squirreling's to fix and out of
this converter's reach, so nothing else changes behaviour here.
Adds `ts NOT IN (300, NULL)` to the NULL-literal census, which failed on
the parent commit with got [1,5].
|
Review round 2 of The De Morgan rewrite is correct, and it was attacked hard. Across 600 randomly generated predicate trees (depth 3, mixing 1. minor - the reason for declining
|
|
Triage after the review budget (LLP 0017). Two rounds ran (5 findings, all fixed, including a blocker the fix itself introduced). Judged mergeable. Deferred items in #734. The triage did not take the review records on trust. It ran its own 71-predicate battery against an independently written Kleene-3VL evaluator, including cases the PR's own tests lack (zero bounds where a coerced NULL reads as 0, empty-string comparisons, double negation, The judgement call: the engine is now less correct than the pushdownThis PR makes convertible predicates fully NULL-correct while declined ones still fall through to squirreling's two-valued WHERE. That asymmetry is the one thing that could argue for holding, so here is the reasoning for shipping anyway:
Merge-time note, please read before touching #721#721 must not land as-written after this merges. icebird 0.8.15's Merge order (#730 first) is protective rather than a trap: |
philcunliffe
added
the
neutral:approved
…al fold icebird's converter still disagrees with SQL on NULLs (11 of 24 predicates in a three-valued battery), so adopting it wholesale would undo #730 and #743. Take the one thing it had that we lacked instead: constant-folding of typed literals, plus the truthiness gate on boolean-position casts. Folded bounds carry the same $ne: null guard plain bounds do. Renumber the LLP to 0219; 0212 is taken.
1 participant
Guards pushed-down parquet filters against NULL rows.
whereToParquetFilteremitted bare relational operators, and hyparquet'smatchFilterevaluates them with raw JS comparison, wherenull <= 300nis true. Sinceparquet-source.js:51setsappliedWhere = Boolean(filter), the engine never re-filtered and the NULL rows reached the caller.More operators leaked than the issue reported
$lt$ltenull <= 300nis true)$ne: nullguard$gt$gteneg > -400over-500, NULL, -300, NULL, -100returned2,3,4,5instead of3,5$ne: nullguard$ne(!=,<>)!equals(null, 300n))$and: [{col:{$ne:null}}, {col:{$ne:v}}]- its guard key collides with its own$nin(NOT IN)ts NOT IN (300)returned1,2,4,5$ne: nullguard$ints IN (300, NULL)returned2,3,4$ne: nullguard$eqequals(null, <non-null>, false)is already false, and$eq: nullis howIS NULLspells itselfThree predicates are UNKNOWN for every row under three-valued logic and are now not pushed down at all, since no hyparquet operator means "never match":
col = NULL,col <op> NULL, andcol NOT IN (..., NULL). Pre-fix,col = NULLpushed$eq: nulland behaved exactly likeIS NULL, returning the NULL rows.The shape matters, and it recovers pruning rather than costing it
The guard rides as a second key in the same condition object,
{col: {$ne: null, $lte: v}}, not the$andwrapping the issue suggested.matchFilterANDs the entries of a condition object, so both are correct - but they prune very differently.canSkipRowGroupcomputesmatchingNulls = matchFilter({value: null}, {value: condition})and refuses statistics-based skipping for any condition a NULL could satisfy;filterPageRangesdoes the same per page. A bare{ts: {$lte: 500n}}looks NULL-matching, so the bug was already forfeiting row-group and page skipping on every chunk holding a NULL.Measured on a 5-row-group x 1000-row file with a counting
AsyncBuffer, ts monotonic per group, 10% NULLs, only group 0 matching:Same result for a negative bound on
>. So the$andshape is correct but keeps the pre-fix un-pruned read; the compact shape is correct and 5x cheaper here. For$ne, which is forced into$and, the cost measured as zero: with no NULLs in the columnnullCount === 0makesmatchingNullsfalse and stats skipping still fires; with NULLs present neither form prunes. Bloom pruning is untouched, since it keys only off$eq/$inand neither target changed.Evidence
The regression tests were written first and run against the unmodified converter: 7 fail, each for the right reason (extra NULL rows). Independently re-derived by the reconciler.
After the fix: 18/18 in that file. Full suite 3978 pass / 0 fail / 1 pre-existing skip; typecheck clean;
npm run smoke -- local_parquet_exportok.Tests run end to end through a real
parquetDataSource, not by asserting filter shape alone, because the whole bug is that a plausible-looking filter behaves wrongly at evaluation time.Interaction with PR #721 - please read before merging either
#721 deletes this file and re-exports icebird's converter. It is currently
neutral:stuckbecause the same null hazard is newly reachable there via its typed-literal fold. This PR will conflict with it.The same guard must move upstream into icebird's
whereToParquetFilterbefore #721 can land, or merging it re-introduces this exact wrong-answer path. And the shape matters there too: icebird must use the compact{col: {$ne: null, <op>: v}}form, not$and, or it silently gives up row-group and page pruning as measured above.Deliberately not done
WHERE ts NOT IN (300, NULL)still returns rows ([1,5]instead of[]). This change correctly declines to push it down, so it falls to squirreling's engine, and the engine gets three-valuedNOT INwith a NULL list element wrong. That is a squirreling bug, not a pushdown one, and the answer is no worse than before, so it was left out of the test expectations rather than papered over in this layer.= NULLand!= NULLfall back the same way and the engine handles those correctly.Fixes #728