Push never-match for always-UNKNOWN predicates instead of declining to a two-valued engine (#734) - #743
Conversation
`whereToParquetFilter` declined every comparison against a NULL literal, so the predicate fell back to squirreling's WHERE. That engine is two-valued: a comparison with a NULL operand is FALSE rather than UNKNOWN, and unary `NOT` is JS `!`. It therefore answered `ts = NULL` right by accident and every negation of it wrong, `NOT (ts = NULL)` returning all five rows of the nullable fixture where SQL returns none. Under three-valued logic `col <cmp> NULL` is UNKNOWN for every row whichever operand holds the literal, and `NOT UNKNOWN` is UNKNOWN, so no negation depth rescues a row. That is a never-match, and `$in: []` already spells it here: `convertInValues` pushes it for `col NOT IN (..., NULL)`. Push it for the comparison family too (`=`, `==`, `!=`, `<>`, `<`, `<=`, `>`, `>=`, and `LIKE`), regardless of `negate`. `BETWEEN NULL AND x` rides along, since it desugars to two comparisons. The never-match composes as Kleene says it should: it zeroes an `$and` and leaves an `$or` to its sibling, so `NOT (ts = NULL AND ts = 300)` still returns the rows where the second conjunct is FALSE. Deliberately still declining, because neither is provably UNKNOWN for every row: a negated LIKE over a real pattern, and any negation over a subtree the converter cannot convert. Those stay SQL-wrong on a nullable column until the engine itself speaks three-valued logic (#734, option 1). Also still declining, for want of a leaf to name: a NULL literal compared against an expression rather than a bare column, and one compared against another literal. A NULL member of a non-negated `IN` list is now dropped rather than pushed. The rows are the same either way, but `compareParquetValues` cannot order `null` against a BYTE_ARRAY bound and one undecidable member fails the whole `canSkipStats` fold, so a string column filtered on `IN ('zz', NULL)` read 199 bytes of row groups it could have skipped entirely.
#734) - Add id + NULL / name || NULL assertions so the conservative-exclusions test reaches the op !== LIKE gate at parquet-pushdown.js:119 instead of short-circuiting on operand shape earlier in extractColumnAndValue. - Add non-empty BETWEEN NOT-negation cases so a bug that pushed never-match for the whole desugared AND, rather than only the NULL conjunct, would fail instead of coincidentally matching empty fixture data. - Relabel the convertBinary @ref to LLP 0098 as [constrained-by], matching guardNulls's identical citation of the same appliedWhere-trust fact.
|
Review round 1 of The reviewer built a hand-written Kleene 3VL reference evaluator over the squirreling AST and ran it against a 7-column fixture covering every physical type the writer emits, 5 rows with interleaved NULLs, 2-row row groups. Coverage: every operator x every type x both operand orders x 0/1/2/3 The 6 mismatches it found were all on declined predicates - the pre-existing over-return this PR explicitly leaves for option 1. Critically, the declining-sibling case does not leak a false empty: 1. minor - the conservative-exclusions test did not pin the new gate. FIXEDThe PR presents It matters because that gate is the only thing between "a NULL literal opposite a bare column" and "...under any Fixed with assertions where the operator declines rather than the operand shape, and verified both ways: with the guard deleted, test 7 now fails with 2. minor - both BETWEEN cases expected
|
The comment justifying the three non-empty BETWEEN cases misattributed why the two cases above them are empty. Only the negated one, `NOT (ts BETWEEN NULL AND 500)`, is a data accident: it converts to an $or whose non-NULL conjunct is `ts > 500`, FALSE for every row only because the fixture's `ts` maxes at 500. The non-negated case converts to an $and holding the never-match, so it is empty at any bound and for any data, and no choice of bound could make it discriminating. Also name the right conjunct: the third case, `NOT (ts BETWEEN 400 AND NULL)`, carries its NULL on the upper bound, not the `>= NULL` lower one, which is the side it is there to cover. Comment only; no assertion or bound changed.
|
Review round 2 of 1. minor - the comment justifying the new BETWEEN cases was itself inaccurate. FIXEDThat comment exists because round 1 found the original BETWEEN cases passing for the wrong reason, so its whole job is to record which case pins what. It was wrong in three ways:
As written it told the next reader that the non-negated case is a data-dependent coincidence, inviting either deleting it as redundant or "fixing" its bound. It is neither: it is the structural Fixed, and the replacement's own claims were measured rather than assumed - this being the second attempt at the comment, the fixer drove the real Bounds were swept at 500, 50, 100000 and -100000: the Verified from round 1
Also checked, clean
The head has moved to |
|
Triage after the review budget (LLP 0017). Two rounds ran (4 findings, all fixed). Judged mergeable. Deferred to #744. The backend asymmetry: the premise was empirically wrongI asked the triage whether this PR creates an inconsistency, since round 2 recorded that icebird declines NULL comparisons. That record is factually wrong, and correcting it resolves the question.
and So the backends already disagreed before this PR, in a worse pattern: for Filed as #744, along with two further defects in icebird's converter that this triage found: no NULL guards on inequalities (a bare Also checkedSilent-empty risk in deployment, not just fixtures. The only queries whose visible answer changes are the always-UNKNOWN family, and they change from "every row" to One shape the tests do not contain, checked directly: Partial-fix framing is right. The body states in bold that it does not close the issue, names option 1 as untouched, enumerates what stays SQL-wrong, and says One stale phrase to fix before merging (non-behavioural): the body's LLP section still says "one |
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
Pushes hyparquet's never-match for predicates that are UNKNOWN for every row, instead of declining them to an engine whose WHERE is two-valued and returns them all.
This implements option 2 of the three #734 lists, and does not close the issue. Option 1 (Kleene 3VL in squirreling's engine) is the correct-everywhere fix and is untouched here;
NOT (... LIKE ...)and other negations over non-convertible subtrees stay SQL-wrong on nullable columns. DeliberatelyRefs, notFixes.The bug
Declined predicates fall back to squirreling's engine, where
applyBinaryOpreturnsfalseon a NULL comparand and unaryNOTis JS!. SoNOT <anything UNKNOWN>evaluates true instead of UNKNOWN and returns rows SQL excludes:The shapes, and why each qualifies
SQL rule: any comparison with a NULL operand is UNKNOWN, and
NOT UNKNOWNis UNKNOWN - so no row is ever TRUE and no negation depth rescues one. That makes these safe to push as{col: {$in: []}}, the never-match already used forcol NOT IN (..., NULL):=,==,!=,<>,<,<=,>,>=), because the UNKNOWN comes from the operand, not the operator.NULL = col). The operator mirror is irrelevant when the result is UNKNOWN either way.LIKE NULLin both directions, under the same any-operand-NULL rule.NOTs, including zero and two. The non-negated ones were already answered correctly by the two-valued fallback, but pushing costs nothing, gains row-group pruning, and removes an asymmetry that would otherwise need defending.BETWEEN NULL AND x, which squirreling desugars to>= AND <=and so inherits the rule.col IN (NULL)where every member is NULL, via the list path.Composition was checked, not assumed: a never-match leaf zeroes an
$and(UNKNOWN AND anything is never TRUE) and defers to its sibling in an$or. SoNOT (ts = NULL AND ts = 300)correctly returns[1,5], not[].Deliberately left declining
Conservatism matters here, because a wrong never-match silently returns zero rows, which is worse than the current over-return.
NOT (col LIKE 'pattern')and any negation over a non-convertible subtree. These are UNKNOWN only for NULL rows, not for every row, so a never-match would be a wrong answer. They stay SQL-wrong until option 1.NOT (ts + 1 = NULL)): provably UNKNOWN-for-every-row, but there is no leaf column to hang$in: []on. Declines, asserted.NOT (NULL = 1)): same, no column at all. Declines, asserted.isComparisonOp(op) || op === 'LIKE'rather than onvalue === nullalone.The
IN-list nit, and it is stronger than the issue saidts IN (300, NULL)pushed{ts: {$ne: null, $in: [300n, null]}}. Matching is unaffected either way (the NULL disjunct is UNKNOWN for every row, and the$ne: nullguard already excludes NULL rows), so dropping NULL members is row-identical. Verified against the installed hyparquet:compareParquetValuesreturnsundefinedfor a non-string against a BYTE_ARRAY bound, and one undecidable member failscanSkipStats'severy. A string column filtered onlabel IN ('zz', NULL)read 199 bytes - more than the 109-byte unfiltered scan, since it must also readlabel- versus 0 bytes with the member dropped, every row group skipped on statistics. On INT64 the member happens to be harmless (nullcoerces to 0 and still orders).Evidence
Tests written first, run against the unmodified converter; independently re-derived by the reconciler:
After: 23/23. The conservative test passed pre-fix and post-fix, which is what makes it a baseline rather than a co-failure - it proves the shapes left declining still return SQL-correct rows.
Full suite 3998 pass / 0 fail / 1 pre-existing skip; typecheck clean;
llp-ref-hygiene11/11.LLP
LLP 0098 is Accepted and this does not contradict it: it settled that a source claims
appliedWhereonly for a faithful filter, and a never-match is faithful for an always-UNKNOWN predicate. Its "an unconvertible predicate such as LIKE leavesappliedWhere: false" consequence remains true. Following PR #730's precedent (which introduced this never-match and touched no LLP), no doc edit was made; one[constrained-by]ref was added onconvertBinarywith a gloss worth having - the decline is not a no-op, it is a handoff to a two-valued filter.Refs #734