A pushed-down WHERE no longer widens the parquet projection by philcunliffe · Pull Request #724 · hyparam/hypaware

philcunliffe · 2026-08-12T22:37:40Z

What

parquetDataSource.scan threw away the engine's column projection whenever a WHERE converted to a parquet filter:

const readColumns = filter ? undefined : hints.columns

undefined means "read every column" to hyparquet, so any filtered scan decoded the whole row, payload columns included. The fix is to pass the projection unconditionally.

Measured on a synthetic three-column file (two thin columns, one fat text column, 2000 rows), SELECT model ... WHERE role = 'user':

	bytes read
before (projection dropped)	8,009,294
after (projection honored)	596

Same 1000 rows out both times.

Why it is safe, not just cheaper

A filter can name columns the SELECT does not, and "read everything" was the always-correct blunt answer to that. Two layers now cover it independently:

squirreling folds WHERE identifiers into the scan projection at plan time (collectColumnsFromExpr(select.where, identifiers) in plan/columns.js).
hyparquet unions columnsNeededForFilter into its own read plan, then deletes the extras from the rows it returns (requiresProjection in read.js). This needs hyparquet >= 1.28.1, which is already the pinned version.

Two adjacent worries were checked rather than assumed:

Additive schema drift is not a hazard. hyparquet ignores a projected column the file lacks instead of throwing, and unionSources already declines to push a predicate to a partition missing its columns.
filterStrict: false governs == against === when comparing values, not whether the filter is fully applied. matchFilter still runs per row, so appliedWhere: true stays honest.

Scope

Narrower than the shape of the bug suggests, and worth stating plainly: neither the local cache nor the server archive reaches this code. Both go through icebird's icebergDataSource, which already keeps the projection and adds filter columns on top. The only caller is the s3 plugin's format: 'parquet' datasets (plugins-workspace/s3/src/query-dataset.js). This is a real fix on a real path, not the fleet-wide query win the line's shape implies.

Testing

The existing tests pass either way: they assert the rows are right, and the rows stay right when the scan reads everything and the engine projects afterwards. The new test asserts the narrow read directly, both in what the scan emits and in the bytes it pulls off the file.

