Push never-match for always-UNKNOWN predicates instead of declining to a two-valued engine (#734)

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

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 <UNKNOWN>` 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