Uh oh!
There was an error while loading. Please reload this page.
Fix LLP 0222 tier swap, add #734/decline regression cases (#756) - #763
Conversation
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.
philcunliffe
commented
Aug 14, 2026
neutral review - round 1Head reviewed: One nit. The doc correction is right, and the reviewer established that the hard Two things stand out:
It also confirms the author was right to leave line 12 alone, and notes #756 named Cleanup done, tree clean. Here is the review record. VERDICT: findings 1. nit — The restored case reads: // A value expression (concatenation), not a predicate.assert.equal(whereToParquetFilter(whereOf('SELECT * FROM t WHERE name || NULL')),undefined)The assertion is correct and the expected value is right, but the stated reason is not the branch that fires. In icebird's const{ column, value, flipped }=extractColumnAndValue(left,right)if(!column||value===undefined)returnundefinedif(value===null)returnundefined// fires for `name || NULL`constmongoOp=mapOperator(op,flipped,negate)if(!mongoOp)returnundefined// the "not a predicate" branch
The reason is a faithful carryover from the deleted test ( Why it matters: this is a boundary case whose whole purpose (per #756) is to prove the NULL-literal branch declines "for the right reason rather than by accident". As written it documents a reason the code does not exercise, which is the failure mode the case exists to prevent. Exact fix, either of:
// ...and a value expression declines on the operator even without a NULL.assert.equal(whereToParquetFilter(whereOf("SELECT * FROM t WHERE name || 'x'")),undefined)I verified that added assertion passes at this head. Nothing here blocks merge. The doc correction is right, and the Tier attribution, re-derivedI traced both converters from definition to caller without relying on the PR body, the issue, or the #721 review. The deleted converter, icebird's converter, Vocabulary check. "archive" is not defined in Independent cross-check from issue #744. Its title is "Cache-path queries answer NULL-literal comparisons with IS NULL semantics", its body says "the iceberg cache path (every intrinsic dataset...)" and names Verdict on every line of the doc that names a tier (all eight occurrences of "tier"):
End-to-end consistency, and the sentences that only made sense under the old labelling. The old doc was uniformly swapped and therefore internally self-consistent, which is why nothing read obviously backwards. There was one place where it broke out and contradicted the external record: old line 95 called #744 "the archive tier's NULL wrongness" while #744 is titled "Cache-path queries...". After the fix the doc is consistent in both directions, and one internal cross-check now closes that did not before: lines 39-42 say the NULL disagreements were fixed in "the kernel's copy" and "icebird retained" them, and line 47 says On the editorial carve-out. The claim holds, and for a stronger reason than the PR body gives. Because the old labelling was uniformly swapped, the doc's entire argument is invariant under exchanging the two names: every inference the doc draws is drawn between the two converters ( Also checked, clean
|
philcunliffe
commented
Aug 14, 2026
neutral review - round 2 (final)Head reviewed: VERDICT: clean. Round 1's finding is fixed and the reviewer re-derived every Cleanup done, worktree clean at VERDICT: clean No findings. Nothing is ship-blocking; nothing else is raised at any severity. Round-1 finding, re-derivedFixed. The comment now names the branch that actually fires, and the added case pins the branch it claims. I re-derived all three guard verdicts myself rather than taking the fixer's account. Method: copied
That matches the fixer's report exactly, and it matches round 1's diagnosis: The new comment's wording ( Does
On durability: the one way it could silently start testing something else is a change to Also checked, clean
|
Uh oh!
There was an error while loading. Please reload this page.
Summary
Closes the three remaining items in #756 (item 4, the PR #721 retitle, was already done). All four items trace back to PR #721 / LLP 0222, which is now merged (
8c08185).llp/0222-one-pushdown-converter.decision.mdattributedparquetDataSource(fed bysrc/core/query/parquet-source.js, the converter Delete our duplicate WHERE-pushdown converter, use icebird's (LLP 0222) #721 deleted) to the cache tier and icebird's converter to the archive tier — backwards. Confirmed by tracing callers:parquetDataSource's one non-test caller ishypaware-core/plugins-workspace/s3/src/query-dataset.js(the archive/S3 tier); the local intrinsic cache reaches icebird's converter viasrc/core/cache/storage.js→src/core/cache/iceberg/store.js(dataSourceForTable→icebergDataSource). Fixed the five load-bearing sentences at lines 19-20, 31, 92, and 95. Line 12 ("The cache tier and the archive tier convert predicates identically") names both tiers without attributing either to a specific converter, so it needed no change, same as the already-correct line 94. This is a mechanical editorial correction to an Accepted doc under CLAUDE.md's typo/broken-link carve-out — it doesn't change what the doc decided (one converter, owned by icebird).#734regression tripwire: added["NOT (label LIKE 'a%')", [3, 5]]to the always-UNKNOWN case list intest/core/parquet-source.test.js. Verified it passes now, and read throughicebird/src/sql/whereFilter.js(op === 'LIKE'always declines, negate or not) and squirreling'sapplyBinaryOp/evaluate.js(anull-operand comparison isnull/UNKNOWN, and unaryNOTkeepsnullasnullrather than JS!flipping it) to confirm the SQL-correct answer is[3, 5], not[2, 3, 4, 5].NOT (NULL = 1)(literal vs. literal, no column to key a filter on) andname || NULL(a value expression, not a predicate) to thedeclines NULL-literal comparisons to the enginetest, alongside the survivingid + NULLcase. Confirmed both still returnundefinedagainst the current (icebird, re-exported) converter.Test plan
npm install(fresh)npm test(4029 pass / 0 fail)npm run typechecknode --test test/core/parquet-source.test.jsnode --test test/core/llp-ref-hygiene.test.jsnpm run smoke -- local_parquet_exportnpm run smoke -- cache_lifecycle_maintenance🤖 Generated with Claude Code