Guard pushed-down parquet filters against NULL rows (#728)

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, <non-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 } }
 }

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<AsyncDataSource>}
+ */
+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<string[]>}
+ */
+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 () => {