Union absent-column tests pin the pre-LLP-0241 contract, so master is red (#820) - #821
Conversation
… red (#820) Four tests in `test/core/union-source.test.js` fail on `master`, and because GitHub's `pull_request` runs build the branch merged into the base, every open PR inherits the failure: the drifted cell is unresolved and throws; only collect() turns it into undefined a partition whose rows carry no resolved map makes a bare projection throw evaluating a column one partition lacks throws, and so does a non-identifier sibling SELECT * keeps each partition row shape, so a drifted key is absent rather than undefined This is a semantic conflict between two changes that were each green alone. `192d3f9e` (#789) landed LLP 0241, which changed runtime behaviour: a scan's rows now carry the column list the scan advertised, so `unionSources` pads a partition that physically lacks a column with a real cell resolving to `undefined`. `70b9c1c7` (#740) landed afterwards but was cut before it, and is doc-and-tests only ("No runtime behaviour changes"). Its tests describe the tree as it stood before the padding. The tests are what is stale. LLP 0241 is Accepted and settles every one of the four behaviours in the padded direction, by name, in its own Consequences section: - "A padded cell resolves to `undefined`" and it is a cell, not the unresolved throwing thunk the first two tests inspect. - "A query whose `WHERE` or `ORDER BY` names a column some partition lacks stops throwing `ColumnNotFoundError` and answers", which is the third test. - "`Object.keys(row).length` for a star over a drifted partition now equals the declared column count rather than the physical one", which is the fourth. 0241 also states why that direction is the intended one rather than a regression: it is "the behaviour LLP 0015 already required of a union ('projecting an absent column reads as null, never throws'); the throw was the same short row surfacing on a different path". Satisfying #740's tests would mean reverting an Accepted decision's implementation, which is the wrong fix. Measured on the drifted two-partition parquet fixture the tests already build, current tree: `SELECT extra FROM t` gives `resolvedHasKey: true` and a cell resolving to `undefined` on both narrow rows; `WHERE extra = 'x'`, `ORDER BY extra`, `max(extra)`, `coalesce(extra, 'none')` and `SELECT extra, 1 AS n` all answer instead of throwing; and `SELECT *` yields keys `[id, score, extra]` on every row while still rendering `[{"id":1,"score":1.5},...]`, because `JSON.stringify` drops `undefined` exactly as it dropped the missing key. So: - Rewrite the four tests against the post-0241 contract, keeping each one's coverage intent (the cell mechanism, a hand-rolled source with no `resolved` map, the evaluating and non-identifier-sibling shapes, and the star) and repointing their `@ref`s at LLP 0241 §alignment. - Replace the now-false absent-column paragraph in the `unionSources` header comment. It described the same pre-0241 tree. - Correct the same paragraph in LLP 0015's "Multi-partition union", which already carried the `Extended-by: LLP 0241 §alignment` forward-ref pointing at the behaviour its prose contradicted, and record the second correction inline the way the first one was. No runtime behaviour changes. The three neutrally-worded ai-gateway comments #740 left ("the exact value depends on the read path") are still true and are untouched.
LLP 0015's corrected paragraph and the `unionSources` header both said every read path now agrees on `undefined`. The `scanColumn` column-stream path is not part of that agreement: the union forwards each partition's chunks unchanged, ai-gateway's `withSchemaColumns` is what maps the holes to `null` (dataset.js), and LLP 0241 says in as many words that it "does not touch the null/undefined split between the scanColumn and row paths". Pinned in-repo by test/core/ai-gateway-dataset.test.js:323, which asserts strict `null` on that path. As written, LLP 0015's paragraph also contradicted itself: "every read path agrees" two sentences before "the exact value a read of it yields depends on the read path". Scope both statements to the row path and name what the scanColumn path actually does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 1 review, PR #821Verdict: the central claim is correct. The tests were wrong, not the source. 1. Is tests-not-source the right fix?I re-derived this from the documents and the tree rather than from the PR body. LLP 0241 is
|
| tree | tests | pass | fail | skipped |
|---|---|---|---|---|
origin/master @ 4adfdb7b |
4258 | 4253 | 4 | 1 |
this branch @ 55d584b3 |
4258 | 4257 | 0 | 1 |
this branch @ db35cf52 (post-fix) |
4258 | 4257 | 0 | 1 |
test/core/union-source.test.js holds 21 test( declarations on both trees.
No skip, todo or only added anywhere in the diff. npm run typecheck
exits 0 on both. The four failures on master are exactly the four tests this
PR rewrites, by name.
2. Findings
[Medium] "Every read path agrees" over-claimed, and contradicted LLP 0241 and itself - FIXED
llp/0015-query-and-datasets.spec.md:84 (as pushed at 55d584b3) and
src/core/query/union-source.js:114-115:
Every read path agrees on that value: reading the row's pre-materialized
resolvedmap (collect()'s fast path), invoking the cell directly, and
evaluating the column above the scan in aWHERE[...]
and
* and is not a way to discover what a partition holds. What no longer varies is
* which read path the caller took: reading `resolved`, invoking the cell, andThe scanColumn column-stream path is a read path, it is not part of that
agreement, and padding did not touch it. LLP 0241 says so in as many words
(0241:108-112):
so this decision does not touch the
null/undefinedsplit between the
scanColumnand row paths [...] Whether that split should be collapsed
remains an open design question, not settled here.
It is live in-repo, not theoretical: test/core/ai-gateway-dataset.test.js:323
asserts assert.strictEqual(v, null, 'absent column streams null, not undefined') on the flagship ai_gateway_messages source. I traced where that
null comes from - unionSources' own scanColumn
(src/core/query/union-source.js:192-267) forwards each partition's chunks
unchanged; the normalization is done one layer up by
withSchemaColumns in hypaware-core/plugins-workspace/ai-gateway/src/dataset.js:225-229.
So neither "every read path agrees" nor "it yields null" would have been an
accurate statement about the union.
As written the paragraph also contradicted itself: 0015:84 said "every read
path agrees on that value" and 0015:93-94, ten lines later and untouched by
this PR, still says "the exact value a read of it then yields depends on the
read path and is not settled here".
Given this exact paragraph has now been measured false five times (per #740's
own descope commit message), an over-broad universal quantifier in it is worth
correcting rather than shipping.
Fixed in db35cf52. Both statements are scoped to the row path, and the
scanColumn path is named for what it actually does (forwards chunks
unchanged; a wrapper above normalizes if it wants uniformity) with the 0241
split called out as deliberately unsettled. Doc and comment only, no
executable line touched; suite still 4258 / 4257 pass / 0 fail, typecheck 0.
[Nit, not fixed] The #740 correction note was softened
The Corrected (#731, PR #740) note in llp/0015-query-and-datasets.spec.md: "That was never true of the code" ->
"That was never true of the code when it was written". The hedge slightly
under-states things: post-0241 the "never throws" half became true, but the
"reads as null" half is still false, so the original flat statement was not
wrong. Editing another PR's historical correction note is also the one edit in
this diff that is not stale-prose repair. Left alone: it is a one-clause
editorial change to a note, not to anything the spec settled, and it is
defensible as written. Flagging for the record only.
3. Verified conventions
- No U+2014 em dash anywhere in the diff, in either the original or my commit.
- No trailing semicolons in added JS.
SqlPrimitiveis reached via the existing top-of-file@importblock
(test/core/union-source.test.js:12), not an inlineimport('...')type.@refs repointed toLLP 0241#alignmentresolve: the anchor exists as
<a id="alignment"></a>at0241:78. The retained
LLP 0015#multi-partition-unionrefs resolve to the## Multi-partition unionheading at0015:51.- The branch still merges cleanly into
master. - Swept for other stale absent-column prose the PR should have caught: the
ai-gatewaydataset.js/message_projector.jscomments were already
neutralized to "depends on the read path" by The union's absent-column contract is undefined-or-throws, not null (#731) #740's own descope commit, and
llp/0098,llp/0032,llp/0096say nothing that this contradicts.
Also independently re-measured two doc claims the tests do not cover:
SELECT extra FROM t GROUP BY extra and SELECT DISTINCT extra FROM t over
the drifted fixture. Both answer ([{},{"extra":"x"}]), so LLP 0015's
enumeration is honest even though the pinned test set stops at WHERE,
coalesce, ORDER BY and max. That gap predates this PR (#740's version
had the same set) and is not worth widening here.
On the automated pass
The /code-review skill was launched against PR 821 and had not returned by the
end of this review window, so nothing from it is folded in below. Everything
above is independently derived: LLP quotations read from the documents, commit
ordering from git show, and every behavioural claim re-measured in a clean
worktree (including the two source mutations and the GROUP BY/DISTINCT
probe). No finding here rests on the skill or on the PR body.
Left for a human
Nothing blocking. Two things worth a maintainer's eye, neither a reason to
hold this PR:
origin/fix/issue-778carries anAccepted-marked LLP 0240 that is not in
master. It is complementary to 0241 rather than contradictory, but it was
cut on the same pre-0241 tree The union's absent-column contract is undefined-or-throws, not null (#731) #740 was, so it deserves the same rebase read
before landing.- The
null/undefinedsplit betweenscanColumnand the row path is still
an open design question by 0241's own words. Now named explicitly in both
places rather than papered over.
…iew) `withSchemaColumns.scanColumn` normalizes an absent column's `undefined` holes to `null` and its comment justified that as "the same ... value the row path reads". That was written in July, before LLP 0241. Post-0241 the row path pads an absent cell with `undefined`, so the two paths read different values, which is exactly the split this PR just scoped in LLP 0015 and in the `unionSources` header. Keep the real justification (one representation across the merged stream) and name the split instead of asserting sameness. Comment only. No runtime behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 2 review, PR #821Verdict: the central claim holds. The tests were wrong, not the source. 1. The perturbation check (round 1 died mid-sentence here)This is the failure mode that matters for a PR whose whole job is rewriting Baseline, unmutated: Mutation A - disable padding entirely
- yield* alignRows(scan.rows(), scanColumns)
+ yield* scan.rows()All four rewritten tests go red, and only those four. 21/21 -> 17 pass / Mutation B - pad with
|
| tree | tests | pass | fail | skipped |
|---|---|---|---|---|
pristine origin/master @ 4adfdb7b |
4258 | 4253 | 4 | 1 |
this branch @ db35cf52 |
4258 | 4257 | 0 | 1 |
branch merged into origin/master (what CI builds) |
4258 | 4257 | 0 | 1 |
branch @ 83674f1d (after my fix) |
4258 | 4257 | 0 | 1 |
Total is 4258 on every tree, so nothing was deleted or skipped to reach green.
test/core/union-source.test.js holds 21 test( declarations on master and
21 on the branch. No .skip / .only / .todo anywhere in the diff. The four
failures on master are exactly the four tests this PR rewrites, by name:
not ok 2273 - the drifted cell is unresolved and throws; only collect() turns it into undefined
not ok 2274 - a partition whose rows carry no resolved map makes a bare projection throw
not ok 2275 - evaluating a column one partition lacks throws, and so does a non-identifier sibling
not ok 2276 - SELECT * keeps each partition row shape, so a drifted key is absent rather than undefined
I also ran the merged tree explicitly, because unblocking CI for the other nine
PRs is the point: the merge is conflict-free and green. npm run typecheck
exits 0 on the branch and after my commit.
3. Tests-not-source, derived independently
I re-derived the direction from the documents and from measurement, not from the
PR body, and reached the same answer round 1 did.
llp/0241-scan-rows-carry-advertised-columns.decision.mdis Accepted, and
its Decision (0241:82-86) requires a scan's rows to carry
options.columns ?? source.columns, with "a column the partition does not
physically carry gets a padded cell rather than a missing slot". Its
Consequences settle all four assertions in the padded direction by name:
the padded cell resolving toundefined(against tests 18/19),WHEREand
ORDER BYanswering instead of throwing (against test 20), and
Object.keys(row).lengthequalling the declared count (against test 21).- The direction is settled too, not just the values:
0241:118-122calls the
padded reading "the behaviour LLP 0015 already required of a union
('projecting an absent column reads as null, never throws'); the throw was the
same short row surfacing on a different path". Making the source satisfy the
old tests would revert an Accepted decision's implementation and reinstate
SELECT *, <col> over a drifted union mis-assigns a value into a neighbouring column #788. - The
#788fix is not hypothetical collateral: Mutation D above is the
narrowest way to keep padding while dropping the advertised column list, and
it is precisely what reintroduces the sliding. Test 21 catches it.
I re-measured the doc's own enumeration on the drifted two-partition parquet
fixture, including the shapes the test set does not cover, to check LLP 0015
is not now over-claiming:
SELECT extra FROM t GROUP BY extra -> [{},{"extra":"x"}]
SELECT DISTINCT extra FROM t -> [{},{"extra":"x"}]
SELECT count(*) AS c FROM t WHERE extra IS NULL -> [{"c":2}]
SELECT extra FROM t ORDER BY extra DESC -> [{},{},{"extra":"x"}]
SELECT *, extra FROM t -> keys [id,score,extra] on every row
All answer, none throws. So GROUP BY / DISTINCT, which the corrected LLP
0015 paragraph names but the pinned test set stops short of, are honest. That
gap predates this PR (#740's version enumerated the same list) and is not worth
widening here.
4. The llp/0015 edit, and LLP 0240
llp/0015-query-and-datasets.spec.md is Status: Active, so this is the one
part of the diff that needs the "settled vs stale" test.
- The section it edits already carried
> **Extended-by: LLP 0241 §alignment**, pointing at the very behaviour the
prose below it contradicted. The PR corrects the prose in the direction the
doc's own forward-ref already pointed, and records the correction inline the
way The union's absent-column contract is undefined-or-throws, not null (#731) #740's correction was recorded. It does not touch what 0015 settled: the
limit/offset stripping, thewherepushdown gate, and "columns is always
forwarded" are all unchanged, word for word. - LLP 0240 is not in
master.llp/has 0241, 0242, 0243, 0244, 0246,
0247 and no 0240; the only commit adding
llp/0240-icebird-absent-column-contract.decision.mdis reachable solely from
origin/fix/issue-778. So there is no second Accepted document in this tree
for the PR to silently overrule. - I read 0240 off that branch anyway, since it is the doc most likely to
collide. It does not contradict the corrected 0015. Its contract table gives
nullfor the single-column-hint shapes andundefinedfor the row-path
shapes, and it already carries its own
> **Amended by LLP 0241 §alignment, which landed first**note moving exactly
one cell (the star). Round 1's scoping fix is what makes the two agree:
beforedb35cf52, 0015's "every read path agrees onundefined" would have
contradicted 0240'snullcolumn; after it, 0015 excludes thescanColumn
path by name and says the wrapper maps those holes tonull, which is 0240's
reading. Whoever lands Follow-up: document the icebird-backed absent-column contract deferred from PR #740 #778 still wants a rebase read against post-0241
master, for the same reason The union's absent-column contract is undefined-or-throws, not null (#731) #740 needed one, but not because of this PR.
I verified round 1's fix against the code rather than against its own prose:
unionSources.scanColumn (src/core/query/union-source.js:195-270) forwards
each partition's chunks unchanged in both the where and no-where branches
(the only mutation is the union's own limit/offset slicing), and
withSchemaColumns.scanColumn (ai-gateway/src/dataset.js:216-232) is what
rewrites undefined to null, pinned by
test/core/ai-gateway-dataset.test.js:323
(assert.strictEqual(v, null, 'absent column streams null, not undefined')).
So the round-1 fix is accurate, and it narrowed a false claim rather than a
test's coverage: db35cf52 touches no test file at all.
5. Findings
[Low, pre-existing, FIXED in 83674f1d] ai-gateway's scanColumn comment asserts the same sameness round 1 just removed
hypaware-core/plugins-workspace/ai-gateway/src/dataset.js:199-204, untouched by
this PR:
// for) surfaces its values as `undefined` holes in the chunk; normalize
// them to null, the same "this partition predates the column" value the
// row path reads, so accumulators see one representation either way.The row path does not read null there. Post-0241 it reads undefined,
which is the split LLP 0241 declines to settle and which round 1 spent its whole
commit scoping into LLP 0015 and the unionSources header. So the repo would
have shipped this PR's correction alongside the mirror image of the same
misstatement, one layer up, in the file that implements the null half.
Dated it before flagging: the comment region's newest commit is 6bd85c46
(2026-07-11), a month before LLP 0241 (2026-08-15), so this is pre-existing debt
that 0241 falsified rather than anything #821 introduced. That is why it is Low
and why it did not block.
Fixed as a comment-only change: keep the real justification (one representation
across the merged stream, which is what the accumulator needs) and name the
split instead of asserting sameness. npm test 4257/0 and npm run typecheck
clean after it.
[Note, not actionable] Round 1's own record is slightly off about test 17
Round 1 wrote that the undefined-not-null claim for the star shape is
"carried by the still-passing test 17". It is not: test 17 passes under
Mutation B. Every one of its shapes keeps executeProject's resolveable
gate open, so collect() answers from the pre-materialized resolved map,
which has no entry for the drifted column and yields undefined whatever the
cell resolves to. Its rows[0]?.[key] === null assertion therefore cannot see
a null-padding regression. No coverage hole results, because tests 18, 19 and 20
all catch Mutation B, but the attribution in the round-1 record is wrong and
should not be relied on by a later reader.
[Note, for whoever maintains LLP 0032] one compressed claim now reads wrong
llp/0032-github-llm-graph-bridge.decision.md:104-106 says the gateway source
"exposes its declared schema columns (padding absent physical columns to
null)". That is true of scanColumn and false of the row path, i.e. the same
compression this PR just uncompressed in 0015. 0032 is Accepted and this PR has
no business editing it; flagging so it is on the record for a future extension.
6. Conventions
- No U+2014 em dash anywhere in the diff, original or either review commit.
- No trailing semicolons in added JS.
SqlPrimitivereaches the test file through the existing top-of-file
@importblock (test/core/union-source.test.js:12), not an inline
import('...')type.npm run typecheckis clean, so the added
(SqlPrimitive | undefined)[]tuple casts check out.@refs:LLP 0241#alignmentresolves (<a id="alignment"></a>exists in
0241); the retainedLLP 0015#multi-partition-unionrefs resolve to the
## Multi-partition unionheading. Each is attached with no intervening blank
line, and each gloss says something the test name does not.- Swept the tree for other absent-column prose the correction falsifies:
message_projector.js:28anddataset.js:166are already neutral ("depends
on the read path"),llp/0098andllp/0096say nothing this contradicts,
and the only two remaining hits are the two Notes above.
7. On the automated pass
Not used. /code-review was launched in round 1 and never returned, and this
round I did not fold in anything from it. Everything above is first-hand: LLP
text read from the documents, commit dates from git log, the mutation runs and
all four suite totals measured in throwaway worktrees, and the GROUP BY /
DISTINCT / star probes run against the same drifted parquet fixture the tests
build.
Left for a human
Nothing blocking. This is the fix for red master and it is correct, correctly
scoped, and now demonstrably mutation-sensitive. Head is 83674f1d.
|
Triage at head |
philcunliffe
added
the
neutral:approved
1 participant
masteris red on four union-source absent-column tests, and because GitHub'spull_requestruns build the branch merged into the base, every open PRinherits the failure regardless of its own content.
Which of the two readings is correct
The tests assert the pre-LLP-0241 contract and are what needed updating.
The source is right.
The two changes were each green alone and conflicted semantically on merge:
192d3f9eunionSourcespads every row out to the column list the scan advertised, so a column a partition physically lacks becomes a real cell resolving toundefined.70b9c1c7The LLP evidence
LLP 0241 is
Accepted, and its Consequences section settles all four failingassertions by name, in the padded direction:
against tests 1 and 2, which assert an unresolved cell and a throwing thunk.
against test 3, which asserts those shapes throw.
against test 4, which asserts the drifted key is absent.
And §alignment states why the padded direction is the intended one rather than
a regression to be reverted:
Making the source satisfy #740's tests would mean reverting an Accepted
decision's implementation, which CLAUDE.md forbids and which would reinstate
issue #788 (
SELECT *, git_remoteanswering withgit_remote's value underthe name
gateway_id).Measured on the current tree
On the drifted two-partition parquet fixture the tests already build:
SELECT extra FROM tgivesresolvedHasKey: trueand a cell that resolves,to
undefinedon both narrow rows and"x"on the wide one. No throw.WHERE extra = 'x',ORDER BY extra,max(extra),coalesce(extra, 'none'),SELECT extra, 1 AS nandSELECT extra, score * 2 AS dall answer where they used to throw.AsyncDataSourcewith noresolvedmap answers too: thepadding happens in the union, below
executeProject, so where the row'scells came from no longer decides the result.
SELECT *yields keys[id, score, extra]on every row and still renders[{"id":1,"score":1.5},{"id":2,"score":2.5},{"id":3,"score":3.5,"extra":"x"}],because
JSON.stringifydropsundefinedexactly as it dropped the missingkey. That is 0241's "renders identically" consequence, now pinned.
Changes
test/core/union-source.test.js: the four tests rewritten against thepost-0241 contract, each keeping its original coverage intent (the cell
mechanism, a source with no
resolvedmap, the evaluating andnon-identifier-sibling shapes, the star), with
@refs repointed toLLP 0241 §alignment.
src/core/query/union-source.js: the absent-column paragraph in theunionSourcesheader comment described the same pre-0241 tree. Replaced.llp/0015-query-and-datasets.spec.md: same paragraph, in a section thatalready carried an
Extended-by: LLP 0241 §alignmentforward-ref pointing atthe behaviour its own prose contradicted. Corrected, with the second
correction recorded inline the way the first one was.
No runtime behaviour changes. The neutrally-worded ai-gateway comments #740
left ("the exact value depends on the read path") are still true and untouched,
as is the still-passing
a projected column one partition lacks reads as undefined, never nulltest.Gate
origin/master)npm testnpm run typecheckNo test was deleted and none was skipped: the suite total is 4258 both ways.
Fixes #820