Pin the icebird absent-column contract at the SQL surface (#778) - #787
Pin the icebird absent-column contract at the SQL surface (#778)#787philcunliffe wants to merge 5 commits into
Conversation
A read of a column a partition physically lacks was documented nowhere for the icebird-backed cache, and five successive written descriptions of it were measured false during PR #740's review. This measures it instead, at the pair `hyp query sql` actually runs (executeSql + collect) over a staged icebird cache, and records only what the runs showed. The contract turns out to be path-dependent and not single-valued: nothing throws, a scan whose hint set is exactly one column reads null (the scanColumn fast path, which withSchemaColumns null-normalizes), and anything widening it to two reads undefined. A literal sibling does not widen it; a WHERE on an unrelated column does, so the same projection flips value when a date filter is added. Both are live readings of the same absent cell. Measuring it exposed a correctness hole. LLP 0098#wrapper-duties already requires withSchemaColumns to strip a predicate naming a declared-but-absent column, but only scanColumn implemented it; scan forwarded it verbatim. icebird converts it to a hyparquet filter over a column its schema never had, filters nothing, and still reports appliedWhere: true, so the engine trusts the unfiltered stream: on a single-partition cache, WHERE git_remote = 'zzz' returned every row. The union's own gate hid it, so only a fresh install with one client was affected. The row path now applies the same gate. LLP 0240 records the measured table and the gate. LLP 0015 and LLP 0098 are Accepted/Active and gain only forward-refs. Co-Authored-By: Claude <noreply@anthropic.com>
Review follow-up: the Consequences bullet records an observed star-expansion defect and says it is left unaddressed, but pointed at no tracking issue. #788 was filed for it; cite it so a reader of the doc can find where it went. Editorial link only; nothing the doc settled changes. Co-Authored-By: Claude <noreply@anthropic.com>
Verdict: cleanReviewed manually in a dedicated worktree at The scepticism this PR asks for is warranted, so I did not read the contract 1. Re-measured the contract independentlyOwn probe (three rows in the narrow partition, two in the wide one, so the row
Nothing threw: I also verified the two mechanism claims the doc rests on rather than taking
My observation agrees with LLP 0240 everywhere. No discrepancy found. 2. The
|
|
Round 2 of 2 - delta review. Clean. Round 1's clearance stands. Round 1 reviewed The delta. The reference is accurate. Issue #788 exists, is OPEN, and carries exactly one label, Hygiene.
CI. Green at Not repeated, by design. A one-sentence doc addition cannot move the measured contract, the
Nothing needs a human. |
Both sides edit `withSchemaColumns.scan`. Keep both duties: this branch's predicate gate (LLP 0240#where-gate) and master's row alignment (LLP 0241#alignment). The gate runs first and decides which options reach the source; the alignment wraps whatever stream comes back. `scanColumns` is read from `options.columns` before the strip, which only drops `where` (plus the limit/offset that are meaningful only after it), so the two do not fight over the advertised list. `appliedWhere` / `appliedLimitOffset` are `pushable && inner.*`, which forwards the source's flags when the predicate is pushed and reports false when it was stripped, matching the `scanColumn` hook right below. The alignment did move one thing this branch had measured. Under `SELECT *` the absent column's key now exists on the row and holds `undefined`, where before it was not on the row at all. The rendering is byte-identical and no other row of LLP 0240's table moves. Confirmed by reverting only master's two alignment call sites, which flips it back. LLP 0240 gains a forward-ref recording the amendment, and the star test now pins the post-alignment shape instead of the pre-alignment one. The two behaviours also turn out to be load-bearing on each other, which neither PR had pinned. A star carries no `columns` hint, so its rows are only as wide as the partition physically is. The gate hands the predicate back to the engine, which reads the absent column off `row.cells`; with master's alignment reverted, `SELECT * FROM t WHERE git_remote IS NULL` raises `ColumnNotFoundError` from `filterRows` instead of answering. A new test pins that composition, and it fails if either side is reverted. LLP 0240 and 0241 do not collide: 0240 exists only on this branch, 0241 only on master, and no other branch or tombstone claims either number.
Re-triage after the #789 merge: clean, held for human as beforeHead The merged
|
# Conflicts: # hypaware-core/plugins-workspace/ai-gateway/src/dataset.js # hypaware-core/plugins-workspace/ai-gateway/src/message_projector.js
Triage at head
|
Documents the icebird-backed absent-column contract deferred from PR #740, and lands
executeSql+collectSQL-surface tests that pin it.Nothing here was described from reading the code. Every claim below is a run I made and its output. Where a prior description (including the issue's own) turned out to be wrong, that is called out.
What the contract actually is
Over an icebird-backed partition that physically lacks a declared column (the normal state after the v7 additive bump, LLP 0032), nothing throws, and the value read is path-dependent:
SELECT git_remote FROM tnull{"git_remote":null}SELECT git_remote AS gr FROM tnull{"gr":null}SELECT git_remote, 1 AS n FROM tnull{"git_remote":null,"n":1}SELECT id, git_remote FROM tundefined{"id":1}(key dropped)SELECT git_remote FROM t WHERE date >= '2026-01-01'undefined{}SELECT * FROM t{"id":1,"date":"2026-05-26"}The discriminator is the size of the scan's hint column set, not the shape of the SELECT list. Squirreling routes a one-hint-column scan through
scanColumn(execute.js, gated onplan.hints.columns?.length === 1, no aggregate required) andwithSchemaColumnsnull-normalizes the hole there. Two hint columns take the row path and readundefined.nullandundefinedare therefore both live readings of the same absent cell, and neither is "the" value. A consumer cannot distinguish "no value" from "column predates this partition" from the read.Two things the issue predicted that measurement contradicts
SELECT git_remote, 1 AS n FROM tcollapses the fast path and throws. It readsnull, because a literal reads no column, so the hint set is still one column and the query never leaves thescanColumnpath.WHEREon an unrelated column silently flips the projection's value.SELECT git_remote FROM treadsnull; addingWHERE date >= '...'makes the identical projection readundefined, because the predicate's column joins the hint set. This shape was not anticipated anywhere.Why it never throws (the mechanism, verified)
icebird builds each row with squirreling's
asyncRow(row, rowColumns)whererowColumns = scanColumns ?? columns, i.e. the requested list (node_modules/icebird/src/sql/icebergDataSource.js:91-92,186).asyncRowsetscells[key] = () => Promise.resolve(obj[key])for every requested key, so the cell exists and resolves toundefined;resolvedis the raw object, which has no such key. I confirmed the row shape directly:A parquet-backed partition builds
asyncRowoverObject.keys(data[0]), the row's physical keys (src/core/query/parquet-source.js:111-113), so the cell does not exist and evaluating it throwsColumnNotFoundError. That is the whole difference. On icebird,ORDER BY,GROUP BY,DISTINCT, an expression, and an aggregate over the absent column all answer where the parquet union throws:A correctness hole the measurement exposed
The maintainer asked for a test on "a
WHERE/aggregate on the absent column". Running it produced a wrong answer.withSchemaColumns.scanforwardedoptionsverbatim. LLP 0098#wrapper-duties already requires the wrapper to strip a predicate naming a declared-but-physically-absent column, but onlyscanColumnimplemented it. icebird converts such a predicate to a hyparquet filter over a column its schema never had, filters nothing away, and still reportsappliedWhere: true, so the engine trusts the stream and does not re-filter.Before the fix (3 fixture shapes, same queries):
Direct scan probe on the lone partition, before the fix:
Two or more partitions hid it, because
createDataSourcethen wrapsunionSources, whose own per-partition gate (LLP 0015#multi-partition-union) fires first. The exposed shape is the ordinary one: a fresh install with a single client.The fix mirrors the gate already in the wrapper's
scanColumn: when the predicate names a column the wrapped source does not advertise, drop it along withlimit/offset(only meaningful post-filter) and reportappliedWhere: false/appliedLimitOffset: false. A predicate the source can satisfy is still pushed and still claimed, so ordinary filtered reads keep their pushdown.After the fix, all three shapes agree:
I consider this a bug fix against an already-settled contract (LLP 0098#wrapper-duties states the duty; the union already honours it on its row path), not a new design call. If you disagree, this is the part to descope and I will resubmit the tests alone.
How to re-run this yourself
The regression pins genuinely bite. Reverting only
dataset.jsand re-running the new file:The other ten pass either way, which is the point: they describe the contract, not the fix.
What is in this PR
test/core/ai-gateway-absent-column-sql.test.js(new, 12 tests).executeSql+collectover a staged icebird cache in two shapes: one partition lacking the column (the no-union path), and a drifted pair. Every value asserted exactly (strictEqualagainstnull/undefined, key presence, the JSON rendering), never through a tolerant?? null. That tolerance is what let the mechanism be described wrongly five times while the suite stayed green; the existingai-gateway-dataset.test.jspin uses?? nulland cannot tell the two apart.llp/0240-icebird-absent-column-contract.decision.md(new). Records the table, the mechanism, and the gate.llp/0015andllp/0098gain forward-refs only. Both are Active/Accepted, so perCLAUDE.mdnothing they settled is rewritten. LLP 0015's union section is being corrected separately for the parquet backing in PR The union's absent-column contract is undefined-or-throws, not null (#731) #740; this branch does not touch it, so the two should not conflict.dataset.js: the row-path predicate gate, plus corrected block comment. The old comment claimed "a row object that lacks the key simply reads as null", which is the sixth wrong description and is now removed.message_projector.js: the v7 column comment now says what a read of one yields, and says not to branch on it.LLP number
0240. Computed, not guessed:
git ls-tree -rover all 49refs/remotes/origin/*refs plusllp/tombstones/. Highest claimed anywhere is 0239, and0240appears in no ref. Unclaimed gaps below the maximum are 0047, 0048, 0082, 0126, 0127, 0221, 0227; I took the next number above the maximum rather than backfill a gap, since a gap can be reserved by work not yet pushed and this repo has already had to renumber one collision (#775).What I could not establish
SELECT *, git_remote FROM tover a drifted union mis-assigns a value into a neighbouring declared column (gateway_idcame back holding thegit_remotevalue). That is a star-expansion defect above this layer, I did not chase it, and the tests deliberately do not cover it. Noted in LLP 0240's Consequences. It probably deserves its own issue.appendRowsToSourceTable-staged icebird tables. That is the same fixture route the existingai-gateway-dataset.test.jspins use, and it exercises the realcreateQueryStorageService/ icebird /withSchemaColumns/ squirreling stack, but it is not an acceptance run.null/undefinedsplit is worth removing (making the row path null-normalize too, so one value is the answer) is a design call I did not make. It would change query output, and LLP 0240 documents the split as it stands rather than deciding against it.Fixes #778