Pushed-down WHERE agrees with the engine on null cells - #37
Merged
Conversation
whereToParquetFilter's output replaces engine-side WHERE (icebergDataSource
sets appliedWhere), so it must select exactly the rows the engine selects.
On nullable columns it did not: squirreling's applyBinaryOp returns false
for any comparison with a null operand, while hyparquet's matchFilter
evaluates $lt/$lte/$gt/$gte with raw JS operators, where a null cell
coerces to 0 and can satisfy the bound, and $ne is true on null. An upper
bound on a nullable column returned rows the query excludes; a negated
comparison dropped rows it includes.
Guard each comparison so null lands on the engine's side of the predicate,
skipping the two operators that already agree so `col = value` keeps its
bare shape. Fall back to the engine for a comparison against a literal
NULL, which folded to {$eq: null} and returned exactly the null rows, and
for a TEXT cast of a non-primitive, which the engine JSON-stringifies
rather than String()-ing.
Adds an end-to-end equivalence suite that runs each predicate through both
icebergQuery and executeSql and compares the row sets; 13 of its 37 cases
fail without this change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #36.
whereToParquetFilter's output replaces engine-side WHERE (icebergDataSourcesetsappliedWhere: truewhenever the conversion succeeds), so it has to select exactly the rows the engine selects. On columns containing nulls it did not, because the two evaluators disagreed:applyBinaryOp) returnsfalsefor any comparison with a null operand, so a null cell never satisfies a bare comparison and always satisfies a negated one.$lt/$lte/$gt/$gtewith raw JS operators, where a null cell coerces to0(null <= new Date()istrue); and it evaluates$neas!equals(null, target), which istrue(correct MongoDB semantics, but not what SQL wants).So
WHERE at <= TIMESTAMP '...'on a nullable column returned the null rows too, andWHERE NOT (n < 7)dropped the null rows it should keep. Wrong answers, not slow ones.What changed
1. Require hyparquet >= 1.28.2. Its range operators now use MongoDB type bracketing: a null cell never satisfies
$lt/$lte/$gt/$gte. That makes every bare comparison ($eqincluded) agree with the engine with no guard at all, so the commoncol < valuepredicate keeps its bare shape and prunes exactly as before.2. Guard the two remaining disagreements, which are MongoDB semantics rather than hyparquet bugs:
col != value:$neistrueon a null cell in Mongo, but the engine excludes null rows. Converted to{$and: [{col: {$ne: null}}, {col: {$ne: value}}]}.NOT (n < 7)flips to$gte, which isfalseon a null cell, but the engine's NOT over a false comparison keeps null rows. Converted to{$or: [{col: {$eq: null}}, <flipped>]}. The exception isNOT (a = b)→$ne, alreadytrueon a null cell, which stays bare.3. Fall back for a comparison against a literal
NULL.WHERE n = NULLfolded to{n: {$eq: null}}, which means "is null" to hyparquet and returned exactly the null rows; the engine answers itfalsefor every row.IS NULL/IS NOT NULLare unaffected - they are a different AST node and were already correct.4. Fall back for a TEXT cast of a non-primitive.
foldCastusedString(val), but the engine JSON-stringifies objects, soCAST(TIMESTAMP '2026-08-11T00:00:00Z' AS TEXT)folded to a locale date string where the engine produces"\"2026-08-11T00:00:00.000Z\"". squirreling'sstringifyhelper is not exported, so this falls back rather than copying it.Everything else already agreed and is untouched:
=,IS NULL,IS NOT NULL,IN,NOT IN(including aNULLinside the list), andNOT (a = b).Not a pushdown regression
Every case that pushed down before still pushes down -
appliedWherestaystrue. Bare comparisons keep their original unguarded shape, so row-group and file-level pruning are unchanged for them; the#20 file-level bounds pruningand#21 row-group pruningsuites pass unchanged, including theirdataFilesRead()and byte-count assertions.The
$orshape used for negated comparisons does give up pruning on row groups that contain nulls, since$orcan only skip when every branch can. Negated bounds are rare, and the alternative is wrong rows.Tests
New
test/sql/whereFilter.equivalence.test.jsbuilds a real 3-row Iceberg table with nullableint,timestamptzandstringcolumns and runs 37 predicates both ways - throughicebergQuery(pushdown) and throughexecuteSqlover the same records (engine truth) - asserting the row sets match. This is the invariant the converter has to hold, tested end to end through the actual parquet reader rather than at the filter-shape level. 13 of the 37 fail onmasterwith hyparquet 1.28.1.test/sql/whereFilter.test.jsgains unit coverage for the guard shapes, the NULL-literal fallback, and the TEXT-cast fallback.Full suite: 682 pass, lint and typecheck clean.
Downstream
hyparam/hypaware#721 is blocked on this - it adopts this converter in place of its own, and eleven of its shipped column specs are nullable TIMESTAMP (
logs.timestamp,logs.observedTimestamp,traces.startTimestamp,traces.endTimestampamong them). It needs a release to bump to.