diff --git a/src/core/query/parquet-pushdown.js b/src/core/query/parquet-pushdown.js index 89a364cd..a2f7e585 100644 --- a/src/core/query/parquet-pushdown.js +++ b/src/core/query/parquet-pushdown.js @@ -6,7 +6,10 @@ * the predicate down to the parquet reader. Returns `undefined` whenever * the expression cannot be fully and faithfully converted. The caller * must then leave `appliedWhere` false and let the SQL engine filter the - * rows itself. + * 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. * * Ported from the Hyperparam app (`lib/tools/parquetPushdownFilter.ts`), * which drives the same squirreling + hyparquet stack. The node-type @@ -69,23 +72,83 @@ function convertBinary(node, negate) { return negate ? { $or: [leftFilter, rightFilter] } : { $and: [leftFilter, rightFilter] } } if (op === 'OR') { - // `$nor` already expresses NOT(a OR b), so the children are converted - // un-negated and the wrapper carries the negation, propagating - // `negate` into them as well would double-negate. - const leftFilter = convertExpr(left, false) - const rightFilter = convertExpr(right, false) + // De Morgan: NOT (a OR b) === (NOT a) AND (NOT b), which holds in Kleene + // three-valued logic too. The obvious `$nor` wrapper does not: hyparquet + // evaluates it as a two-valued complement, reporting "no child matched" + // as a match, so a row that is UNKNOWN for every disjunct sails past + // every leaf guard. Pushing the negation into the children instead lets + // each leaf carry its own `$ne: null`, and `$and` prunes on row-group + // statistics where `$nor` never can. + const leftFilter = convertExpr(left, negate) + const rightFilter = convertExpr(right, negate) if (!leftFilter || !rightFilter) return undefined - return negate ? { $nor: [leftFilter, rightFilter] } : { $or: [leftFilter, rightFilter] } + 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 const mongoOp = mapOperator(op, flipped, negate) if (!mongoOp) return undefined - return { [column]: { [mongoOp]: value } } + return guardNulls(column, mongoOp, value) +} + +/** + * Add the NULL guard a relational or inequality operator needs. + * + * hyparquet's `matchFilter` evaluates `$lt`/`$lte`/`$gt`/`$gte` with raw + * JavaScript relational operators, which coerce a NULL column value to `0`: + * `null <= 300n` and `null > -400n` are both true, so NULL rows sail past a + * bare bound (`>` and `>=` only look safe because a positive bound beats 0). + * `$ne` negates a failed equality, so NULL passes it too. SQL three-valued + * logic rejects every one of those rows. `convertInValues` applies the same + * guard to `$in`/`$nin` for the same reason. + * + * The guard is a `$ne: null` conjunct, and it rides inside the same condition + * object rather than an outer `$and` because `canSkipRowGroup` and + * `filterPageRanges` disable statistics pruning for any condition a NULL + * value could satisfy: a bare `{col: {$lte: v}}` reads as NULL-matching and + * forfeits row-group and page skipping on every chunk holding a NULL, and so + * does `{$and: [{col: {$ne: null}}, {col: {$lte: v}}]}`, whose bound is still + * bare inside its own branch. `{col: {$ne: null, $lte: v}}` prunes. + * + * `$ne` is the one operator whose guard key collides with its own, so it + * takes the `$and` form. That costs no pruning in practice: hyparquet only + * skips on `$ne` when a chunk is constant at the excluded value, and it + * already declines to skip such a chunk once the column has NULLs in it. + * + * `$eq` needs no guard: `equals(null, )` is already false, and + * `$eq: null` is exactly how the `IS NULL` path spells itself. + * + * A per-leaf guard is only sound because every negation is pushed down to a + * leaf: `convertBinary` uses De Morgan for both `AND` and `OR`, and each leaf + * absorbs the negation itself (`mapOperator` for comparisons, `convertExpr` + * for `IS NULL` / `IS NOT NULL`, `convertInValues` for `IN`). Any wrapper + * that complements a subtree wholesale, such as `$nor`, evaluates two-valued + * and hands back the rows its children left UNKNOWN, defeating the guards + * underneath it. Keep negation at the leaves. + * + * @ref LLP 0098 [constrained-by]: pushdown may only claim + * `appliedWhere` for a filter that is faithful to SQL semantics; the engine + * never re-filters a claimed predicate, so a leak here is a wrong answer. + * + * @param {string} column + * @param {'$lt' | '$lte' | '$gt' | '$gte' | '$eq' | '$ne'} mongoOp + * @param {SqlPrimitive} value + * @returns {ParquetQueryFilter} + */ +function guardNulls(column, mongoOp, value) { + if (mongoOp === '$eq') return { [column]: { $eq: value } } + if (mongoOp === '$ne') return { $and: [{ [column]: { $ne: null } }, { [column]: { $ne: value } }] } + return { [column]: { $ne: null, [mongoOp]: value } } } /** @@ -186,5 +249,17 @@ function convertInValues(node, negate) { if (val.type !== 'literal') return undefined values.push(coerceBigInt(val.value)) } - return { [node.expr.name]: { [negate ? '$nin' : '$in']: values } } + // `col NOT IN (…, NULL)` matches no row: it is FALSE for a row equal to one + // of the listed values and UNKNOWN for every other row (no value can be + // proven distinct from NULL), so no row is TRUE. `$in: []` is hyparquet's + // never-match: `matchesIn` folds an empty target list to `[].some(…)`, which + // is false, and `canSkipStats`'s `$in` branch folds it to `[].every(…)`, + // 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.) + if (negate && values.some((value) => value === null)) return { [node.expr.name]: { $in: [] } } + return { [node.expr.name]: { $ne: null, [negate ? '$nin' : '$in']: values } } } diff --git a/test/core/parquet-source.test.js b/test/core/parquet-source.test.js index f112a9c6..7e3accbb 100644 --- a/test/core/parquet-source.test.js +++ b/test/core/parquet-source.test.js @@ -32,6 +32,28 @@ const ROWS = [ { id: 5, name: 'eve', score: 5.5 }, ] +/** + * A nullable-column fixture: `ts` straddles the bound with NULLs either side, + * `neg` is the same shape with negative values (where `>` / `>=` leak, since + * a coerced NULL reads as 0), and `label` proves the same for strings. + * + * @type {ColumnSpec[]} + */ +const NULLABLE_COLUMNS = [ + { name: 'id', type: 'INT64', nullable: false }, + { name: 'ts', type: 'INT64', nullable: true }, + { name: 'neg', type: 'INT64', nullable: true }, + { name: 'label', type: 'STRING', nullable: true }, +] + +const NULLABLE_ROWS = [ + { id: 1, ts: 100, neg: -500, label: 'a' }, + { id: 2, ts: null, neg: null, label: null }, + { id: 3, ts: 300, neg: -300, label: 'c' }, + { id: 4, ts: null, neg: null, label: null }, + { id: 5, ts: 500, neg: -100, label: 'e' }, +] + /** * @param {Uint8Array} bytes * @returns {AsyncBuffer} @@ -62,6 +84,19 @@ async function makeSource() { return parquetDataSource(file, metadata) } +/** + * Same, over `NULLABLE_ROWS`. + * + * @returns {Promise} + */ +async function makeNullableSource() { + const columnData = rowsToColumnSources(NULLABLE_COLUMNS, NULLABLE_ROWS) + const arrayBuffer = parquetWriteBuffer({ columnData, codec: 'SNAPPY', rowGroupSize: 2 }) + const file = asyncBufferFromBytes(new Uint8Array(arrayBuffer)) + const metadata = await parquetMetadataAsync(file) + return parquetDataSource(file, metadata) +} + /** * @param {string} sql * @returns {ExprNode | undefined} @@ -83,36 +118,51 @@ async function run(source, query) { test('whereToParquetFilter converts simple comparisons (integers coerced to bigint)', () => { assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id = 3')), { id: { $eq: 3n } }) - assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id > 3')), { id: { $gt: 3n } }) - assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id <= 3')), { id: { $lte: 3n } }) + assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id > 3')), { id: { $ne: null, $gt: 3n } }) + assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id <= 3')), { id: { $ne: null, $lte: 3n } }) assert.deepEqual(whereToParquetFilter(whereOf("SELECT * FROM t WHERE name = 'bob'")), { name: { $eq: 'bob' } }) }) test('whereToParquetFilter mirrors flipped operands (literal on the left)', () => { - assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE 3 < id')), { id: { $gt: 3n } }) - assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE 3 >= id')), { id: { $lte: 3n } }) + assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE 3 < id')), { id: { $ne: null, $gt: 3n } }) + assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE 3 >= id')), { id: { $ne: null, $lte: 3n } }) }) test('whereToParquetFilter handles AND / OR / NOT', () => { assert.deepEqual( whereToParquetFilter(whereOf('SELECT * FROM t WHERE id >= 2 AND id <= 4')), - { $and: [{ id: { $gte: 2n } }, { id: { $lte: 4n } }] } + { $and: [{ id: { $ne: null, $gte: 2n } }, { id: { $ne: null, $lte: 4n } }] } ) assert.deepEqual( whereToParquetFilter(whereOf('SELECT * FROM t WHERE id = 1 OR id = 2')), { $or: [{ id: { $eq: 1n } }, { id: { $eq: 2n } }] } ) - assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE NOT (id = 1)')), { id: { $ne: 1n } }) - // De Morgan: NOT (a OR b) -> $nor of the un-negated children + assert.deepEqual( + whereToParquetFilter(whereOf('SELECT * FROM t WHERE NOT (id = 1)')), + { $and: [{ id: { $ne: null } }, { id: { $ne: 1n } }] } + ) + // De Morgan: NOT (a OR b) -> $and of the negated children, never `$nor`, + // whose two-valued complement matches the rows its children left UNKNOWN assert.deepEqual( whereToParquetFilter(whereOf('SELECT * FROM t WHERE NOT (id = 1 OR id = 2)')), - { $nor: [{ id: { $eq: 1n } }, { id: { $eq: 2n } }] } + { + $and: [ + { $and: [{ id: { $ne: null } }, { id: { $ne: 1n } }] }, + { $and: [{ id: { $ne: null } }, { id: { $ne: 2n } }] }, + ], + } ) }) test('whereToParquetFilter handles IN / NOT IN / IS NULL', () => { - assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id IN (1, 2)')), { id: { $in: [1n, 2n] } }) - assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE id NOT IN (1, 2)')), { id: { $nin: [1n, 2n] } }) + assert.deepEqual( + whereToParquetFilter(whereOf('SELECT * FROM t WHERE id IN (1, 2)')), + { id: { $ne: null, $in: [1n, 2n] } } + ) + assert.deepEqual( + whereToParquetFilter(whereOf('SELECT * FROM t WHERE id NOT IN (1, 2)')), + { id: { $ne: null, $nin: [1n, 2n] } } + ) assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE name IS NULL')), { name: { $eq: null } }) assert.deepEqual(whereToParquetFilter(whereOf('SELECT * FROM t WHERE name IS NOT NULL')), { name: { $ne: null } }) }) @@ -124,6 +174,117 @@ test('whereToParquetFilter returns undefined for non-convertible predicates', () assert.equal(whereToParquetFilter(undefined), undefined) }) +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. + 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. + assert.deepEqual( + whereToParquetFilter(whereOf('SELECT * FROM t WHERE id IN (1, NULL)')), + { id: { $ne: null, $in: [1n, null] } } + ) +}) + +// --- NULL rows must not leak past a pushed-down filter ------------------------ + +// @ref LLP 0098 [tests]: the scan claims `appliedWhere` for +// every convertible predicate, so the engine never re-filters. hyparquet +// evaluates a bare bound with raw JS comparison, where `null <= 300n` is true, +// so an unguarded filter is a silent wrong answer rather than an error. +test('pushed-down comparisons do not leak NULL rows (issue #728)', async () => { + /** @type {[string, number[]][]} */ + const cases = [ + // rows: ts = 100, NULL, 300, NULL, 500 + ['ts <= 300', [1, 3]], + ['ts < 300', [1]], + ['ts != 300', [1, 5]], + ['ts <> 300', [1, 5]], + ['ts > 300', [5]], + ['ts >= 300', [3, 5]], + ['ts = 300', [3]], + ['300 >= ts', [1, 3]], + ['NOT (ts > 300)', [1, 3]], + ['ts >= 100 AND ts <= 300', [1, 3]], + ['ts IN (300, 500)', [3, 5]], + ['ts NOT IN (300)', [1, 5]], + // negative bounds: a coerced NULL reads as 0, so `>` and `>=` leak here + // even though they happen to be correct for a positive bound. + // rows: neg = -500, NULL, -300, NULL, -100 + ['neg > -400', [3, 5]], + ['neg >= -300', [3, 5]], + ['neg < -400', [1]], + ['neg <= -500', [1]], + ['neg != -300', [1, 5]], + // strings compare the same way once NULL is out of the picture + // rows: label = 'a', NULL, 'c', NULL, 'e' + ["label < 'c'", [1]], + ["label >= 'c'", [3, 5]], + ["label != 'c'", [1, 5]], + // IS NULL / IS NOT NULL still mean what they say + ['ts IS NULL', [2, 4]], + ['ts IS NOT NULL', [1, 3, 5]], + ['NOT (ts IS NULL)', [1, 3, 5]], + // NOT over an OR: a NULL row is UNKNOWN for every disjunct, which SQL + // excludes but a two-valued complement reports as "nothing matched, + // therefore true". Only correct while negation reaches the leaves, so + // each leaf carries its own guard. + ['NOT (ts > 300 OR ts < 100)', [1, 3]], + ['NOT (ts >= 300 OR ts <= 100)', []], + ['NOT (ts = 100 OR ts = 300)', [5]], + ["NOT (ts IN (100) OR label = 'c')", [5]], + ['NOT (neg > -400 OR neg < -600)', [1]], + ["NOT (label < 'c' OR label > 'c')", [3]], + ['NOT (ts IS NULL OR ts > 300)', [1, 3]], + ] + assert.deepEqual(await mismatches(cases), []) +}) + +test('comparison against a NULL literal matches no rows (issue #728)', async () => { + /** @type {[string, number[]][]} */ + const cases = [ + ['ts = NULL', []], + ['ts != NULL', []], + ['ts < NULL', []], + ['ts IN (300, NULL)', [3]], + ['ts NOT IN (300, NULL)', []], + ] + assert.deepEqual(await mismatches(cases), []) +}) + +/** + * 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 + * than failing on the first, keeps the failure output a full census of which + * operators leak. + * + * @param {[string, number[]][]} cases + * @returns {Promise} + */ +async function mismatches(cases) { + /** @type {string[]} */ + const wrong = [] + for (const [predicate, expected] of cases) { + const source = await makeNullableSource() + const rows = await run(source, `SELECT id FROM t WHERE ${predicate}`) + const got = rows.map((r) => Number(r.id)) + if (got.join(',') !== expected.join(',')) { + wrong.push(`WHERE ${predicate} -> got [${got.join(',')}], SQL says [${expected.join(',')}]`) + } + } + return wrong +} + // --- scan through squirreling ------------------------------------------------ test('parquetDataSource exposes schema columns and row count', async () => {