Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) - #721
Conversation
`src/core/query/parquet-pushdown.js` and `icebird/src/sql/whereFilter.js` were two ports of the same Hyperparam original. icebird's kept moving and ours did not, and the drift cost query time and correctness. Typed literals never converted. squirreling parses `TIMESTAMP '2026-08-11T00:00:00Z'` as a `cast` wrapping a string literal; icebird constant-folds that, ours required a bare `literal` operand and returned undefined for the whole predicate, because AND is all-or-nothing. Every timestamp-bounded query therefore pushed nothing down to the cache tier. Measured on the production central server, org hyperparam: one grouped sessions-list scan took 11.4s bounded on `message_created_at` against 7.3s bounded on `date`, same rows, same projection. Any cast unwrapped at boolean position. `WHERE CAST(a = 1 AS TEXT)` pushed down as `a = 1`, but the engine evaluates that cast to the string 'false', which is truthy, so the pushdown dropped rows the query selects, and a converted filter sets appliedWhere so the engine does not re-filter to catch it. icebird gates the unwrap to truthiness-preserving casts. `coerceBigInt` is dropped rather than ported. `filterStrict: false` compares through `==`, so `5n == 5` either way, while hyparquet's bloom hashing rejects a bigint for INT32/FLOAT/DOUBLE: the coercion bought nothing on INT64 and disabled bloom pruning everywhere else. That relies on hyparquet >= 1.28.1, where `$in`/`$nin` match through `equals()` rather than `Array.prototype.includes`; this repo pins 1.28.1. A companion bump of hypaware-server's own 1.27.1 pin follows separately. icebird is already a direct dependency and deep `icebird/src/*.js` imports are the established pattern here, so this costs no new dependency and no type fidelity. The public `hypaware/core/query` surface is unchanged. Tests assert plain numbers where they asserted bigints, and cover the two shapes the drift produced: the folded TIMESTAMP literal (both as a unit and end to end through a real parquet scan, since a mis-folded literal would drop rows rather than merely lose pruning) and the truthiness-cast guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
philcunliffe
added
neutral:adopt
|
Review round 1 of Everything the PR asserts was verified rather than taken on trust: 1. blocker - newly pushed-down timestamp bounds return rows the query excludes, on nullable TIMESTAMP columnsicebird's Measured in the worktree on a three-row parquet file with This PR is what makes it reachable on the cache tier. At Not hypothetical for shipped datasets: eleven shipped Fix, verified against 2. minor - the two new end-to-end tests do not discriminateRe-running the new test file with Fix: assert 3. minor - the timestamp fixture is
|
philcunliffe
added
the
neutral:stuck
|
What neutral was doing. Round 1 review of this PR at head Why it cannot proceed. The review found a blocker that neutral cannot fix in this repo: adopting icebird's converter introduces a wrong-rows path on nullable TIMESTAMP columns, and the fix belongs upstream in icebird. In short: icebird's Neutral did not push anything. Two of the minor findings (the fixture is What it needs from you - a routing decision:
Two further upstream items worth filing alongside whichever you pick, neither introduced by this PR: Also pending on this PR, independent of the blocker: three open PRs (#716, #720, #721) each add a different How to unstick. Reply with a comment on this PR (or push to the branch). Neutral monitors this thread and will re-engage with your guidance on its next tick. |
|
Correction to the stuck report, from evidence found reviewing #724. The blocker stands, but its framing was incomplete in a way that changes which option you probably want. The NULL leak is not introduced by icebird. The identical hazard is already in the kernel's own Filed as #728. What this PR changes is only which predicates reach the leak: icebird's typed-literal fold makes it newly reachable for Why that matters for your decision: option 2 in the stuck report (scope it and merge anyway) now reads worse, since it would document a gap the repo already has unfixed elsewhere. Option 1 (fix upstream in icebird, bump the pin) reads better than it did, because a single null-guard on relational operators closes both the pre-existing kernel leak and this PR's newly-reachable case in one move - which is an argument for landing this PR with the upstream fix rather than narrowing or reverting it. The PR stays |
|
The null-guard now exists in-repo: PR #730 ( Two things from that work bear directly on the decision here: 1. More operators leak than either report listed. 2. The guard shape I suggested in the stuck report was the wrong one. I proposed The compact form (guard as a second key in the same condition object) is correct and 5x cheaper; the What this means for this PR. The same guard must move upstream into icebird's PR #730 touches |
|
Third correction to the porting guidance, from PR #730's review. Porting the null-guard upstream is not sufficient on its own - and applying it alone would make icebird strictly worse than it is today.
Note the middle column: these three were correct while unguarded, because a bare So a guard-only port converts three currently-correct queries into wrong-row queries. The upstream change needs all three:
De Morgan is also a pruning win independently: PR #730 has all three landed and reviewed, in |
|
Merge-order warning, from PR #730's triage. #730 (the in-repo null-guard fix) is now held and approved. Its triage checked icebird So this PR, as written, would reintroduce every NULL leak #730 fixes, plus the The good news is that merge order is protective, not a trap. Two honest exits when you unstick this:
This PR stays |
|
Fourth correction to the porting guidance, and this one is material to whether the PR should proceed at all. PR #743's triage probed the pinned icebird converter directly and found it is worse than every earlier note here assumed. I previously told you icebird needs three things ported (the Icebird does not decline NULL comparisons.
and So the full port list is now four items, three of which this repo has already fixed in its own copy:
Adopting icebird's converter as-written would regress all four. The cache path's independent exposure is filed as #744, since it exists on master today regardless of what happens to this PR. Whoever unsticks this should read that first: it characterizes the gap between the two converters precisely, and it changes the arithmetic of option 1 (port upstream, then bump) versus option 2 (keep the local converter and shrink this PR's scope) in favour of the latter, unless someone is prepared to land four separate fixes upstream first. This PR stays |
…al fold icebird's converter still disagrees with SQL on NULLs (11 of 24 predicates in a three-valued battery), so adopting it wholesale would undo #730 and #743. Take the one thing it had that we lacked instead: constant-folding of typed literals, plus the truthiness gate on boolean-position casts. Folded bounds carry the same $ne: null guard plain bounds do. Renumber the LLP to 0219; 0212 is taken.
squirreling 0.15.3 gives WHERE Kleene NULL semantics and icebird 0.8.22 pushes filters that agree with it, so the kernel converter is no longer ahead on correctness. Delete it for the re-export, bump hyparquet to 1.28.2 (bare relational bounds reject null cells there), and move the shape assertions to icebird's shapes. Row-set and appliedWhere tests are unchanged and stay green either way.
platypii
removed
the
neutral:stuck
unstuck
The question the stuck report was waiting on is answered. It asked for a
One thing to sequence before mergingPR #751 (fix for #744, triaged ship, held for merge) added That is not the failure the #751 review warned about - it warned that #721 landing
Neither ordering looks unsafe, but the second one changes a test's meaning, so it |
neutral review - round 1Head reviewed: Correction to my unstick note above, before the record. I wrote that master is The rest of the unstick note holds: The headline is finding 1, and it needs a human decision before either this PR or #751 is currently held for a human to merge. I am cross-posting this there. All checks complete, worktree clean. Here is the review record. VERDICT: findings — 1 major (cross-PR sequencing), 3 minor, 1 nit. No blocker. The converter swap itself is correct, and I could not make it return a wrong row on any path I traced. Correction to the briefing I was given: master is on Findings1. major —
|
| predicate | rows | bytes read |
|---|---|---|
ts = NULL (declined) |
0 | 4440 |
ts != NULL (declined) |
0 | 4440 |
NOT (ts = NULL) (declined) |
0 | 4440 |
ts NOT IN (1, NULL) ($in: []) |
0 | 0 |
ts IN (NULL) ($in: []) |
0 | 0 |
| no filter | 500 | 2323 |
So the decline does cost a full read of the predicate columns where the never-match cost nothing. But it applies to exactly one family — a literal NULL written into a WHERE — which is a degenerate, near-always-a-bug predicate that returns zero rows by construction. Every non-degenerate never-match survives: NOT IN (…, NULL) and IN (NULL) still fold to {$in: []} and still prune every row group on statistics alone. I judge the trade fine and would not hold the PR for it.
Dropping the $ne: null guards costs no pruning either. This was the worry the earlier neutral notes raised (compact guard vs $and vs bare). On hyparquet 1.28.2 all three shapes are byte-identical:
bare {ts:{$lte:49}} (icebird) rows=42 bytes=438
compact {ts:{$ne:null,$lte:49}} (kernel) rows=42 bytes=438
$and [{$ne:null},{$lte:49}] rows=42 bytes=438
and for $nin, where icebird uses $and and the kernel used the compact form, also identical (both 1953 bytes, both 300 correct rows; bare leaks to 350). The compact-shape argument was specific to 1.28.1's null-coercing comparators and is retired by the floor.
Corpus diff, case by case
Headline: the behavioural corpus was not weakened at all. I extracted every [predicate, expected] tuple from the old file (c483c1a) and the new one and diffed them sorted:
old=66 new=66
=== removed from corpus === (empty)
=== added to corpus === (empty)
All 66 end-to-end cases across issue #728, comparison against a NULL literal, issue #734, and predicates that are not always-UNKNOWN are byte-identical, expectations included. Not one expected row set was edited to match new behaviour. That is the strongest possible answer to "was the corpus bent to fit". Everything that changed is a shape assertion at the unit level, and the row-level truth those shapes must produce is pinned independently and unchanged.
Unit assertions whose expectation changed (7 tests, all shape-only, all justified and each independently confirmed by the unchanged row corpus):
| assertion | old | new | verdict |
|---|---|---|---|
id = 3 |
{$eq: 3n} |
{$eq: 3} |
ok — coerceBigInt dropped; filterStrict:false routes $eq/$in/$nin through equals(), and I confirmed hashParquetValue (hyparquet/src/bloom.js:118) requires typeof value === 'number' for FLOAT/DOUBLE/INT32 and accepts either for INT64, so this is a bloom-pruning gain |
id > 3, id <= 3, 3 < id, 3 >= id |
{$ne:null, $gt:3n} |
{$gt: 3} |
ok — guard moved into hyparquet 1.28.2; verified by the filter.js diff and by the pruning measurements above |
NOT (id = 1) |
$and + $ne guard |
same, number literal | ok — $ne is the one operator that keeps its guard |
NOT (id = 1 OR id = 2) |
De Morgan $and |
same | ok — icebird 0.8.22 carries De Morgan with the reasoning intact; 0.8.21 still had $nor at line 75, which I confirmed from the tarball |
id NOT IN (1,2) |
{$ne:null, $nin:[1n,2n]} |
{$and:[{$ne:null},{$nin:[1,2]}]} |
ok — measured identical pruning |
id IN (1, NULL) |
{$ne:null, $in:[1n]} |
{$in: [1]} |
ok — matchesIn → equals(null, 1, false) is false, so the guard was redundant |
id = NULL family (8 cases) |
{$in: []} |
undefined |
ok — the strategy change, verified equivalent above |
Unit assertions dropped. Six, from the deleted pushes never-match only where the shape proves it and the retired handles predicates whose SQL result is always UNKNOWN:
NOT NOT (id = NULL)— acceptable:NOT NOT (ts = NULL)survives in the row corpus (:379).name LIKE NULL— acceptable: survives at:384.NOT (name LIKE NULL)— acceptable: survives at:385.NOT (id = 3)→ guarded$ne— acceptable:NOT (id = 1)inhandles AND / OR / NOTis the same shape.id = NULL OR id = 3— acceptable: moved into the decline test with the correct new expectation (undefined, since the declined conjunct collapses the whole tree), and the row-level['ts = NULL OR ts = 300', [3]]is unchanged.NOT (NULL = 1)andname || NULL— finding 4 (nit): no replacement at either level. Verified correct by probe.NOT (name LIKE 'a%')— finding 3 (minor): no replacement at either level, and it is the shape LLP 0222 names.
Tests added (5), all strengthening:
folds typed literals (TIMESTAMP casts)— 3 assertions includingTIMESTAMP 'not-a-day'→undefined, which pinsfoldCast's fail-safe.only unwraps truthiness-preserving casts— pins theCAST(a = 1 AS TEXT)gate that is the other correctness fix icebird brings.timestamp day bounds filter correctly through the pushed-down scan— end to end, over a nullable TIMESTAMP fixture with a NULL row, expecting[2,3]. This directly discharges minor findings 1 and 3 from the previous review round.a folded timestamp bound is actually pushed down—assert.equal(scan.appliedWhere, true). This discharges the previous round's finding 2 (the "does not discriminate" one) exactly as it asked.a timestamp bound matching no rows returns none (and one matching all returns all)—4, not 5on the all-matching bound, so the NULL row's exclusion is asserted at both extremes.
The TIMESTAMP_COLUMNS fixture is now { name: 'at', type: 'TIMESTAMP', nullable: true } with a NULL row, with the comment naming the eleven shipped nullable-TIMESTAMP ColumnSpecs. That was the previous round's minor finding 3 and it is properly fixed.
Also checked, clean
- The 23 surviving lines of
parquet-pushdown.js. A pure re-export:export { whereToParquetFilter } from 'icebird/src/sql/whereFilter.js', no re-wrap, no argument or return massaging.build:typesemits a clean one-line re-export totypes/core/query/parquet-pushdown.d.ts.src/core/query/index.js:9still re-exports it, so thehypaware/core/querypublic surface is unchanged. icebird'sexportsmap publishes"./src/*.js"with"types": "./types/*.d.ts",filesincludestypes, andtypes/sql/whereFilter.d.tsis a real signature (ExprNode → ParquetQueryFilter | undefined), notany. The deep-import route is the same one ~20 existing cache modules already take. @refhygiene. The two@ref LLP 0098 [constrained-by]annotations died with the code they annotated; nothing dangles. Grepped the whole tree for the deleted helpers (coerceBigInt,guardNulls,convertInValues,extractColumnAndValue,mapOperator) — the only surviving mentions are the module header comment and LLP 0222's#no-bigint-coercionsection, both deliberate. The new@ref LLP 0222 [implements]is attached with no blank line to the export, carries no anchor (so nothing to resolve), and the LLP exists. The four other@ref LLP 0098sites in the repo all point at wrapper duties untouched by this PR and still hold.test/core/llp-ref-hygiene.test.js— which validates anchor resolution, duplicate LLP numbers, and em dashes inside annotations — is green.- LLP 0222 number, and its claims. 0222 is free on
origin/masterand across all 47 remote branches; I swept every branch'sllp/tree for021[9]/022[0-9]and found0219(master),0220,0221(Pin cross-backend query parity and icebird 0.8.22's pushdown contract (tests only) #751),0222(this),0223, each claimed once. Header shape matches house style. Verified against the tree: the#hyparquet-floorclaim (confirmed by the 1.28.1→1.28.2filter.jsdiff, which is literally four lines addingvalue !== null && value !== undefined &&to$gt/$gte/$lt/$lte);#no-bigint-coercion's three sub-claims (matchesIn→equalsin 1.28.1 and 1.28.2, bloom hashing type gates inbloom.js:118-160); "the same exact pin, resolving to a single deduped copy" (one top-levelhyparquet@1.28.2, matching icebird 0.8.22's own exact"hyparquet": "1.28.2"; the only other copy ishypvector/node_modules/hyparquet@1.26.2, which never sees this filter); every §Decision step-2 semantic (De Morgan, bare flipped operators,$ne/$ninguards,NOT INwith NULL →{$in: []}, NULL members dropped from a plainIN, NULL literals declined) read out of the installed 0.8.22 source; and §consequences' three guardrail claims (row sets end to end,appliedWhereasserted, bytes measured) all present in the test file. The "icebird@0.8.21 wrong on 11 of 24" claim I could not reproduce exactly, but I confirmed 0.8.21 is the pre-fix shape ($norat line 75,guardNullwritten against a two-valued engine), which makes it credible. The production 11.4s/7.3s measurement is unverifiable from here. - LLP 0098 is not rewritten. Zero diff. 0222 cites it as
Relatedand correctly does not claim to extend it — 0098's one mention ofwhereToParquetFilteralready describes icebird's converter insidescanColumn, and 0098's "an unconvertible predicate … leavesappliedWhere: falseand the engine re-filters" is exactly the contract the declining strategy leans on, now backed by a genuinely three-valued engine. No forward-ref needed. - The dependency bump is surgical, and fully accounted for.
diff -rqof each package'ssrc/tree against the previously pinned version: icebird 0.8.20→0.8.22 changes one file (src/sql/whereFilter.js); hyparquet 1.28.1→1.28.2 changes one (src/filter.js, the four-line null guard shown above); squirreling 0.15.2→0.15.3 changes two (expression/binary.js,expression/evaluate.js, the Kleene conversion). Nothing else moved in any of the three. The only behaviour change beyond the NULL work is squirreling's 3VL reaching non-WHERE positions — a projected comparison (SELECT a < b) now yieldsnullinstead offalse, andHAVING/CASE/JOIN ONseenullwhere they sawfalse(falsy either way, so no row-set change). I grepped every SQL string insrc/andsrc/core/query/overview.js: no shipped query projects a comparison, so this is unobservable in the product today. Worth knowing; not worth a finding. - Conventions. No U+2014 anywhere in the four changed files (checked with a real codepoint scan, not a shell glob). No semicolons in the changed JS. No
@typedef, no inlineimport('...')types, no.d.tstype-import specifiers. The two@importlines in the test file are root-anchored.jspaths as required. - What I actually ran, on a fresh
npm installin the worktree ataea733b3:npm test(4029 pass / 0 fail / 1 skipped),npm run typecheck(clean),npm run build:types(clean),node --test test/core/parquet-source.test.js(28/28),npm run smoke -- local_parquet_export(ok),npm run smoke -- cache_lifecycle_maintenance(ok), plus four purpose-built probes (50-predicate 3VL battery across row/aggregate/union paths; 42-case cache-tier parity throughdataSourceForTable; byte-counting pruning measurements for decline-vs-never-match and for all three guard shapes; and a direct conversion probe for the Pin cross-backend query parity and icebird 0.8.22's pushdown contract (tests only) #751-relevant predicates). Scratch files removed;git statusclean.
philcunliffe
changed the title
neutral triage - shipReview budget exhausted with findings open. Triage judged each residual a true A human asked for this PR to be unstuck and moved forward, so re-sticking it needed Re-verified at head The one finding that needed real thought: sequencing with #751Triage established the worst case concretely rather than reasoning about it in the
Correctness survives every path in the merged state: when icebird's converter Recommended order, verified: land #721 first, then close #751 as obsoleted. Its One residual fixed here rather than deferredThe title said The other threeAll on #756: LLP 0222 has cache tier and archive tier swapped at five lines |
…LLP 0222) PR #721 bumped 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 replaced this repo's WHERE converter with icebird's. The NULL work converged upstream, so issue #744 is closed by that bump and `withSqlCorrectWhere` has no bug left to work around: it would only re-apply icebird's own filter to rows that already passed it, which is the per-row materialization LLP 0098 exists to prevent. Deleted: `src/core/query/iceberg-source.js`, its export in `src/core/query/index.js`, the `store.js` and s3 `query-dataset.js` wiring, LLP 0221, and the LLP 0098 forward-ref. LLP 0222 records where converter ownership now lives. Kept, as the durable value: the cross-backend parity corpus, rebuilt through `dataSourceForTable` directly. Every expected row set is SQL's three-valued answer written down by hand, so the suite fails on a shared regression as well as a divergent one. Against the pre-#721 stack (icebird 0.8.20, hyparquet 1.28.1, squirreling 0.15.2) five of its six tests fail. The wrapper-specific tests changed meaning rather than dying: `appliedWhere` honesty and LIMIT/OFFSET now pin icebird 0.8.22's own contract, which is what LLP 0222 makes the whole stack depend on, asserted shape by shape against the parquet tier so a converter change in a dependency cannot diverge silently. The CAST subset chain tightened to full equality, measured rather than assumed. `neg > CAST(-400 AS BIGINT)`, `NOT (neg > CAST(-400 AS BIGINT))`, `NOT (neg >= CAST(-300 AS BIGINT))` and `NOT (neg > CAST(-400 AS BIGINT) OR neg > CAST(-600 AS BIGINT))` all give SQL == cache == parquet, so the `bounded` option, the `isSubset` helper and the subset branch are gone rather than left as an assertion that cannot fail. Co-Authored-By: Claude <noreply@anthropic.com>
…g, s3 comment - Body: pre-#721 corpus divergence is 29, not 27 (round 1's number for the 41-case corpus, carried forward after it grew to 47 with the CAST section) - Body: the CAST subset chain 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, not because of a guard - test/plugins/s3-query-dataset.test.js: the two CAST cases pin row correctness and the hyparquet-floor tripwire on the remote tier, not the cast fold (verified: removing the fold still passes 7/0) Co-Authored-By: Claude <noreply@anthropic.com>
… (tests only) (#751) * Cache-path queries answer NULL-literal comparisons with IS NULL semantics (#744) Wrap every icebergDataSource so the rows it yields are judged by this repo's WHERE converter instead of icebird's, which converts a NULL-literal comparison to IS NULL semantics, pushes unguarded inequalities, and complements a negated OR two-valued - all three claimed as appliedWhere, so the engine never re-filtered and the wrong answer was final. The predicate is still forwarded to icebird as a pruning-only hint (its filter is always a superset of SQL's answer), so no file or row-group pruning is lost. Co-Authored-By: Claude <noreply@anthropic.com> * Scope LLP 0221's parity claim to owned predicates, cover CAST and the s3 iceberg branch (#744) Review round 1 on PR #751 found the Consequences section overclaiming cross-backend parity unconditionally: for a predicate the kernel converter declines (a CAST/typed-literal operand), icebird's own converter still folds it, so the cache path can return fewer rows than the parquet path (safe direction, not the equality the doc claimed). Scope bullet 1 to predicates the kernel converter owns and rewrite bullet 3 to state the declined-predicate relationship is bounded SQL ⊆ cache ⊆ parquet, backed by a new CAST case in iceberg-source-parity.test.js that asserts the subset chain instead of strict equality. Same fix applied to the PR body's own overreaching #734 bullet. Also closes the s3 iceberg branch's test coverage gap: query-dataset.js wraps its icebergDataSource in withSqlCorrectWhere the same way the local cache does, but had no test proving the remote branch is NULL-correct. Added a round-trip test that writes a real table through a real BlobStore and exercises the same NULL-literal predicates. Co-Authored-By: Claude <noreply@anthropic.com> * Drop the cache WHERE wrapper #721 obsoleted, keep its parity corpus (LLP 0222) PR #721 bumped 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 replaced this repo's WHERE converter with icebird's. The NULL work converged upstream, so issue #744 is closed by that bump and `withSqlCorrectWhere` has no bug left to work around: it would only re-apply icebird's own filter to rows that already passed it, which is the per-row materialization LLP 0098 exists to prevent. Deleted: `src/core/query/iceberg-source.js`, its export in `src/core/query/index.js`, the `store.js` and s3 `query-dataset.js` wiring, LLP 0221, and the LLP 0098 forward-ref. LLP 0222 records where converter ownership now lives. Kept, as the durable value: the cross-backend parity corpus, rebuilt through `dataSourceForTable` directly. Every expected row set is SQL's three-valued answer written down by hand, so the suite fails on a shared regression as well as a divergent one. Against the pre-#721 stack (icebird 0.8.20, hyparquet 1.28.1, squirreling 0.15.2) five of its six tests fail. The wrapper-specific tests changed meaning rather than dying: `appliedWhere` honesty and LIMIT/OFFSET now pin icebird 0.8.22's own contract, which is what LLP 0222 makes the whole stack depend on, asserted shape by shape against the parquet tier so a converter change in a dependency cannot diverge silently. The CAST subset chain tightened to full equality, measured rather than assumed. `neg > CAST(-400 AS BIGINT)`, `NOT (neg > CAST(-400 AS BIGINT))`, `NOT (neg >= CAST(-300 AS BIGINT))` and `NOT (neg > CAST(-400 AS BIGINT) OR neg > CAST(-600 AS BIGINT))` all give SQL == cache == parquet, so the `bounded` option, the `isSubset` helper and the subset branch are gone rather than left as an assertion that cannot fail. Co-Authored-By: Claude <noreply@anthropic.com> * Fix PR #751 review nits: re-measured corpus count, guard-split wording, s3 comment - Body: pre-#721 corpus divergence is 29, not 27 (round 1's number for the 41-case corpus, carried forward after it grew to 47 with the CAST section) - Body: the CAST subset chain 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, not because of a guard - test/plugins/s3-query-dataset.test.js: the two CAST cases pin row correctness and the hyparquet-floor tripwire on the remote tier, not the cast fold (verified: removing the fold still passes 7/0) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: test <test@example.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: test <test@test.com>
* Fix LLP 0222 tier swap, add #734/decline regression cases (#756) Three deferred triage findings from PR #721 (LLP 0222): - llp/0222-one-pushdown-converter.decision.md had "cache tier" and "archive tier" swapped at the lines that name which converter fed which tier. parquetDataSource (parquet-source.js, the deleted kernel converter) has its one non-test caller in the S3/archive plugin; the local intrinsic cache reaches icebird's converter via cache/iceberg/store.js. Fixed the five load-bearing sentences this propagated into; left the symmetric "cache tier and archive tier convert predicates identically" sentence as is. - Added the bare `NOT (label LIKE 'a%')` case LLP 0222 names as the #734 closure to the always-UNKNOWN regression list. - Restored the `NOT (NULL = 1)` and `name || NULL` decline boundary cases dropped with the old unit test. * Fix WHERE-decline comment to name the guard that actually fires (#756) --------- Co-authored-by: test <test@test.com>
2 participants
What
src/core/query/parquet-pushdown.jsbecomes a re-export ofwhereToParquetFilterfromicebird/src/sql/whereFilter.js. -188 / +33 lines. The publichypaware/core/querysurface is unchanged, so no consumer moves.Rationale doc: LLP 0212 (extends LLP 0098, which settled that the predicate gets pushed down; this settles whose converter pushes it).
Why
The two files were ports of the same Hyperparam original (
lib/tools/parquetPushdownFilter.ts) - same function names, same structure, same De Morgan comments. icebird's copy kept moving; ours did not. Three consequences:1. Typed literals never converted. squirreling parses
TIMESTAMP '2026-08-11T00:00:00Z'as acastnode wrapping a string literal. icebird constant-folds it (staticLiteral/foldCast); ours required a bareliteraloperand, and because AND is all-or-nothing the whole predicate collapsed toundefined. hypscope's sessions surface bounds every day window this way, so it pushed nothing down to the cache tier.Measured on the production central server, org
hyperparam, 2026-08-12 - one grouped sessions-list scan, identical projection and rows:message_created_at >= TIMESTAMP '2026-08-11T00:00:00Z'date >= '2026-08-11'2. A correctness bug. Our
convertExprunwrapped any cast at boolean position, soWHERE CAST(a = 1 AS TEXT)pushed down asa = 1. The engine evaluates that cast to the string'false', which is truthy, so the pushdown dropped rows the query selects - and a converted filter setsappliedWhere, so the engine does not re-filter to catch it. icebird gates the unwrap to truthiness-preserving casts.3. Bloom pruning was off for non-INT64 numerics.
coerceBigIntis dropped rather than ported.filterStrict: false(whichparquet-source.jsand icebird both pass) compares through==, so5n == 5either way, while hyparquet'shashParquetValuerejects a bigint for INT32/FLOAT/DOUBLE and returnsundefined. The coercion bought nothing on INT64 and silently disabled bloom pruning everywhere else.Dependency floor
Dropping
coerceBigIntrelies on hyparquet >= 1.28.1, where$in/$ninmatch throughmatchesIn->equals(value, target, strict)rather thanArray.prototype.includes. On 1.27.x a number-valued$inagainst an INT64 column (decoded as bigint) matches no rows. This repo pins 1.28.1; a companion one-line bump of hypaware-server's own1.27.1pin follows separately (that pin also governs icebird's archive reads, which have the latent bug today independent of this change).icebird is already a direct dependency, and deep
icebird/src/*.jsimports are the established pattern here (src/core/cache/retention.js,src/core/cache/iceberg/stream_append.js, a dozen sites). Its exports map publishes./src/*.jswith matchingtypes/, so no type fidelity is lost.Tests
Existing assertions move from bigints to plain numbers. New coverage for the two shapes the drift produced:
TIMESTAMPliteral, both as a unit assertion and end to end through a real parquet scan with rows either side of the day boundary - a mis-folded literal would drop rows rather than merely lose pruning, so shape assertions alone are not enoughCAST(... AS INT)converts,CAST(... AS TEXT)does not)Verification
npm run typecheckclean.npm test: 3943/3946 pass. The one failure istest/core/leave-command.test.js:253(central-layer teardown), confirmed failing identically atmasterwith these changes stashed - pre-existing and unrelated.file:../hypaware): typecheck clean, all 65 suites pass.Not measured: the production speedup, which needs a deploy. The 11.4s vs 7.3s gap above is the expected magnitude, not an observed result of this patch.
Out of scope
This is one of four causes behind the slow sessions list. The dominant one is hypscope's per-batch
sessions.titlesquery (61.6s measured, already withdrawn on hypscope master); the others are batch 0 never using the summaries rollup, and the absence of partition-level pruning (sql.jscallsdiscoverPartitionswith no WHERE, so every raw query still opens all 752 cache files before any filter runs).🤖 Generated with Claude Code