Skip to content

Follow-up: deferred triage findings from PR #721 (LLP 0222) #756

Description

@philcunliffe

PR #721 was triaged ship at head aea733b341f8af7f712f322a7573bf3b84014286.
Four residuals were deferred; none affects runtime correctness at that head, and all
were verified by measurement during triage. Each is independently landable.

Backlink: PR #721, its round-1 review, LLP 0222.

1. Swap "cache tier" and "archive tier" in LLP 0222

llp/0222-one-pushdown-converter.decision.md lines 12, 19, 31, 92 and 95 attribute
the two converters to the wrong tiers.

Ground truth: parquetDataSource, fed by the converter this PR deleted via
src/core/query/parquet-source.js, has exactly one non-test caller,
hypaware-core/plugins-workspace/s3/src/query-dataset.js:90, which is the
archive/S3 tier. The local intrinsic cache reaches icebird's converter via
src/core/cache/storage.js to src/core/cache/iceberg/store.js:557
(dataSourceForTable to icebergDataSource). Issue #744's own title and body name
the cache path.

The inversion propagates into load-bearing sentences: the motivating
11.4s-versus-7.3s measurement is attributed to the tier that already had the
typed-literal fold, and line 95 calls #744 "the archive tier's NULL wrongness".

Fix, mechanical and permitted on an Accepted doc under CLAUDE.md's editorial
carve-out: line 19 becomes "(the archive tier, via parquet-source.js)" and
"(the cache tier, via icebergDataSource)"; line 31 "scanned the archive tier
unpruned"; line 92 "Timestamp-bounded predicates prune the archive tier"; line 95
"Issue #744 (the cache tier's NULL wrongness)". Line 94 is symmetric and needs no
change.

2. Assert the bare NOT (col LIKE 'a%') shape LLP 0222 names as the #734 closure

Add one line to the case list in
test('predicates that are not always-UNKNOWN keep their ordinary handling (issue #734)')
at test/core/parquet-source.test.js:412-421, after the
"NOT (label LIKE 'a%') AND ts IS NOT NULL" case:

["NOT (label LIKE 'a%')", [3, 5]],

Verified passing at the PR head. Without it, the largest user-visible behaviour
change after the timestamp fold has no regression tripwire: master returns
[2,3,4,5], the three-valued stack returns the SQL-correct [3,5]. The existing
conjunct case cannot discriminate, because its IS NOT NULL masks the leaked NULL
rows, and the 3VL engine is otherwise gated only via the declined-NULL-literal
route, not the declined-LIKE route the LLP names.

3. Restore two decline boundary cases

NOT (NULL = 1) returning undefined (literal versus literal, no column to key on)
and name || NULL returning undefined (a value expression, not a predicate) were
dropped with the old unit test and have no replacement at either level. Both proved
the NULL-literal branch declines for the right reason rather than by accident, and
both were verified still correct during review.

Add them to the declines NULL-literal comparisons to the engine test at
test/core/parquet-source.test.js:250-266, alongside the surviving id + NULL case.

4. Retitle PR #721 before merge

The title says (LLP 0212); the doc is LLP 0222. On master, 0212 is
session-opt-out-is-a-cli-verb.decision.md, a live unrelated Accepted doc, so
without the retitle the merge commit cites the wrong design record. Commit 0c6af6d
renumbered every in-tree reference correctly; only the title and body lagged.

Done: neutral retitled the PR when this issue was filed. Left here for the
record.

Metadata

Metadata

Assignees

No one assigned

    Labels

    neutral:fixDelegate this issue to neutral for an autonomous fix attempt (reproduce -> fix -> PR)

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions