Pin cross-backend query parity and icebird 0.8.22's pushdown contract (tests only)

[philcunliffe]

Tests only. No production code changes; the diff against master is two test
files.

What happened to this PR

It opened as a fix for #744: withSqlCorrectWhere, a wrapper making the
iceberg cache path SQL-correct on NULL predicates. Then #721 merged
(8c08185), bumping icebird 0.8.20 to 0.8.22, hyparquet 1.28.1 to 1.28.2
and squirreling 0.15.2 to 0.15.3, and replacing this repo's WHERE converter
with icebird's. The NULL work converged upstream, so #744 is closed by that
bump and the wrapper had no bug left to work around: it would only re-apply
icebird's own filter, with the same evaluator, to rows that had already passed
it, which is the per-row materialization LLP 0098 exists to prevent.

Two independent runs of this PR's own parity corpus against master with no
wrapper present came back fully correct. The reduction below follows the
disposition left on this thread:
#751 (comment)

This PR closes nothing. #744 is already closed by #721.

Removed

• src/core/query/iceberg-source.js (withSqlCorrectWhere) and its export in
  src/core/query/index.js
• the dataSourceForTable wiring in src/core/cache/iceberg/store.js
• the s3 wiring in hypaware-core/plugins-workspace/s3/src/query-dataset.js
• LLP 0221 and the Extended-by forward-ref it added to LLP 0098

LLP 0222, which landed with #721, records where converter ownership lives now.
Nothing in the tree references the deleted module.

Kept, and what each surviving test would catch

Both tiers run the same converter now, so "cache agrees with parquet" is a
weaker statement than it was in the first round of this PR. Stated honestly,
per test:

cache and parquet backends answer the corpus identically, and answer it the way SQL does (47 predicates). Every expected row set is SQL's three-valued
answer written down by hand, not a recording of behaviour, so the corpus fails
on a shared regression as well as a divergent one. That is its main job: it
is a tripwire on three pinned dependencies this repo does not own. Verified:
against the pre-#721 stack (icebird 0.8.20, hyparquet 1.28.1,
squirreling 0.15.2) five of this file's six tests fail, the corpus test with
29 wrong answers. The cache-versus-parquet half is still a real cross-backend
check, because the two sources are not the same code below the converter:
icebird prunes on manifests and data-file bounds before hyparquet sees a row
group, and only one tier is a dependency.

filtered aggregates take the same NULL semantics as the row scan.
icebergDataSource converts the predicate a second time in scanColumn.
Catches the two conversion sites drifting apart, which the row corpus alone
would not see.

the cache column stream reports appliedWhere honestly. appliedWhere is
final on this path: the engine never re-judges a claimed predicate, and a
direct scanColumn caller has nothing above it to re-filter. A claim for a
declined predicate is a wrong answer, not a lost optimisation. This is exactly
the #744 failure mode, pinned at the seam where it bit.

both backends agree on which predicates are converted and which are declined. The contract LLP 0222 makes the stack depend on, pinned from the
consumer's side: bare bounds and folded casts converted, NULL literals, LIKE,
function calls and column-versus-column declined, asserted shape by shape
against the parquet tier. An upstream converter change that started claiming a
shape it does not apply, or quietly stopped folding one it used to, fails here.
Note this also records that the projection asymmetry round 2 measured is gone:
scan({columns: ['id'], where: ts > 300}) now reports appliedWhere: true on
both tiers.

LIMIT and OFFSET are held back under a WHERE. icebird would otherwise cap
the scan at offset + limit rows matching its own filter before the engine
finished judging the predicate. Catches both a flag regression
(appliedLimitOffset claimed under a WHERE) and an ordering one, end to end:
ts IS NOT NULL LIMIT 2 returns [1, 3], which is wrong as [1] if the slice
runs before the filter.

a filtered cache scan still prunes whole data files. Two data files with
disjoint ts ranges; the filtered scan must open one. Catches where ceasing
to reach icebird's manifest walk, for instance by a wrapper like the one this
PR just deleted. It now also asserts the cast form prunes
(ts > CAST(8000 AS BIGINT)), which is the fold LLP 0222#context measured in
production at 11.4s versus 7.3s. Verified sensitive: swap that for a shape
icebird declines and the assertion fails with "cast-bounded scan opened 2 data
files".

test/plugins/s3-query-dataset.test.js, the BlobStore round trip, kept in
adapted form. The s3 iceberg branch is a lazy dynamic import reached only by
s3-configured deployments, so a regression there is silent; the test drives the
production buildS3QueryDataset factory over a real table written through a
real BlobStore.

The CAST subset chain: measured, then tightened to equality

Round 2 of this PR asserted SQL ⊆ cache ⊆ parquet for CAST/typed-literal
predicates, and round 2's own review found the bounded flag was dead weight
even under the old code: the one case using it landed on equality, so the
subset branch never exercised a strict subset, and nothing distinguished a
legitimately bounded case from a regression relabelled bounded.

Measured on the fixture at this head, through dataSourceForTable and
parquetDataSource, including the case round 2 measured as a genuine strict
subset under the old stack:

predicate	SQL	cache	parquet
neg > CAST(-400 AS BIGINT)	[3,5]	[3,5]	[3,5]
NOT (neg > CAST(-400 AS BIGINT))	[1]	[1]	[1]
NOT (neg >= CAST(-300 AS BIGINT))	[1]	[1]	[1]
NOT (neg > CAST(-400 AS BIGINT) OR neg > CAST(-600 AS BIGINT))	[]	[]	[]
neg != CAST(-300 AS BIGINT)	[1,5]	[1,5]	[1,5]
ts > CAST('300' AS BIGINT)	[5]	[5]	[5]
neg > CAST(-400.9 AS INTEGER)	[3,5]	[3,5]	[3,5]
neg > CAST(-400 AS DOUBLE)	[3,5]	[3,5]	[3,5]
label > CAST(3 AS TEXT)	[1,3,5]	[1,3,5]	[1,3,5]

