diff --git a/src/core/query/parquet-pushdown.js b/src/core/query/parquet-pushdown.js index a2f7e585..ba5bb5a2 100644 --- a/src/core/query/parquet-pushdown.js +++ b/src/core/query/parquet-pushdown.js @@ -9,7 +9,11 @@ * rows itself. Note that the engine's own filter is two-valued for NULLs, * so declining is a correctness *fallback*, not a correctness *guarantee*: * on a nullable column it can still return rows SQL's three-valued logic - * excludes. Prefer a faithful filter over a decline where one exists. + * excludes, and it does so for every negation of an UNKNOWN subtree + * (`NOT (col LIKE 'a%')` over a nullable column, issue #734). Prefer a + * faithful filter over a decline where one exists: a predicate that is + * UNKNOWN for every row is faithfully pushable as hyparquet's never-match + * even though it looks like nothing worth pushing. * * Ported from the Hyperparam app (`lib/tools/parquetPushdownFilter.ts`), * which drives the same squirreling + hyparquet stack. The node-type @@ -58,6 +62,11 @@ function convertExpr(node, negate) { } /** + * @ref LLP 0098 [constrained-by]: the engine trusts `appliedWhere` and never + * re-judges a claimed predicate, so a predicate that matches nothing is worth + * converting rather than declining: the decline is not a no-op, it is a + * handoff to a two-valued filter that answers the negation wrong. + * * @param {BinaryNode} node * @param {boolean} negate * @returns {ParquetQueryFilter | undefined} @@ -84,17 +93,37 @@ function convertBinary(node, negate) { if (!leftFilter || !rightFilter) return undefined return negate ? { $and: [leftFilter, rightFilter] } : { $or: [leftFilter, rightFilter] } } - // LIKE has no parquet-filter equivalent; let the engine handle it. - if (op === 'LIKE') return undefined - const { column, value, flipped } = extractColumnAndValue(left, right) if (column === undefined || value === undefined) return undefined - // A comparison against a NULL literal (`col = NULL`, `col < NULL`) is - // UNKNOWN for every row under three-valued logic, so it matches nothing, - // NULL rows included. No hyparquet operator says "never match", so hand - // the predicate back to the engine rather than push a filter that would - // read as `IS NULL`. `IS NULL` itself goes through the unary path. - if (value === null) return undefined + // A comparison against a NULL literal (`col = NULL`, `col < NULL`, + // `NULL >= col`, `col LIKE NULL`) is UNKNOWN for every row under + // three-valued logic: no row is TRUE, NULL rows included, and no amount of + // negation rescues one, since `NOT UNKNOWN` is UNKNOWN. So it matches + // nothing whatever `negate` says, which is `$in: []`, the same never-match + // `convertInValues` pushes for `col NOT IN (…, NULL)` (see there for why + // hyparquet reads an empty `$in` as "no row, no row group"). + // + // Pushing beats declining even where the engine happens to agree. Its WHERE + // is two-valued (a comparison with a NULL operand is FALSE, not UNKNOWN, + // and unary `NOT` is JS `!`), so it answers `col = NULL` right by accident + // and every negation of it wrong: `NOT (col = NULL)` returned every row + // (issue #734). Pushing also prunes on row-group statistics, which the + // fallback never can. `IS NULL`, the predicate this shape gets mistaken + // for, goes through the unary path and is unaffected. + // + // Only comparisons and LIKE take this branch. Arithmetic and `||` against a + // NULL literal are never TRUE either, but they are values rather than + // predicates, and a WHERE made of one is exotic enough not to widen the + // claim for. + if (value === null) { + if (!isComparisonOp(op) && op !== 'LIKE') return undefined + return { [column]: { $in: [] } } + } + // LIKE against anything else has no parquet-filter equivalent; let the + // engine handle it. That fallback is only NULL-correct while the LIKE is + // not negated (issue #734 tracks the rest, which needs three-valued logic + // in the engine itself). + if (op === 'LIKE') return undefined const mongoOp = mapOperator(op, flipped, negate) if (!mongoOp) return undefined @@ -257,9 +286,19 @@ function convertInValues(node, negate) { // which is true, so every row group is skipped on statistics alone. Pushing // it is also what keeps the answer right: squirreling's own `WHERE` is // two-valued for NULLs, so handing the predicate back returns the very rows - // it excludes. (`col IN (…, NULL)` is fine: the NULL entry can never make - // the disjunction true, and the `$ne: null` guard below stops it from - // matching NULL rows.) + // it excludes. if (negate && values.some((value) => value === null)) return { [node.expr.name]: { $in: [] } } - return { [node.expr.name]: { $ne: null, [negate ? '$nin' : '$in']: values } } + // A NULL member of a list that is NOT negated is dropped instead. It can + // never make the disjunction TRUE (`col = NULL` is UNKNOWN for every row) + // and the guard below already excludes NULL rows, so the row set is + // identical either way, but carrying it costs pruning: `canSkipStats` orders + // every `$in` member against the chunk bounds through `compareParquetValues`, + // which returns `undefined` for a non-string against a BYTE_ARRAY bound, and + // one undecidable member makes the whole `every` fail. A string column + // filtered on `IN ('zz', NULL)` therefore reads every row group it could + // have skipped. When every member is NULL the list drops to `$in: []`, which + // is exactly what `col IN (NULL)` means. (Nothing is dropped on the negated + // path: a negated list holding a NULL returned above.) + const pushed = values.filter((value) => value !== null) + return { [node.expr.name]: { $ne: null, [negate ? '$nin' : '$in']: pushed } } } diff --git a/test/core/parquet-source.test.js b/test/core/parquet-source.test.js index cff4b2a9..02353d23 100644 --- a/test/core/parquet-source.test.js +++ b/test/core/parquet-source.test.js @@ -176,26 +176,67 @@ test('whereToParquetFilter returns undefined for non-convertible predicates', () test('whereToParquetFilter handles predicates whose SQL result is always UNKNOWN', () => { // Comparison against a NULL literal never matches a row, not even a NULL - // one. The engine keeps the predicate rather than the scan claiming a - // filter that reads like IS NULL. - assert.equal(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id = NULL')), undefined) - assert.equal(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id != NULL')), undefined) - assert.equal(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id < NULL')), undefined) - // NOT IN over a list containing NULL matches no row either, and that one is - // expressible: `$in: []` is hyparquet's never-match, and pushing it beats - // declining, whose fallback (squirreling's two-valued WHERE) returns rows. + // one, and `NOT UNKNOWN` is still UNKNOWN, so the whole family pushes + // `$in: []`, hyparquet's never-match. Declining instead would hand the + // predicate to squirreling's two-valued WHERE, which returns every row for + // the negated shapes (issue #734). + assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id = NULL')), { id: { $in: [] } }) + assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id != NULL')), { id: { $in: [] } }) + assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id < NULL')), { id: { $in: [] } }) + assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE NULL >= id')), { id: { $in: [] } }) + assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE NOT (id = NULL)')), { id: { $in: [] } }) + assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE NOT NOT (id = NULL)')), { id: { $in: [] } }) + assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE name LIKE NULL')), { name: { $in: [] } }) + assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE NOT (name LIKE NULL)')), { name: { $in: [] } }) + // NOT IN over a list containing NULL matches no row either: `$in: []` is + // hyparquet's never-match, and pushing it beats declining, whose fallback + // (squirreling's two-valued WHERE) returns rows. assert.deepEqual( whereToParquetFilter(whereOf('SELECT * FROM t WHERE id NOT IN (1, NULL)')), { id: { $in: [] } } ) - // IN over such a list is expressible too: the NULL entry cannot make the - // disjunction true, and the guard keeps NULL rows out. + // A NULL member of a non-negated list cannot make the disjunction true, so + // it is dropped: same rows, and the leaf keeps its statistics pruning. assert.deepEqual( whereToParquetFilter(whereOf('SELECT * FROM t WHERE id IN (1, NULL)')), - { id: { $ne: null, $in: [1n, null] } } + { id: { $ne: null, $in: [1n] } } + ) + assert.deepEqual( + whereToParquetFilter(whereOf('SELECT * FROM t WHERE id IN (NULL)')), + { id: { $ne: null, $in: [] } } ) }) +test('whereToParquetFilter pushes never-match only where the shape proves it', () => { + // A comparison against a non-NULL literal keeps its ordinary filter: the + // never-match is for UNKNOWN-for-every-row, not for "has a NULL somewhere". + assert.deepEqual( + whereToParquetFilter(whereOf('SELECT * FROM t WHERE NOT (id = 3)')), + { $and: [{ id: { $ne: null } }, { id: { $ne: 3n } }] } + ) + // A never-match leaf composes: it zeroes an AND and leaves an OR to its + // sibling, which is what Kleene logic says (UNKNOWN AND x is never TRUE, + // UNKNOWN OR x is TRUE exactly where x is). + assert.deepEqual( + whereToParquetFilter(whereOf('SELECT * FROM t WHERE id = NULL OR id = 3')), + { $or: [{ id: { $in: [] } }, { id: { $eq: 3n } }] } + ) + // The NULL literal has to sit opposite a plain column. Against an + // expression there is no leaf to name, and against another literal there is + // no column at all, so both keep declining. + assert.equal(whereToParquetFilter(whereOf('SELECT * FROM t WHERE NOT (id + 1 = NULL)')), undefined) + assert.equal(whereToParquetFilter(whereOf('SELECT * FROM t WHERE NOT (NULL = 1)')), undefined) + // The branch is for predicates, not values: an arithmetic or concat + // expression against a NULL literal still declines even though its column + // is a bare identifier. + assert.equal(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id + NULL')), undefined) + assert.equal(whereToParquetFilter(whereOf('SELECT * FROM t WHERE name || NULL')), undefined) + // ...and a negated LIKE over a real pattern stays declined (and, on a + // nullable column, stays SQL-wrong until the engine speaks three-valued + // logic: issue #734, option 1). + assert.equal(whereToParquetFilter(whereOf("SELECT * FROM t WHERE NOT (name LIKE 'a%')")), undefined) +}) + // --- NULL rows must not leak past a pushed-down filter ------------------------ // @ref LLP 0098 [tests]: the scan claims `appliedWhere` for @@ -262,6 +303,130 @@ test('comparison against a NULL literal matches no rows (issue #728)', async () assert.deepEqual(await mismatches(cases), []) }) +// A predicate the converter declines falls back to squirreling's WHERE, which +// is two-valued: a comparison with a NULL operand is FALSE rather than +// UNKNOWN, and unary NOT is JS `!`, so `NOT ` comes back TRUE. Every +// negation of a NULL-literal comparison therefore returned rows SQL excludes. +// These shapes are UNKNOWN for every row whatever the negation depth, so the +// converter pushes hyparquet's never-match instead of declining. +test('negated comparisons against a NULL literal match no rows (issue #734)', async () => { + /** @type {[string, number[]][]} */ + const cases = [ + // rows: ts = 100, NULL, 300, NULL, 500 + ['NOT (ts = NULL)', []], + ['NOT (ts != NULL)', []], + ['NOT (ts <> NULL)', []], + ['NOT (ts < NULL)', []], + ['NOT (ts <= NULL)', []], + ['NOT (ts > NULL)', []], + ['NOT (ts >= NULL)', []], + // literal on the left mirrors the operator but not the UNKNOWN + ['NOT (NULL = ts)', []], + ['NOT (NULL > ts)', []], + // NOT UNKNOWN is UNKNOWN, so negation depth never makes it TRUE + ['NOT NOT (ts = NULL)', []], + ['NOT NOT NOT (ts = NULL)', []], + // strings and LIKE are UNKNOWN against a NULL literal too + ['NOT (label = NULL)', []], + ['label LIKE NULL', []], + ['NOT (label LIKE NULL)', []], + // BETWEEN desugars to two comparisons, one of them against the NULL + ['ts BETWEEN NULL AND 500', []], + ['NOT (ts BETWEEN NULL AND 500)', []], + // These three are non-empty on purpose. The negated case above is empty + // only because `ts` maxes at 500, so its `ts > 500` conjunct is FALSE for + // every row: an accident of the data, not of the logic. (The non-negated + // case above is empty at any bound, since a never-match zeroes an $and.) + // A bound of 50, or moving the NULL to the upper bound, leaves rows whose + // non-NULL conjunct is FALSE rather than TRUE, so the negation matches. + // A bug that pushed never-match for the whole desugared AND, rather than + // only for the conjunct holding the NULL, would return [] and fail these. + ['NOT (ts BETWEEN NULL AND 50)', [1, 3, 5]], + ['ts NOT BETWEEN NULL AND 50', [1, 3, 5]], + ['NOT (ts BETWEEN 400 AND NULL)', [1, 3]], + // composition: UNKNOWN AND TRUE is UNKNOWN, UNKNOWN OR TRUE is TRUE + ['NOT (ts = NULL) AND ts >= 300', []], + ['NOT (ts = NULL) OR ts >= 300', [3, 5]], + ['NOT (ts = NULL OR ts = 300)', []], + // ...and the never-match branch must not swallow its sibling: NOT (a AND b) + // is TRUE wherever b is FALSE, however UNKNOWN a is + ['NOT (ts = NULL AND ts = 300)', [1, 5]], + ['ts IN (NULL)', []], + ] + assert.deepEqual(await mismatches(cases), []) +}) + +// The conservative direction. A predicate that is not UNKNOWN for every row +// must keep its ordinary filter (or keep declining), and the rows must still +// be the ones SQL names. +test('predicates that are not always-UNKNOWN keep their ordinary handling (issue #734)', async () => { + /** @type {[string, number[]][]} */ + const cases = [ + ['NOT (ts = 300)', [1, 5]], + ['NOT (ts = 300 OR ts = 500)', [1]], + ['NOT (ts IS NULL)', [1, 3, 5]], + ['ts IN (300, NULL)', [3]], + // declined subtrees the engine still gets right + ["label LIKE 'a%'", [1]], + ["label LIKE 'a%' OR ts > 300", [1, 5]], + ["NOT (label LIKE 'a%') AND ts IS NOT NULL", [3, 5]], + ] + assert.deepEqual(await mismatches(cases), []) +}) + +// The row set is the same either way, so only the read proves this one: a +// NULL member left in a non-negated `$in` list is undecidable against +// BYTE_ARRAY bounds, and one undecidable member forfeits statistics pruning +// for the whole leaf. Measure the bytes the scan pulls off the file. +test('a NULL member in an IN list does not cost row-group pruning (issue #734)', async () => { + const columnData = rowsToColumnSources(NULLABLE_COLUMNS, NULLABLE_ROWS) + const arrayBuffer = parquetWriteBuffer({ columnData, codec: 'SNAPPY', rowGroupSize: 2 }) + const bytes = new Uint8Array(arrayBuffer) + + /** + * @param {string} predicate + * @returns {Promise<{ read: number, ids: number[] }>} + */ + async function scanReading(predicate) { + const counting = asyncBufferFromBytes(bytes) + let read = 0 + const file = { + byteLength: counting.byteLength, + /** + * @param {number} start + * @param {number} [end] + */ + slice(start, end) { + read += (end ?? bytes.byteLength) - start + return counting.slice(start, end) + }, + } + const source = parquetDataSource(file, await parquetMetadataAsync(file)) + // Count only what the scan reads, not the metadata read above. + read = 0 + /** @type {number[]} */ + const ids = [] + const scan = source.scan({ columns: ['id'], where: whereOf(`SELECT id FROM t WHERE ${predicate}`) }) + assert.equal(scan.appliedWhere, true) + for await (const row of scan.rows()) ids.push(Number(await row.cells.id)) + return { read, ids } + } + + // No row group holds a label at or above 'zz', so every one is skippable. + const withNull = await scanReading("label IN ('zz', NULL)") + const withoutNull = await scanReading("label IN ('zz')") + const unfiltered = await scanReading('id >= 1') + + assert.deepEqual(withNull.ids, []) + assert.deepEqual(withoutNull.ids, []) + // Pruned to nothing, and the NULL member costs nothing. + assert.equal(withNull.read, withoutNull.read) + assert.ok( + withNull.read < unfiltered.read, + `pruned scan read ${withNull.read} bytes, unfiltered read ${unfiltered.read}` + ) +}) + /** * Run every predicate against a fresh nullable source and report the ones * whose result differs from SQL's. Reporting all of them at once, rather