test/core/parquet-source.test.js 16/16, including the new a pushed-down filter does not widen the projection, verified to fail against the old line
test/core/union-source.test.js 15/15, test/plugins/s3-query-dataset.test.js 6/6
npm run typecheck clean
npm test 3942/3945 pass. The one failure (leave tears down a central layer whose active-slot pointer does not resolve (#623)) reproduces on clean master and is unrelated.

Note for reviewers

No LLP was minted. The comment being replaced encoded the old decision, and the replacement comment plus this commit message carry the new one; a one-line change did not seem to warrant its own decision doc. Happy to add one if you disagree.

🤖 Generated with Claude Code

Fixes #729

`parquetDataSource.scan` dropped the engine's column projection whenever a WHERE converted to a parquet filter: const readColumns = filter ? undefined : hints.columns `undefined` means "read every column", so any filtered scan decoded the whole row, payload columns included. On a table where a few columns carry most of the bytes that is the difference between reading a projection and reading the file. Measured on a synthetic three-column file (two thin columns, one fat text column, 2000 rows): 8,009,294 bytes read for `SELECT model WHERE role = 'user'` against 596 bytes for the same query honoring the projection, same 1000 rows out. The original reason no longer holds. A filter can reference columns the SELECT does not, and reading everything is the always-correct blunt answer, but two layers now cover that case independently. squirreling folds WHERE identifiers into the scan projection when it plans (`collectColumnsFromExpr(select.where, identifiers)`), and hyparquet unions `columnsNeededForFilter` into its own read plan and deletes the extras from the rows it returns before handing them back (`requiresProjection`, hyparquet >= 1.28.1, already the pinned version). So passing the narrow projection alongside a filter is safe, not merely cheaper. Two adjacent worries checked rather than assumed. Additive schema drift is not a hazard: hyparquet ignores a projected column the file lacks instead of throwing, and `unionSources` already refuses to push a predicate to a partition missing its columns. `filterStrict: false` governs `==` against `===` when comparing values, not whether the filter is fully applied, so `matchFilter` still runs per row and `appliedWhere: true` stays honest. Scope is narrower than the shape of the bug suggests. Neither the local cache nor the server archive reaches this code: both go through icebird's `icebergDataSource`, which already keeps the projection and adds filter columns on top. The only caller is the s3 plugin's `format: 'parquet'` datasets. The existing tests pass either way, because they assert the rows are right and they stay right when the scan reads everything and the engine projects afterwards. The new test asserts the narrow read directly, in what the scan emits and in the bytes it pulls off the file, and fails on the old line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…et test unionSources always reports appliedWhere: false, handing filters back to the engine to re-apply over the merged stream. Since the projection pushdown fix now emits exactly the requested columns, that re-apply only works because squirreling folds WHERE columns into the projection it hands to scan(). Add an end-to-end test over two real parquet partitions that pins this, and note the dependency in both parquet-source.js and union-source.js comments.

philcunliffe · 2026-08-12T23:20:44Z

Review round 1 of 27cd933 (adopted PR). Verdict: findings - 1 minor, fixed and pushed as 50daa2f.

The core change is correct. The reviewer could not produce a single row-level or value-level divergence between the pre-fix and post-fix scan across 35 queries covering NULLs, aggregates, SELECT *, LIMIT/OFFSET, GROUP BY, ORDER BY, IN, NOT, non-convertible predicates, multi-row-group files, page-index pruning, and a two-partition union with additive schema drift. Byte reads drop 4x to 100x.

1. minor - the union's re-filter input set is now narrowed, and nothing pinned the invariant that keeps it safe. FIXED

unionSources reports appliedWhere: false unconditionally (union-source.js:56), handing the predicate back to the engine to re-apply over the merged stream. For the engine to do that, the predicate's columns must be present in the rows the sub-scans emit. Before this change a partition that accepted the pushdown had readColumns = undefined and emitted every column, so the where-column was there regardless of what the caller asked for. After, the emitted set is exactly hints.columns:

old: union appliedWhere=false emitted=[{"cols":["id","name","score"]}, ...]
new: union appliedWhere=false emitted=[{"cols":["name"]}, ...]

Not a live bug: squirreling's extractColumns folds WHERE identifiers into the per-table projection (plan/columns.js:142), verified present across 20 engine-driven queries. But the PR converts a belt-and-braces guarantee into a single implicit dependency on a pinned third-party planner, on the one path (the s3 multi-partition union) where the engine, not hyparquet, owns the filter. If that folding ever stopped, the single-source path would still be correct, so the whole test file would stay green while the union path silently returned wrong rows.

Fixed by pinning the invariant where it is actually depended on: a new test unions two real parquet partitions and runs SELECT name FROM t WHERE score > 3 end to end through executeSql, asserting the rows. Verified it is a genuine fixture (parquetWriteBuffer / parquetDataSource / real executeSql), not a fake source. Both comments were extended too: parquet-source.js:56-67 now says a caller that downgrades appliedWhere to false must include the predicate's columns in columns, and union-source.js:26-38 notes that forwarding columns now also determines what the engine gets to re-filter on.

Also checked, clean

The projection computation. In hyparquet 1.28.1, if (columns && filter) unions columnsNeededForFilter into readColumns and sets requiresProjection, then deletes the extras from each matched row after matchFilter runs. Row-group skipping is unaffected: parquetPlan computes physicalColumns from the full schema tree, not the requested columns, so canSkipRowGroup sees the same statistics either way. The read set is strictly (projection union filter columns) intersect file schema - a strict narrowing of the old "everything".
Filter columns not in the projection - the headline hazard, covered on both layers. The new test exercises it directly, and COUNT(*), SUM(score) with a WHERE on id, ORDER BY score with a WHERE on id, and NOT (score > 3) were all verified identical old versus new.
appliedWhere, the Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721 shape. Untouched, and narrowing cannot make the filter less applicable: hyparquet raises the read set to cover the filter regardless of projection, and matchFilter still runs per row. There is no path where the projection narrows such that the filter is only partially applied while appliedWhere stays true.
NULL and missing-column behaviour. A fixture with every 7th row NULL, compared across <=, <, >, >=, !=, IS NULL, IS NOT NULL: byte-identical row sets old versus new. Narrowing does not change whether a predicate sees a real NULL, because the filter column is always read.
Additive schema drift. Verified on a real two-partition union where partition B physically lacks score. parquetPlan skips a projected column the file lacks, and canPushWhere still declines to push a predicate to a partition missing its columns, so hyparquet's "filter columns not found" guard is never reached. Identical output on every drift query.
Column ordering and row alignment. hyparquet materialises whole rows per row group and deletes filter columns uniformly, so Object.keys(data[0]) stays representative. The columns are never read independently, so no desynchronisation is possible.
Wrappers that could lose a column. withLocalOnlyVisibility force-adds cwd whenever it is needed and the caller narrowed, so it does not rely on the old read-everything behaviour. withSchemaColumns is on the iceberg-backed path and never wraps a parquetDataSource.
Request count, not just bytes. Reading two non-adjacent columns instead of one contiguous run can double range GETs per row group (measured 20 versus 10) while cutting bytes 3-6x. Not a latency regression: prefetchAsyncBuffer issues all ranges concurrently, so it is one more parallel connection, not another round trip.
Test discrimination, verified by reverting. Restoring readColumns = filter ? undefined : hints.columns makes exactly one test fail (a pushed-down filter does not widen the projection), and both of its assertions discriminate. Using columns: undefined as the baseline is a good choice - it is literally the pre-fix code path.
LLP. No LLP added, so no collision with the three competing llp/0212-* in hyp status is a triage summary; the inventory moves behind --full (LLP 0212) #716/Reduce the client skill surface to three; always activate the graph #720/Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721. Nothing in the corpus encodes the removed decision; LLP 0015 documents the limit/offset stripping and the canPushWhere gate but says nothing about the parquet source's read set. No @ref warranted - one here would only restate the comment and filename. The "no LLP minted" call is right.
Conventions. No semicolons, no U+2014, no @typedef, no inline import types.

Gates at the new head: npm test 3945 pass / 0 fail / 1 pre-existing skip; typecheck clean.

Separately: this review found a live bug on `master`

While probing NULL semantics the reviewer found that the kernel's own converter already leaks NULL rows through <, <= and != pushdowns, identically before and after this PR, and live on master today for s3 format: 'parquet' datasets. Filed as #728. Nothing for this PR to do, but it materially reframes #721: that PR does not introduce the null hazard, it only makes a new set of predicates reach it. A shared null-guard would close both at once.

The head has moved to 50daa2f, so the next tick reviews that head (round 2).

…lure mode The new end-to-end test was annotated `@ref LLP 0098#union-flags`, but 0098's union flag merging covers the `scanColumn` path and settles that the union's `appliedWhere` is the AND across partitions. This test never reaches that path: `parquetDataSource` defines no `scanColumn`, so `unionSources` installs no hook, and the query is not an aggregate. The invariant it pins is stated verbatim in LLP 0015 `#multi-partition-union` (unconditional `appliedWhere: false`, engine re-applies over the merged stream), which is already the ref on the construct under test in `src/core/query/union-source.js`. Also correct the comment's stated failure mode. If squirreling stopped folding WHERE columns into the projection, squirreling's `asyncRow` raises `ColumnNotFoundError` when the engine re-filters on a column the rows no longer carry, so the break is loud at query time, not silent wrong rows. The overstated clause would skew a future squirreling upgrade calculus toward "silent data corruption" when the true exposure is caught by the first query. Comment and annotation text only; no assertions or code changed.

philcunliffe · 2026-08-12T23:59:40Z

Review round 2 of 50daa2f. Verdict: findings - 2 minor, both fixed and pushed as 8e20ee0. Both were confined to the comment and annotation text added in the round-1 fix; the code is correct and the new test genuinely discriminates.

1. minor - the `@ref` pointed at the wrong LLP section. FIXED

The new test was annotated @ref LLP 0098#union-flags. LLP 0098's "Union flag merging" is about the scanColumn path and settles that the union's appliedWhere is the AND across partitions. This test never touches that path: parquetDataSource defines no scanColumn (zero occurrences), so unionSources never installs that hook here, and the query is not an aggregate. It exercises union.scan() end to end.

The invariant it actually pins is stated verbatim in LLP 0015 #multi-partition-union: "The union reports appliedWhere: false and appliedLimitOffset: false, so the SQL engine re-applies both over the merged stream" - which is already the ref on the construct under test (union-source.js:39). A reader following the old ref landed on a different flag rule for a different code path, and 0098's "AND across partitions" phrasing directly contradicts the unconditional appliedWhere: false this test depends on. Retargeted to LLP 0015#multi-partition-union; anchor verified to resolve.

2. minor - the comment overstated the failure mode. FIXED

It read "If squirreling ever stopped folding, this would start silently returning wrong rows." The reviewer simulated exactly that (wrapping the union so the projection reaching scan() omits score while the predicate stays) and it is not silent:

NARROWED scan columns = ["name"]
DISCRIMINATES (throws): ColumnNotFoundError: Column "score" not found. Available columns: name (row 1)

squirreling's asyncRow raises ColumnNotFoundError when the engine re-filters on a column the row does not carry, so the failure is a hard throw at query time. That matters because the sentence was the stated rationale for the test: a maintainer weighing a future squirreling bump would read "silent data corruption" when the real exposure is "loud, immediate, caught by the first query" - a very different upgrade calculus. Corrected to name the throw; the accurate part (every other test would stay green, since they exercise a single source) is preserved verbatim.

Verified from round 1

The fix landed and it does discriminate - the main thing round 2 was asked to establish. Forcing the narrowed case does not merely change the rows, it fails outright with ColumnNotFoundError. So the test cannot pass while the invariant is violated; it is a real pin, not a tautology. Baseline instrumentation first confirmed the real planner does fold (SCAN HINTS columns = ["name","score"]).

It also pins at the right layer: it is the only test in the repo that runs the union's row path through a real executeSql with a WHERE on a non-selected column. Every other union test uses fake sources that record ScanOptions, and every parquet-source.test.js test uses a single source where appliedWhere: true means the engine never re-filters. Round 1's concern was that the guarantee had moved from belt-and-braces into an implicit dependency on a pinned third-party planner; this converts that into an explicit, failing-on-regression assertion.

Both added comments state the contract accurately. The only nuance either omits is that hints.columns may be undefined (SELECT *), in which case the reliance does not arise - not worth an edit.

Also checked, clean

The appliedWhere: true hazard, confirmed at the source rather than assumed. In hyparquet read.js:35-51, columnsNeededForFilter(filter) is computed first; a filter column absent from the file schema throws (so a partially-applicable filter can never be silently half-applied), and one absent from the projection is appended to readColumns with requiresProjection = true. The filter is evaluated against the widened row at :110, and only afterwards are the extras deleted from rows that already matched. The read set is always a superset of the filter's needs regardless of projection, so appliedWhere = Boolean(filter) cannot overclaim. matchFilter handles $and/$or/$nor recursively and columnsNeededForFilter flattens nested paths, so compound predicates are covered.
Schema drift with pushdown accepted - the one case whose behaviour this line actually changes. An additive-drift union was built and six queries run across every combination of drifted projection, drifted predicate, SELECT *, and aggregate, then the identical matrix re-run with the line reverted. Output is byte-identical before and after, in both partition orders.
Delta 27cd933..50daa2f - three files, no behaviour change. The test helpers mirror parquet-source.test.js, with rowGroupSize: 2 against 3- and 2-row partitions so both span a row-group boundary rather than degenerating to a single group.
Core change re-sweep. appliedLimitOffset is now the only remaining where-dependent branch and is untouched. Object.keys(data[0]) is still correct under the narrowed read: hyparquet builds rows in readColumns order and deletes only the filter extras it appended, so the surviving key set is exactly hints.columns.
Conventions clean across all four files.

Gates: npm test 3945 pass / 0 fail / 1 pre-existing skip; typecheck clean.

One pre-existing gap found, out of scope for this PR

LLP 0015 #multi-partition-union and the pre-existing comment at union-source.js:28 both claim "projecting an absent column reads as null, never throws." That is not true on master today, independent of this PR: SELECT extra FROM t WHERE score > 3 yields [{}, {extra:"x"}] - the drifted row comes back with the key missing, not null - and SELECT extra FROM t WHERE extra IS NOT NULL throws ColumnNotFoundError. Both reproduce identically on the reverted line, so it is a master-level doc-versus-behaviour gap, not a regression here. Recorded so it is not lost; it should not gate this PR.

Also noted, not raised as a finding: union-source.test.js:439 carries a pre-existing inline import('squirreling/src/types.js') type, which the repo's conventions forbid. It predates this PR (introduced in 6bd85c4) and appears nowhere in this diff, but it is a standing violation someone may want to sweep.

The head has moved to 8e20ee0, so the round budget (2) is spent at an unreviewed head: the next tick triages rather than opening a round 3.

philcunliffe · 2026-08-13T00:29:47Z

Triage after the review budget (LLP 0017). Two review rounds ran (3 findings, all fixed). Both residuals were judged non-blocking, so this PR can ship. Deferred to #731.

(A) The LLP 0015 doc-versus-behaviour gap - the triage reproduced it independently on a two-partition drift fixture, then reverted this PR's line and re-ran the identical fixture, getting byte-identical output. So the false invariant is neither introduced nor worsened here. It also traced why: unionSources already drops where for a partition that cannot satisfy the predicate before parquetDataSource.scan runs, and hyparquet treats an explicit-but-absent column request identically to "give me everything this file has". One more reachability point worth recording: even pre-PR, unfiltered union queries already took the narrow hints.columns path, so the drift-throw was already reachable via any unfiltered SELECT extra FROM t. This PR does not newly expose a previously-safe path.

(B) The inline import('...') type at union-source.test.js:439 - git blame traces it to 6bd85c4 (LLP 0098 scanColumn wrappers, #304), about a month before this PR's first commit, and this PR's only edits to that file are the new import block and the new test around lines 126-197. A lint-level violation with zero runtime effect, in an untouched line of a touched file.

Blocking a verified 170x latency and 1800x memory fix on either would be a poor trade.

Keeps both intents in the union's `columns`-forwarding comment: this branch's corrected absent-column contract (undefined-or-throws, with the three `executeProject`/`collect()` conditions and the `resolveable` gate) and #724's clause on the narrowed projection determining what the engine re-filters on. Both test files converge on the shared `test/helpers/parquet_source_fixture.js` rather than keeping master's re-copied inline parquet fixtures. Re-verified the documented contract against the post-merge tree: with #724's `readColumns = hints.columns`, a partition asked for a column it lacks still emits only the columns it has, so a bare projection still reads `undefined` even under a pushed-down WHERE.

philcunliffe added neutral:adopt Foreign PR adopted into neutral's reconcile scope neutral:adopted Adoption completion record: merged while carrying neutral:adopt (LLP 0031) labels Aug 12, 2026

This was referenced Aug 12, 2026

Pushed-down <, <=, != leak NULL rows past the filter on nullable columns (live on master) #728

Closed

Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721

Merged

philcunliffe mentioned this pull request Aug 12, 2026

parquet source drops column projection whenever a filter is pushed down: filtered queries read every column (measured 170x time / 1800x memory) #729

Closed

philcunliffe mentioned this pull request Aug 13, 2026

Follow-up: deferred review findings from PR #724 #731

Open

philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 13, 2026

philcunliffe merged commit d5c53ed into master Aug 13, 2026
9 checks passed

philcunliffe deleted the fix/projection-survives-filter branch August 13, 2026 06:39

philcunliffe mentioned this pull request Aug 13, 2026

The union's absent-column contract is undefined-or-throws, not null (#731) #740

Draft

Navigation Menu

Uh oh!

A pushed-down WHERE no longer widens the parquet projection - #724

A pushed-down WHERE no longer widens the parquet projection#724
philcunliffe merged 3 commits into
masterfrom
fix/projection-survives-filter

philcunliffe commented Aug 12, 2026 •

edited

Loading

Uh oh!

philcunliffe commented Aug 12, 2026

Uh oh!

philcunliffe commented Aug 12, 2026

Uh oh!

philcunliffe commented Aug 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

philcunliffe commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why it is safe, not just cheaper

Scope

Testing

Note for reviewers

Uh oh!

philcunliffe commented Aug 12, 2026

1. minor - the union's re-filter input set is now narrowed, and nothing pinned the invariant that keeps it safe. FIXED

Also checked, clean

Separately: this review found a live bug on master

Uh oh!

philcunliffe commented Aug 12, 2026

1. minor - the @ref pointed at the wrong LLP section. FIXED

2. minor - the comment overstated the failure mode. FIXED

Verified from round 1

Also checked, clean

One pre-existing gap found, out of scope for this PR

Uh oh!

philcunliffe commented Aug 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

philcunliffe commented Aug 12, 2026 •

edited

Loading

Separately: this review found a live bug on `master`

1. minor - the `@ref` pointed at the wrong LLP section. FIXED