Under the old stack, NOT (neg > CAST(-400 AS BIGINT)) was SQL [1], cache
[1], parquet [1,2,4]. The divergence is gone, and the mechanism is not
luck: the predicate class is no longer declined at all. The kernel converter
used to refuse a cast operand while icebird folded it, so only the cache tier
got a pruning hint and only the parquet tier fell to a two-valued WHERE. Both
tiers now run icebird's converter, which folds the cast, pushes a bare bound
(guarded only for !=/NOT IN; the rest are SQL-correct on null cells
because of the hyparquet >= 1.28.2 floor, LLP 0222#hyparquet-floor) and
claims appliedWhere on both sides.

So the chain is asserted as full equality, and the bounded option, the
isSubset helper and the subset branch are deleted rather than left behind
as an assertion that cannot fail. The negated forms are in the corpus as plain
equality cases, which is strictly stronger than the subset assertion they
replace.

Checks

Rebased onto master at 8c08185 (clean, no conflicts). Fresh npm install.

• npm test: 4036 pass, 0 fail, 1 pre-existing skip
• npm run typecheck: clean
• node --test test/core/iceberg-source-parity.test.js test/plugins/s3-query-dataset.test.js test/core/llp-ref-hygiene.test.js: 24/24
• npm run smoke -- local_parquet_export: ok
• npm run smoke -- cache_lifecycle_maintenance: ok

package_bin_boot and walkthrough_picker_to_first_query are red on master
for unrelated reasons (#758, #750).

🤖 Generated with Claude Code

[philcunliffe]

neutral review - round 1

Head reviewed: 026855b41350d583f2bb5d58658244b123d5bc9f. All 9 checks SUCCESS at
that SHA. Reviewed in a detached worktree; nothing was written to the branch.

Two minor findings, no blockers. The design's load-bearing claim, that
icebird's converted filter never excludes a row SQL says TRUE, was not accepted as
written: the reviewer attacked it with a prune-isolation proxy (strip where
before it reaches icebird, leave the wrapper's own matching intact, so any
difference is exactly a row lost to pruning), a hand-written Kleene 3VL oracle, and
the parquet backend as a third judge. 612 randomised predicates: 0 pruning
losses, 0 divergences from the oracle, 0 cache-versus-parquet divergences. The
argument holds.

Finding 1 is the one place the PR's own headline overreaches, and it is exactly at
the seam judgement call 5 is about.

---

VERDICT: findings

---

1. minor — llp/0221-cache-where-sql-correctness.decision.md:126-127 and :134-138 (also PR body, "Interaction with #721 and #734")

The LLP's Consequences assert cross-backend parity unconditionally: "A predicate over ai_gateway_messages and the same predicate over a parquet file return the same rows" and "[the #734 gap] is now identical on both backends rather than worse on one." Both are falsifiable, and I falsified them.

Judgement call 5 keeps forwarding the where to icebird even when the kernel converter declines. For the shapes icebird converts and this repo's converter does not (typed literals, casts of literals), the hint prunes files whose non-null values are all outside the bound — including the NULL rows those files also carry. The engine's two-valued fallback then never sees them. Fixture: two data files, ts nullable, file A = [2026-01-01], file B = [2026-09-01, NULL]:

NOT (ts > TIMESTAMP '2026-06-01T00:00:00Z')   cache=[1]  parquet=[1,3]
NOT (ts >= TIMESTAMP '2026-06-01T00:00:00Z')  cache=[1]  parquet=[1,3]

Why it matters: the direction is safe (pruning can only remove rows icebird's filter excludes, and that set contains no SQL-TRUE row, so the cache lands between SQL and the parquet path — exactly what judgement call 5 of the PR body says, correctly). No rows SQL selects are lost. But an Accepted LLP now records a parity claim that does not hold, and the corpus contains no CAST/typed-literal predicate at all, so nothing pins the exception. This is the one predicate class where the PR's own headline ("makes the two agree") is not true, and it is the class judgement call 5 is specifically about.

Exact fix: (a) scope Consequences bullet 1 to "a predicate the kernel converter owns"; (b) amend bullet 3 to state that for a declined predicate the cache path may return fewer rows than the parquet path, because the pruning hint drops UNKNOWN rows the engine's two-valued WHERE would have returned, and that this is bounded between SQL's answer and the parquet path's; (c) add a typed-literal case to PARITY_CASES in test/core/iceberg-source-parity.test.js asserting SQL ⊆ cache ⊆ parquet rather than cache === parquet, so the exception is pinned instead of latent.

---

2. minor — hypaware-core/plugins-workspace/s3/src/query-dataset.js:128

The s3 branch ships with zero test coverage; the author lists "no s3 round-trip" under "What I could NOT verify."

I verified it. The change is correct and the s3 iceberg path is NULL-correct end to end (ts = NULL, NOT (ts = NULL), ts != NULL, ts NOT IN (300, NULL) → []; ts != 300 → [1,5]; NOT (ts > 300 OR ts > 400) → [1,3]; COUNT(ts) WHERE NOT (ts = NULL) → 0). So this is a coverage gap, not a defect — but the branch is a lazy dynamic import('icebird') inside an async factory reached only by s3-configured deployments, so a regression there is silent, and the round-trip is cheap: createLocalFsBlobStore (local-fs/src/blob-store.js) + commitBatch/probeTable (format-iceberg/src/commit.js) + tableUrlForBlobPrefix write a real iceberg table into a BlobStore, which buildS3QueryDataset then reads. Roughly 60 lines; I built and ran it.

Exact fix: add that round-trip to test/plugins/s3-query-dataset.test.js next to the existing iceberg query source with no metadata reads as empty test, asserting the four NULL-literal predicates plus one filtered aggregate.

---

The superset argument, attacked

The design stands or falls on: icebird's converted filter never excludes a row SQL says TRUE, at row level or through any pruner. I did not accept LLP 0221 #pruning-hint as written. Method: a noPrune proxy that strips where before it reaches icebird while leaving the wrapper's own matchFilter untouched, so any difference between withSqlCorrectWhere(raw) and withSqlCorrectWhere(noPrune(raw)) is exactly a row lost to pruning. Plus a hand-written Kleene 3VL oracle, and the parquet-file backend, as independent judges.

Randomised fuzz. 612 distinct predicates (grammar: comparisons in both operand orders, NULL literals at ~15%, IS [NOT] NULL, IN/NOT IN with NULL members, BETWEEN, string and boolean comparisons, nested AND/OR/NOT to depth 3) over a 6-file / 60-row iceberg table with ~30% NULLs per column, columns INT64/INT64/STRING/DOUBLE/BOOLEAN. Result: 0 pruning losses, 0 divergences from the 3VL oracle, 0 cache-vs-parquet divergences.

Leaf-by-leaf, reading icebird/src/sql/whereFilter.js, icebird/src/prune.js, hyparquet/src/filter.js:

• NULL-literal comparisons. icebird emits {$eq: null} / {$ne: null}; SQL's TRUE set is empty, so any filter is trivially a superset. boundsOpMightMatch('$eq', null, …) → safeCompare returns undefined → keeps the file. Row-group: matchingNulls is true whenever null_count is unknown or > 0; a group with null_count === 0 is skipped, which is correct (it holds no NULL row). Clean.
• Unguarded inequalities over NULLs / negative bounds. {neg: {$gt: -400}} matches NULL rows (null → 0); those rows are SQL-UNKNOWN, never SQL-FALSE, so the filter is wider, never narrower. boundsOpMightMatch prunes on hi/lo from non-null manifest bounds, and every prune requires the predicate to be provably outside [lo, hi], which excludes no TRUE row. Verified directly: neg > -400, neg >= -300, neg < -400, neg <= -500 all lossless with pruning live.
• $nor / negated OR. The invariant that carries it is no leaf matches a SQL-FALSE row; a row FALSE for every disjunct is therefore in no child, hence in the complement. I checked this holds for every leaf icebird can emit, including nested NOT (a OR (b AND c)) and NOT (NOT a OR b). Independently, both pruners refuse to touch $nor at all (prune.js nodeMightMatch continues; filter.js:124 returns false; filterPageRanges returns undefined), so $nor prunes nothing and cannot lose anything. Clean.
• IN / NOT IN, with and without NULL members. {$in: [300, null]} never prunes at file level (the null member's eqInRange is undecidable → keep) and never prunes on statistics while nulls are possible. {$nin: […]} is in boundsOpMightMatch's default: return true. Clean.
• BETWEEN. Parses to >= AND <=; icebird's $and pruning skips only if some conjunct proves the whole range impossible, which is sound. ts BETWEEN NULL AND 500 → SQL empty, filter wider. Clean.
• IS NULL / IS NOT NULL. The only predicates where a NULL row is SQL-TRUE, so the only place a superset argument could break. IS NULL → {$eq: null}: file-level undecidable (keep), row-group suppressed by matchingNulls, page-level additionally protected by pages.nullPages[i]. IS NOT NULL → {$ne: null}: canSkipStats's $ne branch only fires on a constant chunk equal to the target, impossible for null. Under $or (ts IS NULL OR ts > 300) canSkipRowGroup requires every branch to allow the skip. Verified empirically with pruning live (3 files opened, not 1). Clean.
• Boolean columns and mixed-type literals (the place no leaf matches a FALSE row is most likely to break, since a mismatch there would let $nor's complement drop a TRUE row): k = 1, k != 1, NOT (k = 1 OR k = 0), label = 1, ts = '300', ts > '100', d = 0, ts = 0, NOT (ts > 0 OR ts < 0), label = '', NOT (d >= 0.5 OR d <= -1) — all lossless, all cache == parquet. The bigint/number split that worried me is handled permissively at every layer: equals(…, strict=false) is ==, hashParquetValue normalizes a safe-integer number to BigInt for INT64 bloom lookups, and compareParquetValues orders mixed bigint/number correctly.
• Predicates the kernel converter declines: does the hint still go down? Yes — deliberately (plan === undefined still calls source.scan({...options}) with where intact). For LIKE, functions and identifier-vs-identifier both converters decline, so no filter and no pruning: identical. For typed literals only icebird converts, and it prunes. No SQL-TRUE row was lost in any case I ran (ts > TIMESTAMP '…', ts >= …, ts < …, ts = …, a > CAST('300' AS BIGINT), a > CAST('300.9' AS INTEGER), a = CAST(300 AS BIGINT), a > CAST(300 AS DOUBLE), s > CAST(3 AS TEXT) — all pruned === noprune, files opened 1-3 of 3). This is where finding 1 lives: the engine-fallback answer does change.
• foldCast fidelity (LLP 0221 flags it unverified). I diffed it against squirreling/src/expression/evaluate.js:735-775 and date.js:133: INTEGER/BIGINT both Math.trunc, FLOAT/DOUBLE both Number, BOOLEAN both Boolean, TIMESTAMP a literal copy of toDate's ^\d{4}-\d{2}-\d{2}(T…)? guard. One genuine divergence, upstream and pre-existing: squirreling returns null on !isFinite, icebird only checks isNaN, so CAST('Infinity' AS BIGINT) throws a RangeError inside icebird's converter. Not reachable through the wrapper's own code and unchanged by this PR; not raised as a finding.
• Where I could not reach. Bloom-filter skipping needs bloom filters in the written files; the repo's writer does not emit them, so canSkipRowGroup's bloom branch was not exercised at runtime. I read it instead: it fires only for $eq/$in, hashParquetValue returns undefined (no skip) for every type/annotation combination it cannot hash exactly, including all TIMESTAMP-annotated INT64, and it proves absence only. I found no way for it to drop a SQL-TRUE row.

Verdict on the argument: it holds. LLP 0221 #pruning-hint is correct as reasoning, and correct on 600+ empirical inputs, for every predicate class I could construct.

---

Also checked, clean

filterStrict: false and the double conversion (priority 2). icebird/src/read.js passes filterStrict: false to parquetReadObjects; the wrapper passes false to matchFilter. Same evaluator, same strictness. The values reaching the wrapper are hyparquet's raw decoded values (read.js builds mapped[field.name] = row[parquetColumnName] after the read, then asyncRow(obj, columns) sets resolved = obj), so the wrapper matches the identical representation hyparquet matched — the claim "agree by construction" is literally true, not merely intended. Every shape the kernel converter emits behaves as the converter's comments assume: {$in: []} folds to [].some(…) → never matches; {col: {$ne: null, $lte: v}} is evaluated as an AND of both operators within one condition object, and equals(undefined, null, false) is true, so an absent cell reads as NULL rather than sneaking past the guard; De Morgan'd $and/$or map to every/some. No mismatch.

appliedWhere honesty (priority 3). Traced all seven paths. scan no-where → icebird's own honest flags. scan converted → true, every row matched. scan declined → false, and appliedLimitOffset also false (which is what keeps execute.js:342's "applied limit/offset without applying where" invariant satisfied). scanColumn no-where → normalizeScanColumn pass-through. scanColumn declined → false + limit/offset withheld from icebird. scanColumn same-column → true, each chunk value matched. scanColumn cross-column → delegates to wrapped.scan and re-checks scan.appliedWhere before claiming. No path claims a predicate it did not apply. The second conversion site the author found (icebergDataSource.scanColumn, whereFilter called again at line 232) is covered: COUNT(ts) WHERE NOT (ts = NULL) and COUNT(ts) WHERE ts NOT IN (300, NULL) both return 0 through both dataSourceForTable and the s3 dataset. The claim that the engine never sends the cross-column scanColumn shape checks out (execute.js:283 gates on plan.hints.columns?.length === 1; aggregates.js:317-324 returns unless the aggregate's single physical column is the projection), and unionSources/the ai-gateway dataset forward {column, where, signal} from that same gated call.

LIMIT/OFFSET (priority 4). Verified over an 8-file / 80-row table with a 1-in-3 NULL pattern: LIMIT 3, LIMIT 3 OFFSET 5, OFFSET 50, LIMIT 0, LIMIT 1000, IS NULL LIMIT 4, and ORDER BY id DESC LIMIT 3 all exact. Ordering is right (filter → offset → limit). Early termination survives: LIMIT 3 opened 1 of 8 files, LIMIT 3 OFFSET 5 opened 2, a direct scan({where, limit: 2}) opened 1 — the for await loop's abrupt completion calls the inner generator's .return(), which ends icebird's file walk. ORDER BY … LIMIT correctly reads all 8. The subtle reason limit/offset must not ride along with the hint is right and non-obvious: icebird's whereResolved would let it cap at offset + limit rows matching its wider filter, and the wrapper's narrowing would then under-return.

Performance (priority 6). Reproduced the methodology on a 20-file / 100k-row table (ts nullable, 1 in 97 NULL), instrumenting the resolver for files opened and bytes sliced, wrapper vs bare icebergDataSource, and re-ran with 12 extra 120-byte STRING columns to test wide rows:

query	files (bare→wrapped)	MB	ms narrow	ms wide
ts >= 5e6 AND ts <= 7e6 (9898 rows)	3 → 3	0.13 → 0.13	48 → 37	48 → 38
ts > 200 (98969 rows, high selectivity)	20 → 20	0.89 → 0.89	134 → 172	144 → 176
COUNT(ts) WHERE ts > 18000000	2 → 2	0.09 → 0.09	8 → 11	8 → 10
COUNT(id) WHERE ts > … (cross-column scanColumn)	2 → 2	0.09 → 0.09	16 → 20	22 → 32
SELECT id (no WHERE)	20 → 20	0.89 → 0.89	203 → 162	253 → 195

Files opened and bytes read are byte-identical in every shape, including wide rows and the cross-column scanColumn path — no pruning lost, and the cross-column rebuild costs no extra IO because icebird already reads the filter's columns for hyparquet's own matching. The worst case is high selectivity: +38 ms over 98969 matched rows ≈ 0.38 µs/matched row, inside the disclosed 0.5-1 µs envelope, so the constant generalises and nothing is hidden. Unfiltered scans are untouched (the wrapper returns the inner scan unchanged).

Tests (priority 7). Re-derived the discrimination independently: with git checkout c483c1a -- src/core/cache/iceberg/store.js, 5 of 6 fail, the 6th (pruning) passes both ways — exactly as stated. Re-derived the corpus claim by running the 41 predicates against the unwrapped icebergDataSource: 27 diverged before, 0 after, matching the body's numbers exactly. The corpus encodes SQL-correct answers, not current behaviour: I spot-checked every expectation against Kleene 3VL by hand (NOT (ts = NULL AND ts = 300) → [1,5], NOT (ts >= 300 OR ts <= 100) → [], ts IN (300, NULL) → [3], label LIKE 'a%' OR ts > 300 → [1,5]) and cross-checked the whole class with the independent oracle above. The parity harness genuinely runs both backends — makeIcebergSource goes through the real dataSourceForTable seam (a fresh iceberg table on disk, not a stub) and makeParquetSource through parquetDataSource, and the harness asserts cache === SQL and parquet === cache separately, so a shared regression cannot hide. The pruning test is honestly labelled as passing both ways by design.

Sequencing hazard with #721 (priority 8). The code carries no guard and LLP 0221 only notes the ordering — but the tests do guard it, more strongly than the doc: if parquet-pushdown.js became a re-export of icebird's converter before icebird takes the NULL fixes, whereToParquetFilter would hand the wrapper the wrong filter and test/core/iceberg-source-parity.test.js would fail on 27 predicates (as would the test/core/parquet-source.test.js corpus). That is an executable tripwire, not just a note. Worth saying so in LLP 0221's last bullet, but not a finding.

LLP hygiene (priority 9). 0221 is unclaimed (single file, no collision), header conventional (Type: Decision, Status: Accepted, Systems: Query, Cache — an existing pair used by 6 other docs), Related targets exist. All anchors referenced from code resolve: 0221#wrapper (iceberg-source.js:68, store.js:556) and 0098#wrapper-duties (iceberg-source.js:129, an <a id> anchor at 0098:81). Claims match code — I checked the API surface it asserts against pinned icebird 0.8.20 and hyparquet 1.28.1: icebergDataSource at icebergDataSource.js:52 takes no filter, scan at :91 and scanColumn at :227 each call whereToParquetFilter(where) at :96 and :232, filter.js:124 declines to skip on $nor, filter.js:145 is matchingNulls. Option 2a is genuinely unreachable, as stated. Option 2b's rejection holds: whereResolved, the filter ? dataEntries.filter(…) prune, and readDataFile's pushed filter all hang off where, so stripping it costs every file, row-group and page prune — my instrumentation puts that at 20 files instead of 2-3 on the perf table. The Extended-by on 0098 is three added lines and touches nothing 0098 settled.

Conventions (priority 10). No em dashes in any changed file. No trailing semicolons in code (the three ;-terminated lines are prose inside comments). No @typedef, no inline import('…') types. @import specifiers are third-party (hyparquet, squirreling/src/types.js) or root-anchored .js (hypaware-plugin-kernel-types.js in the test) — the new file imports no repo-internal types, so the root-anchoring rule is not engaged. hyparquet/src/filter.js is a legitimate deep import (published in hyparquet's exports map, same route the cache already takes into icebird).

Ran: fresh npm install; npm run typecheck (clean); npm test (4030 pass, 1 skipped, 0 fail); node --test test/core/iceberg-source-parity.test.js (6/6, and 5/6 fail with the source hunk reverted); npm run smoke -- local_parquet_export (ok); npm run smoke -- cache_lifecycle_maintenance (ok); plus six purpose-built harnesses (612-predicate fuzz with 3VL oracle and prune-isolation proxy, typed-literal/cast probe, cache-vs-parquet divergence probe, LIMIT/OFFSET + early-termination probe, s3 iceberg round-trip, mixed-type probe) and two perf runs. All scratch files removed; the worktree is clean and untouched. walkthrough_picker_to_first_query not run (known-failing on master, issue #750).

[philcunliffe]

All checks done, worktree clean. Here is the review record.

---

VERDICT: findings

Head reviewed: 8ce8d2965490a215bc61ae2ab2573d2f99a2993f, round 2, reviewed in a detached worktree; nothing written to the branch, git status clean.

Three findings, all minor/nit. None is ship-blocking. The round-2 diff is doc + test + PR body only: src/core/query/iceberg-source.js, src/core/cache/iceberg/store.js and src/core/query/index.js are byte-identical to the round-1 head (blob SHAs 46451f2, 4bd3a1e, f1897c1 on both 026855b and 8ce8d29), so round 1's clearance of the wrapper's behaviour carries over untouched. Both round-1 findings are genuinely fixed and both new tests genuinely discriminate. What is left is that the interesting half of the new claim - the case where the cache path really does return fewer rows than the parquet path - is stated in the doc but is not pinned by any test, even though a one-line case on the existing fixture pins it.

---

1. minor - test/core/iceberg-source-parity.test.js:137 (and :145, :218-228)

The bounded case is ['neg > CAST(-400 AS BIGINT)', [3, 5], { bounded: true }], and on the fixture all three answers are equal: SQL [3,5], cache [3,5], parquet [3,5]. The fixer's own comment admits it ("a case that happens to land on equality"). The consequence is that the bounded branch is dead weight for the only case that uses it: delete { bounded: true } and the case still passes through the strict cache === expected / parquet === cache path. So the round-2 test change pins the subset chain only where the chain is an equality, and the exception the LLP rewrite exists to record - "the cache path may return FEWER rows than the parquet path" - has no executable tripwire at all.

I verified every load-bearing step rather than trusting the case's name:

• The kernel converter really declines it. whereToParquetFilter reaches extractColumnAndValue (src/core/query/parquet-pushdown.js:212), which requires right.type === 'literal'. squirreling parses CAST(-400 AS BIGINT) as {type: 'cast', expr: {type: 'literal', value: -400}, toType: 'BIGINT'}, so it returns {column: undefined} and convertBinary returns undefined. Confirmed at runtime: kernel undefined, versus {neg: {$ne: null, $gt: -400n}} for the bare neg > -400.

• icebird really converts it. staticLiteral recurses into the cast and foldCast('BIGINT', -400) yields -400n, so icebird emits the unguarded {neg: {$gt: -400n}}. Confirmed at runtime.

• It really discriminates. With git checkout c483c1a -- src/core/cache/iceberg/store.js, the case fails with exactly the message the fixer reported: WHERE neg > CAST(-400 AS BIGINT) -> cache [2,3,4,5] is not a subset of parquet [3,5]. So the assertion is not a tautology with respect to the wrapper. It is a tautology only with respect to the bounded flag.

• A strict-subset case exists on the same fixture, one line away. Measured through the real dataSourceForTable and parquetDataSource seams:

  predicate	SQL	cache	parquet
  neg > CAST(-400 AS BIGINT) (the case shipped)	[3,5]	[3,5]	[3,5]
  NOT (neg > CAST(-400 AS BIGINT))	[1]	[1]	[1,2,4]
  NOT (neg >= CAST(-300 AS BIGINT))	[1]	[1]	[1,2,4]
  NOT (neg > CAST(-400 AS BIGINT) OR neg > CAST(-600 AS BIGINT))	[]	[]	[2,4]

  No new fixture, no second table, no timestamps: the negated form of the very predicate already in the corpus is the strict subset.

On the silencing question the answer is partly. The SQL ⊆ cache half is still a real safety check, so marking a case bounded can never hide a row SQL selects going missing - that is the property the design actually rests on. What bounded can hide is the cache widening all the way out to the parquet path's two-valued answer, i.e. a regression that undoes the wrapper for that predicate class. And because no shipped case ever needs bounded, nothing in the suite distinguishes "legitimately bounded" from "silently regressed and relabelled". The structural fix is to make bounded cost something to assert.

Exact fix: (a) add ['NOT (neg > CAST(-400 AS BIGINT))', [1], { bounded: true }] to PARITY_CASES so the strict-subset half of LLP 0221 #consequences is executable; (b) make the bounded opts carry the expected parquet answer as well, e.g. { bounded: true, parquet: [1, 2, 4] }, and assert parquetIds against it alongside the two isSubset checks. Both answers then have to be written down to mark a case bounded, so relabelling a regression is no longer a one-word edit.

Not ship-blocking: the code is correct and unchanged, the doc now states the true bound, and the shipped case does discriminate against removing the wrapper. This is under-pinning, not a defect.

---

2. minor - llp/0221-cache-where-sql-correctness.decision.md:126-127

The rewritten bullet 1 reads: "A predicate the kernel converter owns (whereToParquetFilter converts it, so the wrapper claims appliedWhere) returns the same rows...". The parenthetical asserts an implication that is false. planWhere (src/core/query/iceberg-source.js:238-252) requires three things, not one: the converter succeeds, whereColumns(where) is enumerable, and every predicate column is present in the scan's projection. The PR's own test asserts the counter-case (test/core/iceberg-source-parity.test.js, the unprojected assertion: scan({columns: ['id'], where: ts > 300}) gives appliedWhere: false while the converter converted fine).

That gap is not symmetric across the two backends, and it is measurable. parquetDataSource has no projection gate at all - appliedWhere = Boolean(filter) (src/core/query/parquet-source.js:50-51), because hyparquet unions columnsNeededForFilter into its own read. Measured on the fixture with scan({columns: ['id'], where: neg > -400}):

parquet : appliedWhere=true   rows=[3,5]
cache   : appliedWhere=false  rows=[2,3,4,5]

A predicate the kernel converter unambiguously owns, and the two backends return different rows at the source API. It does not surface through SQL - squirreling folds WHERE columns into the projection, so the engine never sends this shape, and judgement call 4 says so - which is why this is minor rather than major. But bullet 1 was rewritten precisely to make "owns it" a crisp checkable condition, and as written it names the wrong condition.

I also checked the two edge cases the doc's new bound could have missed, and both are fine:

• AND/OR with one owned conjunct and one declined. The kernel converter is strictly all-or-nothing: convertBinary's AND and OR branches both do if (!leftFilter || !rightFilter) return undefined, so there is no partial conversion to reason about. Confirmed at runtime for ts > 100 AND neg > CAST(-400 AS BIGINT), ts > 100 OR neg > CAST(-400 AS BIGINT), ts > 100 AND label LIKE 'a%' and ts > 100 OR label LIKE 'a%' - kernel undefined in all four. Note the asymmetry the doc's bullet 3 correctly covers: for the two CAST forms icebird does convert ({$and: [...]} / {$or: [...]} with an unguarded $gt) so the hint still prunes, while for the two LIKE forms icebird also declines and no hint goes down at all. Both land inside SQL ⊆ cache ⊆ parquet.
• scanColumn versus scan. The declined scanColumn path forwards where to icebird's own second conversion site, so a chunk stream can be narrowed by icebird before the engine sees it. The bound survives: COUNT(id) WHERE neg > CAST(-400 AS BIGINT) gives cache 2 / parquet 2 / SQL 2; COUNT(id) WHERE NOT (neg > CAST(-400 AS BIGINT)) gives cache 1 / parquet 3 / SQL 1; COUNT(neg) WHERE neg > CAST(-400 AS BIGINT) gives 2/2/2; COUNT(id) WHERE NOT (... OR ...) gives cache 0 / parquet 2 / SQL 0. Same chain, same direction, aggregates included. Bullet 3 covers this without needing to say scanColumn explicitly.

Exact fix: replace the parenthetical with the real condition, e.g. "(whereToParquetFilter converts it AND the scan projects the predicate's columns, which is what makes the wrapper claim appliedWhere - see planWhere; squirreling folds WHERE columns into the projection, so the engine always satisfies the second half)".

Not ship-blocking.

---

3. nit - PR body, ## Tests and ## What I could NOT verify; plus llp/0221:146 wording

Three stale or self-contradicting sentences, all introduced by the round-2 changes:

• ## Tests still says the parity test asserts "all three of cache == parquet == SQL". That is now false for the bounded case, which is the case the same PR body's #734 bullet explains at length.
• ## What I could NOT verify still says "the s3 path change is covered only by the shared wrapper's tests, not by an s3 round-trip." Round 2 added exactly that round-trip (test/plugins/s3-query-dataset.test.js:176). A reviewer or releaser reading the caveat list would be told a gap exists that the same PR closed.
• llp/0221:146 (and the identical sentence in the PR body's #734 bullet) says the cache path "may return FEWER rows than the parquet path, not the same rows and not more." The very case the PR pins returns the same rows. The following sentence corrects it (SQL ⊆ cache ⊆ parquet), so the binding claim is right, but "not the same rows" overstates it in the direction opposite to the original overreach.

Exact fix: in ## Tests, say "asserting cache == parquet == SQL, except one CAST case asserted as SQL ⊆ cache ⊆ parquet"; delete the "not by an s3 round-trip" clause from ## What I could NOT verify (or restate it as covered); and change "not the same rows and not more" to "possibly the same rows, never more" in both the LLP and the PR body.

Not ship-blocking.

---

Round-1 findings, re-derived

1. minor, LLP 0221 Consequences parity overreach - fixed, with the residue in finding 1 above. Bullet 1 is now scoped to predicates the kernel converter owns (its condition is imprecise, finding 2, but the overreach is gone). Bullet 3 is rewritten and its new claim is true: I re-derived SQL ⊆ cache ⊆ parquet for the declined class both analytically (cache = icebird's row filter ∩ the engine's two-valued WHERE, both supersets of SQL-TRUE, so their intersection contains SQL-TRUE and is contained in the engine-only answer the parquet path returns) and empirically on eight CAST/typed-literal predicates through dataSourceForTable and parquetDataSource, plus four aggregate forms through scanColumn. The PR body's #734 bullet carries the same corrected text and the #721 bullet is untouched. The PARITY_CASES entry was added with a working isSubset helper and it discriminates against reverting the wrapper. What it does not do is pin the strict-subset exception the bullet is about, which is finding 1.
2. minor, s3 branch had zero coverage - fixed, clean. test/plugins/s3-query-dataset.test.js:176 is a genuine round trip, not a stub: a real createLocalFsBlobStore over an fs.mkdtemp directory, a real iceberg table written through probeTable + commitBatch from @hypaware/format-iceberg, read back through the production buildS3QueryDataset factory (so createIcebergDataSource at query-dataset.js:128 is the code under test, not a re-implementation). It discriminates exactly as reported: with git checkout c483c1a -- hypaware-core/plugins-workspace/s3/src/query-dataset.js it fails with 5 predicate mismatches (ts = NULL -> [2,4], NOT (ts = NULL) -> [1,3,5], ts != NULL -> [1,3,5], ts NOT IN (300, NULL) -> [1,5], ts != 300 -> [1,2,4,5]), and passes restored. Cleanup is fs.rm(dir, {recursive: true, force: true}) in a finally; no hyp-s3-iceberg-* directory survived two runs. The filtered aggregate (COUNT(ts) WHERE NOT (ts = NULL) → 0) covers the second conversion site on the remote branch too.

---

Also checked, clean

Regressions in what round 1 cleared. git diff 026855b 8ce8d29 touches exactly three files: llp/0221-...decision.md, test/core/iceberg-source-parity.test.js, test/plugins/s3-query-dataset.test.js. The three source files named in the brief are byte-identical across the two heads by blob SHA. Nothing round 1 established about the wrapper's behaviour, filterStrict: false agreement, appliedWhere honesty, LIMIT/OFFSET ordering, early termination, or performance is disturbed; I found nothing that contradicts round 1 and did not redo it.

PR body. The #734 bullet now describes the code accurately (declined predicate filtered twice, cache possibly narrower, SQL ⊆ cache ⊆ parquet, test pins the chain). The #721 bullet is unchanged and still states the sequencing hazard in full: "If #721 landed before icebird is fixed, this wrapper would become a no-op and the bug would return, so the two must be sequenced." Round 1's observation that the tests are an executable tripwire for that hazard still holds, and finding 1's proposed extra case strengthens it. The two stale sentences elsewhere in the body are finding 3.

LLP hygiene. 0221 is still the only 022x document in a 207-file corpus, no collision. All anchors survived the Consequences rewrite: #wrapper (referenced by src/core/cache/iceberg/store.js:556 and src/core/query/iceberg-source.js:68), #pruning-hint and #consequences (referenced from the parity test's comments at :53, :56, :58, :222), and #context/#options/#slice all resolve to real headings; LLP 0098#wrapper-duties still resolves to the <a id="wrapper-duties"> at llp/0098-...md:81. The Extended-by on 0098 is the same three added lines and edits nothing 0098 settled. 0221 is introduced by this PR (026855b), so editing it in round 2 does not touch a merged Accepted record.

Conventions. No U+2014 anywhere in the repo (grep -rlP '\x{2014}' over llp/, src/, test/, hypaware-core/ returns nothing). No trailing semicolons in the new test code. No @typedef, no inline import('...') types. The new @import in test/plugins/s3-query-dataset.test.js is the pre-existing root-anchored '../../hypaware-plugin-kernel-types.js'; the new imports are plain runtime imports of repo-internal .js files, correct as written. The one -- in the parity test's JSDoc at :49 is prose punctuation, not U+2014, and has precedent in the repo (test/plugins/openclaw-backfill.test.js:402, :404) - not a finding.

Ran: fresh npm install (exit 0); npm run typecheck (clean); npm test (4031 pass, 1 skipped, 0 fail); node --test test/core/iceberg-source-parity.test.js and node --test test/plugins/s3-query-dataset.test.js at head and with each of the two source hunks reverted to c483c1a (discrimination re-derived, then restored to 8ce8d29); npm run smoke -- local_parquet_export (ok); npm run smoke -- cache_lifecycle_maintenance (ok); plus four purpose-built probes (both converters over 12 CAST/typed-literal/mixed AND-OR shapes; cache-versus-parquet over 8 CAST predicates through the real dataSourceForTable seam; the narrow-projection scan asymmetry; the aggregate/scanColumn bound over 4 forms). walkthrough_picker_to_first_query not run (known-failing on master, issue #750). All scratch files removed; git status is clean.

[philcunliffe]

neutral triage - ship

Review budget exhausted at 2 rounds with three residual findings open. Triage
judged each a true blocker (a production defect) or a preference.
All three are preferences. This PR can merge. Deferred to #754.

Weighed against the baseline, which is the thing that matters here: without this
PR, ts = NULL over every intrinsic dataset (ai_gateway_messages, traces,
logs) returns the NULL rows while SQL says none, with appliedWhere: true
making the wrong answer final. That is shipped, silent, wrong data today. Every
residual is strictly smaller than that.

The one finding triage re-derived rather than inherited was the appliedWhere
condition (finding 2), because it is the only residual that could have meant a user
seeing wrong rows. Round 2 measured a real source-API asymmetry:
scan({columns: ['id'], where: neg > -400}) gives parquet appliedWhere=true rows
[3,5] and cache appliedWhere=false rows [2,3,4,5]. The question was whether
SQL can ever send that shape. It cannot, established by reading squirreling's
planner rather than assuming: plan/columns.js:142 folds every WHERE identifier
into the scanned table's projection unconditionally, and plan/plan.js:220-221
attaches hints.where only when there are no joins. Confirmed empirically with a
spy data source over 24 query shapes (narrow projections, CAST predicates,
aggregates, DISTINCT, GROUP BY, subqueries, CTEs, EXISTS): 30 scans, 28 carrying
where, zero with a predicate column missing from the projection. And the failure
mode if it were reached is safe rather than wrong: the wrapper answers
appliedWhere: false, an honest decline the engine re-filters. The folding
invariant is also not a new dependency this PR smuggled in, union-source.js:28-35
already documents relying on it.

On the missing test tripwire (finding 1): the suite as shipped does catch the
regression that matters. The bounded case fails when the wrapper is reverted
("cache [2,3,4,5] is not a subset of parquet [3,5]"). What the missing case covers
is narrower, the cache widening back to the two-valued answer for the declined
class, and that widened answer is exactly what the parquet-file path returns for
the same predicate, which is the already-tracked #734 class, not the #744 class
this PR exists to fix.

Verified at head 8ce8d29 before deciding: npm test 4031 pass / 0 fail / 1
pre-existing skip, npm run typecheck clean, node --test over both new test
files 13/13, smokes local_parquet_export and cache_lifecycle_maintenance ok.

Sequencing note for whoever merges. PR #721 is now unstuck and under review; it
replaces this repo's whereToParquetFilter with icebird's. This wrapper calls that
function. icebird@0.8.22 carries the NULL fixes, so #721 no longer threatens to
make the wrapper a no-op, but the two have never been tested against each other and
the ordering changes what one test here means. Details on #721's thread.

[philcunliffe]

Hold before merging: this PR may be obsoleted by #721

This PR is held ready-to-merge on a triage that said ship. That triage predates
evidence that arrived on #721 minutes later, and a human should read this before
merging.

PR #721 (Delete our duplicate WHERE-pushdown converter, use icebird's) was
unstuck and reviewed at head aea733b3, which bumps icebird 0.8.20 to 0.8.22,
hyparquet 1.28.1 to 1.28.2, squirreling 0.15.2 to 0.15.3. Its review ran
this PR's own 42-predicate parity corpus against the iceberg cache tier at
that head, through the real dataSourceForTable seam, with no
withSqlCorrectWhere present:

=== iceberg cache tier, NO #751 wrapper, icebird 0.8.22 ===
all 42 correct
=== cache tier filtered aggregates (scanColumn) ===
all 42 correct

So the bump closes #744 by itself. This PR's wrapper then converts with icebird's
converter, forwards the same predicate to icebird for pruning, and re-applies the
identical filter with the identical evaluator (matchFilter, filterStrict: false)
over rows that already passed it. That is not a correctness problem; it is a cost
one, and it lands on exactly what LLP 0098 exists to protect: rowMatches
materializes every predicate column per row, and the cross-column scanColumn
branch rebuilds the column stream from a row scan in 1024-value batches.

Two assertions here fail outright once #721 lands, rather than merely ceasing to
discriminate. In the cache column stream is NULL-correct and still claims the predicate:

assert.equal(sameColumn.appliedWhere, true)   // becomes false
assert.deepEqual(sameColumn.values, [])       // becomes [100, null, 300, null, 500]

because icebird 0.8.22 declines a NULL-literal comparison rather than folding it
to a never-match, so planWhere returns undefined and the wrapper streams the raw
column. A direct scanColumn call has no engine above it to re-filter.

Separately, the bounded case flips exactly as predicted: neg > CAST(-400 AS BIGINT) now folds instead of declining, so the corpus loses its only exercise of
the declined-CAST path, and the 12-line comment explaining that case becomes wrong,
along with three section comments asserting icebird pushes {ts: {$eq: null}}, bare
unguarded bounds, and $nor. All three were true of 0.8.20 and are false of 0.8.22.

The choice

Both PRs fix #744, by different mechanisms. On today's master (icebird 0.8.20) the
bug is live, so whichever lands, one of them must.

• Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721 first, then close this PR as obsoleted. Its premise ("icebird's converter
  is wrong three ways") is retired by the bump. The 42-case parity corpus is worth
  keeping: re-point it at the unwrapped dataSourceForTable, drop
  withSqlCorrectWhere, src/core/query/iceberg-source.js, the bounded option
  and its subset branch, and keep every expected row set verbatim. The Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721 reviewer
  ran exactly that shape and it is 42/42 green.
• This PR first, for schedule reasons. Then Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721's rebase must delete
  withSqlCorrectWhere and its test file in the same commit, or it ships a
  redundant filter application on the cache tier's hot path.

Either way, LLP 0221 and LLP 0222 both currently claim to settle #744 by different
mechanisms, so whichever lands second needs a Superseded-by: on the other.

Neutral is not merging or closing anything here. Full evidence is in the round-1
review on #721; #754 still tracks this PR's own deferred findings, which stay valid
only if this PR lands.

[philcunliffe]

neutral is holding this PR: #721 landed, and this one should not be merged as written

What changed. PR #721 merged to master as 8c08185f at 00:16 UTC, bumping
icebird 0.8.20 to 0.8.22, hyparquet 1.28.1 to 1.28.2, squirreling 0.15.2 to 0.15.3 and replacing this repo's WHERE converter with icebird's. That is the
sequencing decision both this thread and #721's thread flagged, taken in the
recommended order.

Why this PR cannot proceed unchanged. Everything above the fold still says
MERGEABLE, CLEAN, 9 checks green. That green is stale: it ran against the
pre-#721 base, and GitHub does not re-run CI when the base moves. Against today's
master:

• This PR's premise is retired. Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721's review ran this PR's own 42-predicate
  parity corpus against the iceberg cache tier at Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721's head with no
  withSqlCorrectWhere present: 42/42 correct on the row path and 42/42 on the
  filtered-aggregate path. Cache-path queries answer NULL-literal comparisons with IS NULL semantics #744 is closed by the dependency bump. The wrapper has
  nothing left to repair.
• What it would ship is a cost, not a fix. withSqlCorrectWhere now converts
  with icebird's converter, forwards the same predicate to icebird for pruning, and
  re-applies the identical filter with the identical evaluator over rows that
  already passed it. rowMatches materializes every predicate column per row and
  the cross-column scanColumn branch rebuilds the column stream from a row scan in
  1024-value batches. That is the per-row materialization LLP 0098 exists to
  prevent.
• Two assertions in this PR fail outright, not merely stop discriminating. In
  the cache column stream is NULL-correct and still claims the predicate,
  appliedWhere becomes false and values becomes [100, null, 300, null, 500],
  because icebird 0.8.22 declines a NULL-literal comparison where this repo's
  converter folded it to a never-match, so planWhere returns undefined and the
  wrapper streams the raw column. A direct scanColumn call has no engine above it
  to re-filter.
• The bounded parity case flips too: neg > CAST(-400 AS BIGINT) now folds
  instead of declining, so the corpus loses its only exercise of the declined-CAST
  path, and the comment explaining that case becomes wrong along with three section
  comments asserting icebird pushes {ts: {$eq: null}}, bare unguarded bounds, and
  $nor. All three were true of 0.8.20 and are false of 0.8.22.

What it needs from you. A disposition, and neutral will not pick for you because
closing a PR is not an act it performs:

1. Close this PR as obsoleted (recommended by both reviews), and land its
   parity corpus separately: re-point the 42 cases at the unwrapped
   dataSourceForTable, drop withSqlCorrectWhere, src/core/query/iceberg-source.js,
   the bounded option and its subset branch, and keep every expected row set
   verbatim. Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721's reviewer ran exactly that shape and it is 42/42 green. The corpus
   is the durable value here: it is the cross-backend tripwire that would catch this
   class returning.
2. Or keep it, in which case it needs a rebase that deletes the wrapper and its
   test file, at which point it is only the corpus, i.e. option 1 by a longer road.

Either way LLP 0221 needs a Superseded-by: pointing at LLP 0222, since both
now claim to settle #744 and 0222 is the one on master.

Also note #754, which tracks this PR's own deferred findings, is neutral:stuck
waiting for this PR to merge. If you close this one, close #754 with it: its three
items are all about withSqlCorrectWhere and LLP 0221.

How to unstick. Reply on this thread, or close the PR. Neutral monitors the
thread and will re-engage on its next tick. The neutral:stuck label is here mainly
so this PR stops presenting itself as ready-to-merge while its green is stale.

[platypii]

The sequencing hazard called out in "Interaction with #721" resolved the good way: #721 landed together with an icebird bump to 0.8.22, which has the NULL work converged upstream (De Morgan instead of $nor, null guards on $ne/$nin, never-match for NOT IN lists holding NULL, declines answered by the three-valued engine in squirreling 0.15.3, hyparquet 1.28.2 rejecting null cells in bare relational operators). I ran this PR's parity corpus (26 predicates plus the filtered aggregates) against master at 8c08185 through storage.dataSourceForTable and all of them now match SQL, so #744 is closed and the wrapper no longer has a bug to work around.

Suggestion for salvaging this PR as a tests-only change, since the parity coverage is the part still worth landing:

• Drop src/core/query/iceberg-source.js, its export in src/core/query/index.js, the store.js wiring, the s3 query-dataset.js wiring, LLP 0221, and the LLP 0098 forward-ref. LLP 0222 (landed with Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721) now records where converter ownership lives.
• Keep test/core/iceberg-source-parity.test.js, rewritten to build its sources through dataSourceForTable directly instead of the wrapper, still asserting cache == parquet == SQL over the corpus, the filtered aggregates, and the two-data-file pruning assertion.
• The wrapper-specific tests change meaning rather than dying: the appliedWhere-honesty and LIMIT/OFFSET tests now pin icebird 0.8.22's own behavior (it declines LIKE/functions/casts and holds back position pushdown under a WHERE), which is exactly the contract Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721 now depends on and worth pinning from this side.
• The CAST/typed-literal subset-chain test (SQL ⊆ cache ⊆ parquet) can likely tighten to full equality: a declined predicate now falls to a three-valued engine on both paths, so the two-valued divergence it was tolerating should be gone. Worth checking rather than assuming.
• The s3 test additions are worth keeping in the same adapted form, since that dataset reaches the same icebergDataSource.

Happy to push the reduction here if useful, or it can land as a fresh tests-only PR.

[philcunliffe]

unstuck, taking the reduction

neutral:stuck removed. Thanks for the disposition, and for re-running the corpus
against master at 8c08185 yourself rather than taking the #721 review's word for
it - that is the second independent confirmation that #744 is closed by the bump.

Taking you up on the offer the other way round: neutral will push the reduction here
rather than leaving it to you, on this branch so the PR keeps its history and its
review record. Dispatching now to do exactly what you listed:

• drop src/core/query/iceberg-source.js, its export in src/core/query/index.js,
  the store.js wiring, the s3 query-dataset.js wiring, LLP 0221, and the LLP 0098
  forward-ref;
• keep test/core/iceberg-source-parity.test.js, rebuilt through
  dataSourceForTable directly, still asserting cache == parquet == SQL over the
  corpus, the filtered aggregates and the two-data-file pruning assertion;
• keep the appliedWhere-honesty and LIMIT/OFFSET tests in their changed meaning, as
  pins on icebird 0.8.22's own contract;
• keep the s3 additions in adapted form.

On the CAST/typed-literal subset chain: your instinct that it should tighten to full
equality now is being checked rather than assumed, which is the right call and
was already the sharp edge here. Round 2 of this PR's review found that flag was dead
weight even under the old code (the shipped case landed on equality, so the subset
branch never exercised a strict subset), and #766 tracks a related "assertion that
cannot fail" problem. So the worker is told: measure the three answers, and if they
are equal, assert equality and delete the bounded machinery entirely rather than
leaving an unexercised escape hatch behind. If they are not equal, say so with the
numbers.

Once it lands, this PR re-enters the ladder at its new head: fresh review round, then
triage. #754, which tracks this PR's own deferred findings, is currently held on
this PR merging; its three items are all about withSqlCorrectWhere and LLP 0221, so
if the reduction lands as described I will close it as moot rather than leave it
pointing at deleted code.

[philcunliffe]

neutral review - fresh round 1 at the reduced head

Head reviewed: 6707ad59e30b2eb0e97043e51eee935568174575. MERGEABLE, all 9 checks
SUCCESS. Reviewed in a detached worktree; nothing was written to the branch.

Why this is a fresh round 1 and not round 3. The two rounds already in this
thread reviewed a different change: withSqlCorrectWhere plus LLP 0221, all of which
this head deletes. The reconciler's rung reads triage because it counts comments,
not diffs, so it would have triaged 595 lines of test nobody had read. Reviewing was
the right call over honouring the counter.

Finding 1 is mine. I told the reduction worker to preserve HTML-comment markers
in the PR body exactly as they are. That was wrong for this one: the
<!-- neutral-triage: 8ce8d296... #754 --> marker records a ship decision about
code this head deletes, and points at #754, which I closed as moot an hour ago. It is
the only machine-readable disposition on the PR and it currently claims this head is
triaged when nothing has triaged it. I have removed it from the body; the fresh
review and triage will mint their own.

The change itself holds up, and the reviewer earned that verdict by injection
rather than reading. Every test in both files was checked by breaking something and
measuring, including the two results that matter most:

• The item-4 equality is a property, not a fixture coincidence. The nine-shape
  table reproduced exactly, and the mechanism was established two ways: icebird
  0.8.22's staticLiteral genuinely folds casts (every one of the nine converts to
  a real filter with appliedWhere: true on both tiers), and
  src/core/query/parquet-pushdown.js is now a one-line re-export of icebird's
  converter, so the two tiers are literally the same module object and cannot
  decline differently. There is no residual predicate class that could produce
  SQL subset cache subset parquet. Deleting bounded and isSubset was right.
• The cross-backend half earns its keep on its own. Under a parquet-tier-only
  regression (appliedWhere = Boolean(hints.where)), 12 tests fail, all of the
  form "cache correct, parquet wrong", with the SQL half entirely green. And under
  the most realistic drift, root hyparquet@1.28.1 with icebird's nested 1.28.2, the
  corpus fires 7 pure cache-versus-parquet divergences. That is the regression LLP
  0222#hyparquet-floor exists to prevent.

The reviewer also confirmed the reduction worker's correction to the human
guidance: icebird 0.8.22 declines LIKE, function calls, NULL literals and
column-vs-column, but folds casts. The kept flag test pins the real split, so no
copied assumption survived.

---

VERDICT: findings

Head reviewed: 6707ad59e30b2eb0e97043e51eee935568174575, reviewed as a fresh round 1 on its merits, in a detached worktree. Nothing was written to the branch; git status is clean.

Four findings, none ship-blocking: one minor (a stale marker) and three nits (two wrong sentences in the PR body, one comment that overstates what its assertion pins). The change itself holds up. The deletion is complete and exact, all six parity tests and the s3 round trip discriminate against at least one plausible regression that I injected and measured, item 4's equality claim is a real property rather than a fixture coincidence, and deleting the bounded/isSubset machinery was the right call.

---

1. minor - PR body, trailing marker <!-- neutral-triage: 8ce8d2965490a215bc61ae2ab2573d2f99a2993f #754 -->

The marker records a ship triage at 8ce8d296, which is three commits behind this head and was a fundamentally different change (it shipped withSqlCorrectWhere, 595 lines of production code plus LLP 0221, all of which this head deletes). It also defers to #754, which I confirmed is now CLOSED / NOT_PLANNED. So the marker asserts a merge-readiness decision about code that no longer exists, pointing at a tracking issue that has been closed as moot.

Why it matters: it is the only machine-readable disposition on the PR, and it currently claims this head is triaged when nothing has triaged it. That is precisely the confusion that produced this review round. A human skimming the body sees a ship verdict attached to a SHA they cannot match to the diff.

Exact fix: delete the marker line from the PR body. The fresh review and triage rounds on 6707ad5 will mint their own.

---

2. nit - PR body, ## Kept, and what each surviving test would catch, first bullet

"against the pre-#721 stack (icebird 0.8.20, hyparquet 1.28.1, squirreling 0.15.2) five of this file's six tests fail, the corpus test with 27 wrong answers."

I installed exactly that trio and replayed. Five of six do fail (correct), but the corpus reports 29 wrong answers, not 27. The 27 is round 1's number for the 41-case corpus; the corpus grew to 47 and the CAST section it gained contributes two more old-stack divergences (neg > CAST(-400 AS BIGINT) -> cache [2,3,4,5], SQL says [3,5] and neg != CAST(-300 AS BIGINT) -> cache [1,2,4,5], SQL says [1,5]). 29 = 27 + 2, so the stale number was carried forward rather than re-measured.

Why it matters: it is a small number in the one paragraph whose entire job is to prove this tests-only PR discriminates. A carried-forward measurement in a verification claim is the same class of error the PR body elsewhere warns about.

Exact fix: change "27 wrong answers" to "29 wrong answers".

---

3. nit - PR body, ## The CAST subset chain: measured, then tightened to equality, final paragraph

"Both tiers now run icebird's converter, which folds the cast, pushes a guarded filter and claims appliedWhere on both sides."

Seven of the nine shapes in the body's own table push a bare, unguarded operator. guardNull (icebird/src/sql/whereFilter.js:112-115) guards $ne only; $nin is guarded separately in convertInValues. The filters I measured for the table are {neg: {$gt: -400n}}, {neg: {$lte: -400n}}, {neg: {$lt: -300n}}, {ts: {$gt: 300n}}, {label: {$gt: '3'}} and so on, all unguarded. Their SQL-correctness on null cells comes from the hyparquet 1.28.2 floor, not from a guard. The test file's own comment at test/core/iceberg-source-parity.test.js:95-98 states this correctly ("icebird pushes these bare ... which is only correct because hyparquet >= 1.28.2 rejects a null cell"), so the body contradicts the code it ships.

Why it matters: it misattributes the mechanism that makes the equality claim true. A reader who believes the guard is doing the work would not treat the hyparquet floor as load-bearing, and the floor is the single dependency constraint the whole equality rests on. I confirmed by measurement: with root hyparquet at 1.28.1 and icebird's nested copy left at 1.28.2, the corpus fires seven cache-versus-parquet divergences.

Exact fix: "pushes a bare bound (guarded only for !=/NOT IN; the rest are SQL-correct on null cells because of the hyparquet >= 1.28.2 floor, LLP 0222#hyparquet-floor) and claims appliedWhere on both sides".

---

4. nit - test/plugins/s3-query-dataset.test.js:216-217

// the typed-literal fold has to reach the remote tier too, or an
// s3-backed dataset scans unpruned where the local cache does not
['ts > CAST(300 AS BIGINT)', [5]],
['NOT (ts > CAST(300 AS BIGINT))', [1, 3]],

The comment claims these cases pin the fold (a pruning property). They do not, and I proved it: with icebird's staticLiteral cast branch removed so casts are declined instead of folded, node --test test/plugins/s3-query-dataset.test.js is 7 pass / 0 fail. The assertions are on rows only, and the row answer is identical whether the cast is folded and pushed or declined and answered by the three-valued engine. Nothing in this file measures files opened.

The cases are not decoration - NOT (ts > CAST(300 AS BIGINT)) is the only predicate in the s3 corpus that produces a bare unguarded relational operator over a nullable column, and it is the sole case that fires when the remote tier drops below the hyparquet floor (measured: with both tiers on 1.28.1 it is the one failure, -> [1,2,3,4], SQL says [1,3]). But that is a different property from the one the comment names.

Why it matters: the comment tells the next maintainer these cases guard pruning. If pruning coverage is later added elsewhere, they would be deleted as redundant, silently removing the s3 tier's only floor tripwire.

Exact fix: replace the comment with what the assertion actually pins, e.g. // the negated form is the only bare unguarded relational operator in this corpus, so it is where a hyparquet-floor regression on the remote tier shows up (LLP 0222#hyparquet-floor). If the fold really should be pinned here, it needs a files-opened assertion like the local pruning test's, not a row assertion.

---

The deletion, verified

Complete and exact. Established by blob SHA rather than by reading:

• git diff --stat origin/master HEAD -- src/ hypaware-core/ llp/ bin/ docs/ package.json package-lock.json is empty. git diff --name-status origin/master HEAD is exactly two lines: A test/core/iceberg-source-parity.test.js, M test/plugins/s3-query-dataset.test.js.
• src/core/query/index.js, src/core/cache/iceberg/store.js, hypaware-core/plugins-workspace/s3/src/query-dataset.js and llp/0098-scancolumn-where-pushdown.decision.md are byte-identical to master (blob SHAs 405ead7e, 5ee9a2e8, 9d5a3f30, a6b68e70 on both origin/master and HEAD).
• src/core/query/iceberg-source.js exists on neither master nor HEAD, so there is no residue of the module itself.
• LLP 0098 carries no Extended-by: or Superseded-by: line at all (grep for both returns nothing), so no forward-ref stub was left behind. llp/ contains no 0221 file; the only 022x document is 0222-one-pushdown-converter.decision.md.
• Tree-wide greps over everything except node_modules and .git: withSqlCorrectWhere 0 hits, LLP 0221 0 hits, bare 0221 0 hits. iceberg-source has exactly one hit, test/plugins/s3-query-dataset.test.js:190, and it is what the worker reported: a prose cross-reference to the kept test file's own path inside a comment. Confirmed by reading it.
• All four LLP 0222#... anchors the new test cites (#context, #decision, #hyparquet-floor, #consequences) resolve to real headings in llp/0222-one-pushdown-converter.decision.md. test/core/llp-ref-hygiene.test.js passes.

Item 4, re-derived

I rebuilt the table myself through the real dataSourceForTable and parquetDataSource seams (fresh iceberg table on disk, parquet written at rowGroupSize: 2), with the SQL column written out by hand from Kleene three-valued logic rather than read back from either engine, and I additionally captured the converted filter and both appliedWhere flags:

predicate	SQL	cache	parquet	pushed filter	appliedWhere (cache / parquet)
neg > CAST(-400 AS BIGINT)	[3,5]	[3,5]	[3,5]	{neg:{$gt:-400n}}	true / true
NOT (neg > CAST(-400 AS BIGINT))	[1]	[1]	[1]	{neg:{$lte:-400n}}	true / true
NOT (neg >= CAST(-300 AS BIGINT))	[1]	[1]	[1]	{neg:{$lt:-300n}}	true / true
NOT (neg > CAST(-400 AS BIGINT) OR neg > CAST(-600 AS BIGINT))	[]	[]	[]	{$and:[{neg:{$lte:-400n}},{neg:{$lte:-600n}}]}	true / true
neg != CAST(-300 AS BIGINT)	[1,5]	[1,5]	[1,5]	{$and:[{neg:{$ne:null}},{neg:{$ne:-300n}}]}	true / true
ts > CAST('300' AS BIGINT)	[5]	[5]	[5]	{ts:{$gt:300n}}	true / true
neg > CAST(-400.9 AS INTEGER)	[3,5]	[3,5]	[3,5]	{neg:{$gt:-400}}	true / true
neg > CAST(-400 AS DOUBLE)	[3,5]	[3,5]	[3,5]	{neg:{$gt:-400}}	true / true
label > CAST(3 AS TEXT)	[1,3,5]	[1,3,5]	[1,3,5]	{label:{$gt:'3'}}	true / true

The numbers reproduce exactly, including NOT (neg > CAST(-400 AS BIGINT)), which round 2 measured as a genuine strict subset (SQL [1], cache [1], parquet [1,2,4]) under the old stack.

Verdict on the mechanism claim: correct, and the worker's correction to the human's guidance is right. This is a property, not a fixture coincidence, for two reasons I established independently of the numbers:

1. Casts are folded, not declined. icebird/src/sql/whereFilter.js:143-151 (staticLiteral) recurses into node.type === 'cast' and calls foldCast, which handles TEXT/STRING/VARCHAR, INTEGER/INT, BIGINT, FLOAT/REAL/DOUBLE, BOOLEAN and TIMESTAMP. The right-hand column above is the runtime proof: every one of the nine converts to a real filter and appliedWhere is true on both tiers. The human's guidance said 0.8.22 "declines LIKE/functions/casts"; the code declines LIKE (:83), NULL literals (:90), and anything where neither operand is an identifier-with-a-static-literal-counterpart (:85-86, which is what rejects function calls and column-versus-column) - but it folds casts. The worker's correction is accurate and the four decline classes it lists are exactly the four in the source.
2. The two tiers cannot decline differently. src/core/query/parquet-pushdown.js is now a one-line export { whereToParquetFilter } from 'icebird/src/sql/whereFilter.js', and parquetDataSource imports from it (src/core/query/parquet-source.js:7). So the converter is literally the same module object on both sides. The old divergence existed only because this repo carried a second converter that refused a cast operand; that converter is gone from master. There is therefore no residual predicate class that can produce SQL ⊊ cache ⊊ parquet.

Deleting bounded, isSubset and the subset branch was correct, and deleting rather than retaining it is the stronger choice: round 2's own review had already established the flag was dead weight even under the old code, and with the shared converter there is no longer any case that could need it. The negated forms now ship as plain equality cases, which is strictly stronger than the subset assertion they replace. I found no case where the deleted machinery would have been protecting something real.

What each kept test would catch

I did not accept the PR body's per-test claims. For each, I injected a regression and measured. All injections were to node_modules or to a tracked file restored immediately with git checkout; the worktree is clean and the full suite is green at the end.

cache and parquet backends answer the corpus identically, and answer it the way SQL does (47 predicates, count verified). Catches, measured:

• Shared regression across both tiers. Old trio icebird 0.8.20 / hyparquet 1.28.1 / squirreling 0.15.2: fails with 29 wrong answers, every one of the form "cache X, SQL says Y". Note that on this regression the cache-versus-parquet half fires zero times, because both tiers move together. The expectations really are hand-written Kleene truth, not a recording, which is what makes the shared case catchable at all.
• Cache-tier-only regression below the converter. I injected an off-by-one into icebird/src/prune.js boundsOpMightMatch's $lte branch (c <= 0 to c < 0), a data-file bound prune the parquet tier does not have. Result: WHERE neg <= -500 -> cache [], SQL says [1] and WHERE neg <= -500 -> cache [], parquet [1]. So the worker's answer to "what could ever make the two tiers differ" is correct and I verified it directly: they differ below the converter.
• Parquet-tier-only regression, which the SQL half structurally cannot see (the corpus compares SQL to cacheIds only). I set appliedWhere = Boolean(hints.where) in src/core/query/parquet-source.js:50. Result: 12 failures, all of the form "cache [correct], parquet [1,2,3,4,5]", SQL half entirely green. This is the cross-backend half earning its keep on its own.
• The hyparquet floor drifting on one tier only, which is the most realistic version of the above: root hyparquet@1.28.1 while npm nests icebird's pinned 1.28.2. Result: 7 pure cache-versus-parquet divergences (neg > -400 -> cache [3,5], parquet [2,3,4,5] and similar). This is the concrete regression LLP 0222#hyparquet-floor exists to prevent, and this test is where it surfaces.
• The Cache-path queries answer NULL-literal comparisons with IS NULL semantics #744 bug itself. Removing if (value === null) return undefined from convertBinary: fails.
• Does not catch a loss of cast folding, correctly so: both tiers decline together and the three-valued engine answers correctly, so the rows are right. That gap is covered by tests 3, 4 and 6 below.

filtered aggregates take the same NULL semantics as the row scan. The body claims it catches "the two conversion sites drifting apart, which the row corpus alone would not see". Verified precisely: I patched the second occurrence only of const appliedWhere = where !== undefined && filter !== undefined in icebird/src/sql/icebergDataSource.js (line 232, inside scanColumn) to over-claim. Result: test 1 passes, test 2 fails. The claim is exact, and this test is not redundant with the row corpus.

the cache column stream reports appliedWhere honestly. Fires on the cast-fold loss (its cast assertion at :310), on the #744 regression, and on the hyparquet floor regression. The NULL literal assertion (appliedWhere: false, values [100, null, 300, null, 500]) is the one the round-2 stuck note predicted would flip under 0.8.22, and it is now written to 0.8.22's actual behaviour with the reason stated (:276-278). This is not "encoding current behaviour" in the bad sense: for a declined predicate on a scanColumn with no engine above it, returning the raw column with appliedWhere: false is the only honest answer, so the expectation is SQL truth about the contract, not a recording.

both backends agree on which predicates are converted and which are declined. This is where a copied assumption from the human's guidance would have shown, and it does not: the folded cast case expects true, which is what icebird actually does, contradicting the guidance's "declines casts". Every one of the seven shapes matches the source I read. Fires on the cast-fold loss (folded cast flips to false), on the #744 regression (NULL literal flips to true), and on a limit/offset flag regression (its appliedLimitOffset check at :368). The unprojected column case expecting true on both tiers is real: with the wrapper gone there is no projection gate, so the asymmetry round 2 measured is genuinely gone, and this case pins that it stays gone.

LIMIT and OFFSET are held back under a WHERE. I changed canPushOffset = !where && !hasDeletes && !pruned to whereResolved && !hasDeletes && !pruned at both sites in icebergDataSource.js, the plausible regression of treating a converted WHERE as safe for position pushdown. Result: test 5 fails (expected: false, actual: true), and test 4 fails too. It also fires under the hyparquet floor regression. Slice-before-filter ordering is still covered end to end: ts IS NOT NULL LIMIT 2 asserts [1, 3], which is [1] if the slice runs first, and LIMIT 2 OFFSET 1 asserts [3, 5].

a filtered cache scan still prunes whole data files. The new discrimination is real, verified by a better method than the worker's. Rather than swapping the predicate for a declined shape (which only proves the harness measures something), I removed cast folding from icebird's converter so the contract regresses: not ok 6 ... cast-bounded scan opened 2 data files, the exact message the body predicts. This is the decisive result of the review, because on that same injection tests 1 and 2 stay green: the row corpus cannot see a loss of cast folding at all, and this test plus tests 3 and 4 are the only things that can. The test earns its place.

test/plugins/s3-query-dataset.test.js, the BlobStore round trip. Genuinely drives the production buildS3QueryDataset factory over a real table written through probeTable + commitBatch into a real createLocalFsBlobStore, so createIcebergDataSource's lazy dynamic import is the code under test. Fires under the old trio, under the #744 regression, and under a hyparquet floor regression on the remote tier. Finding 4 above is about what its two CAST cases actually pin versus what their comment says.

Nothing here is decoration. Every test in both files fails under at least one regression I injected, and no two tests are covered by an identical set.

Also checked, clean

Ran, all at this head with a fresh npm install: npm test (4036 pass, 0 fail, 1 pre-existing skip, matching the body); npm run typecheck (clean); node --test test/core/iceberg-source-parity.test.js test/plugins/s3-query-dataset.test.js test/core/llp-ref-hygiene.test.js (24/24, matching the body); npm run smoke -- local_parquet_export (ok); npm run smoke -- cache_lifecycle_maintenance (ok). Plus one purpose-built probe (the item-4 re-derivation with converter and flag capture) and nine dependency-swap or injection runs, each restored and re-verified green afterwards. package_bin_boot and walkthrough_picker_to_first_query not run (known red on master, #758 and #750).

Corpus expectations spot-checked against Kleene 3VL by hand, not against either engine: NOT (ts IS NULL OR ts > 300) -> [1,3] (the only disjunct that is ever definite over a NULL row), NOT (ts >= 300 OR ts <= 100) -> [], NOT (ts = NULL AND ts = 300) -> [1,5] (a never-match leaf must not swallow its sibling), ts IN (300, NULL) -> [3] versus ts NOT IN (300, NULL) -> [], ts BETWEEN NULL AND 500 -> [], and all six CAST cases including neg > CAST(-400.9 AS INTEGER) -> [3,5] (truncation toward zero) and label > CAST(3 AS TEXT) -> [1,3,5] (string '3' below 'a'). All correct. No expectation encodes current behaviour where SQL truth differs.

PR body, everything else. It describes what the PR now is. It states plainly "This PR closes nothing. #744 is already closed by #721", and gh pr view 751 --json closingIssuesReferences returns [], so it cannot auto-close anything. #744 is CLOSED / COMPLETED independently. The "Removed" list matches the diff exactly. The item-4 table reproduces. "five of this file's six tests fail" reproduces. npm test and node --test counts reproduce. "Verified sensitive: ... fails with 'cast-bounded scan opened 2 data files'" reproduces verbatim. The claim that the round-2 projection asymmetry is gone reproduces. Findings 2 and 3 are the only two sentences I could falsify.

Conventions. No U+2014 anywhere in either changed file (grep -rlP '\x{2014}' returns nothing). No trailing semicolons. No @typedef. No inline import('...') types. Type imports are declared at the top via @import: third-party (hyparquet, squirreling/src/types.js) plus the root-anchored '../../hypaware-plugin-kernel-types.js', which is the correct repo-root-anchored .js form; neither file imports a repo-internal .d.ts, so the routing rule is satisfied rather than bypassed. Both files carry // @ts-check and typecheck clean. The new @ref LLP 0222#consequences [tests] annotation uses the documented relation syntax, is attached without an intervening blank line, and says something the filename does not (that this file is the guardrail from the cache tier's side, with test/core/parquet-source.test.js covering the other tier alone).

Test hygiene. Every test wraps its temp directory in try/finally with fs.rm(dir, { recursive: true, force: true }); no hyp-iceberg-parity-* or hyp-s3-iceberg-* directory survived my repeated runs. The corpus test rebuilds the parquet source per predicate, which is wasteful but not incorrect, and the whole file runs in well under a second.

Worktree. All scratch files removed, all injected patches reverted and verified by re-running the suites green. git status reports "nothing to commit, working tree clean". /work/hypaware was not touched, nothing outside this worktree was created or deleted, and nothing was pushed, committed, or posted.

[philcunliffe]

neutral review - round 2 (final)

Head reviewed: f705417851c8fb4d4874823efa90a0617ab84163. MERGEABLE, all 9 checks
SUCCESS. Reviewed in a detached worktree; nothing was written to the branch.

One nit, nothing ship-blocking. Both re-measured numbers are correct as written,
and both claims in the new s3 comment hold.

The measurement work is the substance of this round, and it found a second trap
beyond the nested-copy one the fixer disclosed: an npm overrides block scoped to
icebird shadows the top-level rule, so overrides: { hyparquet: "1.28.1", icebird: { ... } } leaves icebird resolving 1.28.2 while root sits at 1.28.1. That
run looks like a floor drop and is not one. Forcing the nested copy requires naming
it inside the scoped block.

The payoff is a five-state table that separates what the previous rounds had
conflated. A true floor drop and the split-version state both report exactly
7 items, on the same 7 predicates, which is what made the artifact easy to miss - but
they are different failures: the true drop is 7 cache-versus-SQL divergences across
three failing tests, the split state is 7 cache-versus-parquet divergences across
one. Round 1 labelled its state honestly, so it stated no falsehood, but it offered
that measurement as evidence the floor is load-bearing, and two of its downstream
sentences about which tests fire are wrong as a result. None of that reached the
shipped artifact.

---

All measurements done, worktree clean. Here is the review record.

---

VERDICT: findings

Head reviewed: f705417851c8fb4d4874823efa90a0617ab84163. MERGEABLE, all 9 checks SUCCESS. Reviewed in a detached worktree with a fresh npm install; nothing was written to the branch, /work/hypaware was not touched, and every dependency experiment ran in a throwaway tree outside the scratchpad (/tmp/r4-exp-2f8a, since deleted).

One nit. Nothing is ship-blocking. Both re-measured numbers are correct as written, the bare-versus-guarded count settles at 8/1 and the body's wording genuinely does not depend on it, and both claims in the new s3 comment are true. The round-2 edit is exactly one test comment plus PR-body prose, and the stale neutral-triage marker is gone and has not returned.

---

1. nit (not ship-blocking) - test/plugins/s3-query-dataset.test.js:216-223

The new comment is true, but it now explains only one of the two rows it sits above, and the row it leaves unexplained is measurably inert. ['ts > CAST(300 AS BIGINT)', [5]] (line 222) fires under none of the three regression classes I injected: the old trio (6 of the 7 predicates go wrong, this one does not), a true hyparquet floor drop (1 failure, and it is the negated form), and cast-fold removal (0 failures). The comment says the pair "does not pin the fold" and then names only the negated form as the tripwire, so a maintainer following its logic would delete line 222 as decoration.

It is not decoration, and the reason is the mechanism the fixer traced but did not write down: hyparquet 1.28.1 evaluates $gt/$lte with plain JS relational comparison, so a null cell coerces to 0; 0 > 300 is false, which makes the positive form right by luck, and 0 <= 300 is true, which makes the negated form wrong. The positive row is the control that makes the asymmetry legible. I verified both compile to bare operators: {ts:{$gt:300n}} and {ts:{$lte:300n}}.

Why it matters: only at nit level. Nothing is wrong today; the risk is that the next maintainer deletes the control and the comment stops making sense.

Exact fix (optional): after "so this does not pin the fold ... 7/0)", add one clause, for example The negated form is the one case in this corpus that fails when the remote tier's hyparquet drops below 1.28.2: a null cell coerces to 0 there, so 0 <= 300 wrongly matches while 0 > 300 misses by luck, which is why both directions are here. While editing, 7/0 hardcodes the file's current test count and will read stale the moment an eighth test is added; still fully green would age better. Neither change is required to ship.

---

Round-1 nits, re-derived

1. "27 corpus divergences" for the pre-Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721 stack. Fixed, and 29 is the right number. I installed icebird@0.8.20 hyparquet@1.28.1 squirreling@0.15.2 in a clean tree with overrides forcing every subtree, verified a single hoisted hyparquet@1.28.1 and a single squirreling@0.15.2 with no nested copies, and instrumented the corpus loop to emit its wrong array. Result: 29, and 5 of the file's 6 tests fail (test 6, the pruning test, passes). The 29 split as 29 cache-versus-SQL, 0 cache-versus-parquet, which is the shape the body claims for a shared regression.

2. "guarded filter" credited for the CAST equality. Fixed. The fixer's 8/1 is right and round 1's "seven of nine" was wrong. I executed whereToParquetFilter over all nine shapes in the body's table: eight produce a bare operator ({neg:{$gt:-400n}}, {neg:{$lte:-400n}}, {neg:{$lt:-300n}}, {$and:[{neg:{$lte:-400n}},{neg:{$lte:-600n}}]}, {ts:{$gt:300n}}, {neg:{$gt:-400}} twice, {label:{$gt:'3'}}), and exactly one is guarded, neg != CAST(-300 AS BIGINT) giving {$and:[{neg:{$ne:null}},{neg:{$ne:-300n}}]}. The body's wording does not depend on the count and is correct at the source level: guardNull (icebird/src/sql/whereFilter.js:112-115) guards $ne and nothing else, and convertInValues (:279-303) guards $nin the same way. "Guarded only for !=/NOT IN" is a statement about the converter, not about this table, so it stays true for any corpus. Disclosing the discrepancy rather than smoothing it was the right call.

3. The s3 CAST comment. Fixed, and both of its claims hold. Claim one: I removed the node.type === 'cast' branch from staticLiteral so casts are declined instead of folded, and node --test test/plugins/s3-query-dataset.test.js is 7 pass / 0 fail. The same injection fails parity tests 3, 4 and 6 while leaving 1 and 2 green, so the fold is pinned in the parity file and genuinely not here. Claim two: under a true floor drop the s3 file has exactly one failure, WHERE NOT (ts > CAST(300 AS BIGINT)) -> [1,2,3,4], SQL says [1,3]. Round 1's proposed replacement framing was indeed wrong: both CAST rows compile to bare unguarded relational operators, so "the only bare unguarded relational operator in this corpus" would have been a second false sentence. The fixer catching that is the right outcome.

---

The numbers, re-measured

How I controlled for the nested-copy trap. At this head there is no icebird/node_modules/hyparquet at all: root hyparquet@1.28.2 satisfies icebird's exact pin, so it hoists. The tree does carry two other nested copies, hyparquet-writer/node_modules/hyparquet@1.28.1 and hypvector/node_modules/hyparquet@1.26.2, neither of which is on a read path under test. Before every run I enumerated all copies with find node_modules -maxdepth 4 -name hyparquet -type d and printed each package.json version, rather than trusting the install.

I also hit a second trap worth recording. overrides: { hyparquet: "1.28.1", icebird: { "hyparquet-writer": "0.16.6" } } does not reach icebird's subtree: the icebird-scoped block shadows the top-level rule, and icebird still resolved 1.28.2 while root sat at 1.28.1. Forcing the nested copy requires naming it inside the scoped block, overrides.icebird.hyparquet. That run looked like a floor drop and was not one.

state	icebird	root hyparquet	icebird's hyparquet	parity file	corpus divergences	s3 file
HEAD	0.8.22	1.28.2	1.28.2 (hoisted)	6/6 pass	0	7/7 pass
old trio, all copies	0.8.20	1.28.1	1.28.1 (hoisted)	5 of 6 fail	29, all cache-vs-SQL, 0 cross-backend	1 of 7 fails, 6 predicates wrong
true floor drop	0.8.22	1.28.1	1.28.1 (forced)	tests 1, 3, 5 fail	7, all cache-vs-SQL, 0 cross-backend	1 of 7 fails, 1 predicate wrong
split, root only	0.8.22	1.28.1	1.28.2 (nested)	test 1 fails	7, all cross-backend, 0 cache-vs-SQL	7/7 pass
split, nested only	0.8.22	1.28.2	1.28.1 (swapped in)	tests 1, 3, 5 fail	7	1 of 7 fails

The body's 29 is correct. Every one of the 29 is a cache-versus-SQL line, which is the point the paragraph is making: the expectations are hand-written Kleene truth, so a regression that moves both tiers together is still caught.

The true floor drop fires 7 cache-versus-SQL divergences, on neg > -400, neg >= -300, neg > CAST(-400 AS BIGINT), ts <= 300, NOT (ts > 300), NOT (ts > 300 OR ts > 400), NOT (neg < -400 OR neg < -600). It also fails tests 3 and 5, not just test 1.

Round 1's 7-divergence measurement was a split-version artifact, for the record. Round 1 labelled it honestly ("root hyparquet@1.28.1 while npm nests icebird's pinned 1.28.2"), so it did not state a falsehood, and the split state is a real hazard worth knowing about. But it is not the floor dropping, and it was offered as evidence that the floor is load-bearing for the CAST equality. The true drop produces a different failure shape entirely: cache-versus-SQL rather than cache-versus-parquet, three failing tests rather than one. The coincidence that both states report exactly 7 items, on the same 7 predicates mirrored between the two halves of the comparison, is what makes the artifact easy to miss. Two downstream round-1 sentences are wrong as a result: test 3 and test 5 do not fire under the split state round 1 measured (they pass), though they do fire under a true drop. None of this is in the shipped artifact, and the PR body makes no claim about a 7.

The s3 tripwire isolates to icebird's own copy, which is what "the remote tier's hyparquet" in the comment means. Swapping only icebird/node_modules/hyparquet down to 1.28.1 with root left at 1.28.2 reproduces the single failure; downgrading only root and leaving icebird nested at 1.28.2 leaves the s3 file 7/0. The s3 iceberg path never touches root hyparquet, so a root-only downgrade is a clean false negative there. That is exactly the trap the fixer disclosed, confirmed from both directions.

---

Also checked, clean

Diff scope. git diff --name-status origin/master HEAD is exactly two lines, A test/core/iceberg-source-parity.test.js and M test/plugins/s3-query-dataset.test.js. git diff --stat origin/master HEAD -- src/ hypaware-core/ llp/ bin/ docs/ package.json package-lock.json is empty, so nothing round 1 cleared about the deletion could have moved. The round-2 commit 6707ad5..f705417 touches one file, +6/-2, and the whole of it is the comment block at test/plugins/s3-query-dataset.test.js:216-221. No assertion, no case, no expectation changed, which is why I did not re-derive round 1's injection matrix from scratch.

PR body. No neutral- marker of any kind survives (grep -n 'neutral-' returns nothing), so the stale neutral-triage line is gone and has not returned. Zero U+2014. The two edited sentences read as round 1 asked: "29 wrong answers", and "pushes a bare bound (guarded only for !=/NOT IN; the rest are SQL-correct on null cells because of the hyparquet >= 1.28.2 floor, LLP 0222#hyparquet-floor)". The corpus really is 47 cases (counted: 15 NULL-literal, 13 relational, 6 negated-OR, 7 shapes, 6 CAST) and the file really has 6 tests. npm test and node --test counts reproduce exactly. One cosmetic residue: the body still says "round 2 of this PR" and "round 2's own review" in the CAST section, referring to the review rounds on the pre-reduction change, which now sits oddly beside a thread whose reduced-head review was numbered round 1. It is not a false statement and I am not making it a finding.

Conventions. No U+2014 in either changed file. No trailing semicolons. The new comment matches the file's idiom: lowercase continuation lines, wrapped under 80 columns, attached directly above the rows it describes with no intervening blank line, citing a LLP 0222#hyparquet-floor anchor that resolves to a real heading in llp/0222-one-pushdown-converter.decision.md:72. test/core/llp-ref-hygiene.test.js passes.

Ran, all at this head with a fresh npm install: npm test (4036 pass, 0 fail, 1 pre-existing skip, matching the body); npm run typecheck (clean); node --test test/core/iceberg-source-parity.test.js test/plugins/s3-query-dataset.test.js test/core/llp-ref-hygiene.test.js (24/24); node --test on the two changed files alone (13/13); npm run smoke -- local_parquet_export (ok); npm run smoke -- cache_lifecycle_maintenance (ok). Plus, in the throwaway tree: four full installs (old trio forced through every subtree, split root-only, split with a shadowed override, restore to real versions), two direct file-swap states to isolate the nested copy, one cast-fold removal injection, and one converter probe over the nine CAST shapes. package_bin_boot and walkthrough_picker_to_first_query not run (known red on master, #758 and #750).

Worktree. Real dependency versions confirmed restored before the final runs (root hyparquet@1.28.2, icebird@0.8.22, squirreling@0.15.3, no icebird/node_modules). All scratch removed. git status reports "nothing to commit, working tree clean". Nothing was pushed, committed, or posted, and nothing under the scratchpad outside this worktree was created, moved or deleted.

[philcunliffe]

neutral triage - ship

One residual nit, a comment in a test file on a tests-only PR. Preference. This PR
can merge. Deferred to #768.

The classification was close to foregone, so triage spent its effort on the question
the classification does not answer: is a PR that fixes nothing and adds 595 lines
of test worth shipping at all? It re-ran four regression classes itself rather than
taking the two rounds' injection matrix on faith.

• The Cache-path queries answer NULL-literal comparisons with IS NULL semantics #744 bug reinstated (removing if (value === null) return undefined from
  icebird's convertBinary): parity tests 1-4 fail, the corpus with 10 wrong answers,
  every one cache-versus-SQL. That is the proof the expectations are hand-written
  Kleene truth rather than a recording, so a regression moving both tiers together is
  still caught. The s3 round trip fails on 3 predicates too.
• A parquet-tier-only over-claim (appliedWhere = Boolean(hints.where)): the
  corpus fails entirely on its cross-backend half with the SQL half green. The half a
  SQL comparison structurally cannot provide earns its keep on its own.
• Cast folding removed: tests 3, 4 and 6 fail (6 with the verbatim
  cast-bounded scan opened 2 data files) while 1, 2 and the whole s3 file stay
  green, which simultaneously confirms the pruning and flag tests are the only guards
  on the fold and confirms the s3 comment's "does not pin the fold" claim.
• The WHERE gate dropped from position pushdown: tests 4 and 5 fail.

Tally: all 6 parity tests and the s3 round trip failed under at least one
injection. No test is decoration. The single inert element in 595 lines is the s3
positive-CAST row, and it is a deliberate control, which is exactly what #768 asks the
comment to admit.

The baseline argument holds up. These tests pin three dependencies this repo does not
own (icebird, hyparquet, squirreling) whose NULL semantics all moved this week,
at the one seam where a regression is silent: appliedWhere is final on the cache
path, so converter or floor drift produces wrong rows on ai_gateway_messages,
traces and logs with no engine re-filter and no error. The hyparquet floor is one
lockfile drift or nested-copy resurrection away, and triage measured the 1.28.1 versus
1.28.2 behavioural difference directly to confirm it.

Verified at head f705417: npm test 4036 pass / 0 fail / 1 pre-existing skip,
npm run typecheck clean, node --test on the two files plus llp-ref-hygiene 24/24,
smokes local_parquet_export and cache_lifecycle_maintenance ok. All injections
reverted, icebird force-reinstalled pristine after npm rewrote the pin to ^0.8.22,
and the battery re-run green.