Skip to content

A scan's rows carry the column list the scan advertised (#788) - #789

Merged
philcunliffe merged 3 commits into
masterfrom
fix/issue-788
Aug 17, 2026
Merged

A scan's rows carry the column list the scan advertised (#788)#789
philcunliffe merged 3 commits into
masterfrom
fix/issue-788

Conversation

@philcunliffe

@philcunliffe philcunliffe commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Nothing below was described from reading the code. Every claim is a run and its output. The reproduction is a fixture you can re-run.

Reproduced, and it is broader than reported

Stage an ai_gateway_messages cache with one icebird partition whose schema never had git_remote (the normal post-v7 state, LLP 0032), plus optionally a second that does have it. Columns id, gateway_id, date are physical in both; the dataset declares 57.

On origin/master:

=== lone (ONE partition, unionSources not involved) ===
  SELECT *, gateway_id AS trailing FROM t
    {"id":1,"gateway_id":"gw-narrow","date":"2026-05-26","schema_version":"gw-narrow"}
  SELECT *, id AS n FROM t
    {"id":1,"gateway_id":"gw-narrow","date":"2026-05-26","schema_version":1}
  SELECT *, 1 AS lit FROM t
    THREW TypeError: asyncRow.cells[k] is not a function

=== drifted (one partition with git_remote, one without) ===
  SELECT *, git_remote AS gr FROM t
    {"id":1,"gateway_id":"gw-narrow","date":"2026-05-26"}
    {"id":2,...,"git_remote":"git@example.com:acme/app.git","schema_version":"git@example.com:acme/app.git"}
  SELECT *, gateway_id AS trailing FROM t
    {"id":1,"gateway_id":"gw-narrow","date":"2026-05-26","git_remote":"gw-narrow"}
    {"id":2,...,"git_remote":"git@example.com:acme/app.git","schema_version":"gw-wide"}

Three things the issue did not have:

  1. It does not need a union. A single partition lacking a declared column reproduces it. createDataSource returns withSchemaColumns(sources[0]) with no union underneath, and the mis-assignment is already there.
  2. The victim column is not fixed. The value lands under whichever declared column sits at the star's physical width: schema_version on the lone partition, git_remote on the narrow half of the drifted union. "gateway_id held git_remote's value" is one instance of a family.
  3. The same misalignment also crashes. SELECT *, <literal> throws TypeError: asyncRow.cells[k] is not a function. Same root cause, surfacing as a hard failure instead of a wrong value.

Root cause, established by isolation not by reading

The defect is not in icebird, not in hyparquet, and not in withSchemaColumns' column declaration. A hand-rolled AsyncDataSource with no HypAware and no icebird anywhere reproduces it exactly:

// declares columns [a, b, c, d]; yields rows whose `columns` is only [a, b]
SELECT * FROM t      -> [{"a":1,"b":2}]
SELECT *, b FROM t   -> [{"a":1,"b":2,"c":2}]   // b's value, under the name c

Instrumenting the layer boundary shows the invariant that is broken:

source.columns.length: 57
  row.columns       : ["id","gateway_id","date"]
  row.cells keys    : ["id","gateway_id","date"]

and what the engine passes for a star:

SELECT * FROM t       scan options.columns = undefined
SELECT *, b FROM t    scan options.columns = undefined
SELECT a, b FROM t    scan options.columns = ["a","b"]

Squirreling derives a query's output column names once, from the scan's advertised list (executeScan returns columns: plan.hints.columns ?? table.columns; selectColumnNames expands the star over that), then fills them per row by walking that row's own columns and advancing a shared colIdx. A row narrower than the advertised list under-runs colIdx, so every output name after the star lands one or more slots early. Since a star never carries a columns hint, the advertised list for any star is the source's full declared set, and a drifted partition's row is always short.

Nothing above the source can repair this: the output name list is fixed and already reported in QueryResults.columns before the first row is read. So the duty belongs on the source.

The fix

A scan must yield rows whose columns equals options.columns ?? source.columns. A column the partition does not physically carry becomes a padded cell rather than a missing slot. This is not a new promise: it is the schema the engine already reported to the caller. Only the row objects disagreed with it.

Core gains alignRowColumns / alignRows next to unionSources, applied at the two places a HypAware row can be narrower than what its source advertises:

  • unionSources.scan (the union advertises the union of its partitions' columns; each partition yields only its own, which covers parquet-backed unions where parquetDataSource derives columns from Object.keys(data[0]))
  • withSchemaColumns.scan in the AI-gateway dataset (the wrapper advertises the declared schema over partitions that predate part of it, and the single-partition path has no union underneath)

A row that already matches is returned untouched, and the verdict is memoized by columns array identity, so the ordinary case costs one reference compare per row.

The padded cell resolves to undefined, deliberately the same value the row path already read for a declared-but-absent column. This PR therefore does not touch the null/undefined split measured in #778.

An earlier draft of this description claimed it changes no existing query's answer. Review measured otherwise, and the real picture is below.

After the fix, same fixture, same queries:

=== lone ===
  SELECT *, gateway_id AS trailing FROM t
    {"id":1,"gateway_id":"gw-narrow","date":"2026-05-26","trailing":"gw-narrow"}
  SELECT *, id AS n FROM t
    {"id":1,"gateway_id":"gw-narrow","date":"2026-05-26","n":1}
  SELECT *, 1 AS lit FROM t
    {"id":1,"gateway_id":"gw-narrow","date":"2026-05-26","lit":1}

=== drifted ===
  SELECT *, git_remote AS gr FROM t
    {"id":1,"gateway_id":"gw-narrow","date":"2026-05-26"}
    {"id":2,...,"git_remote":"git@example.com:acme/app.git","gr":"git@example.com:acme/app.git"}
  SELECT *, gateway_id AS trailing FROM t
    {"id":1,"gateway_id":"gw-narrow","date":"2026-05-26","trailing":"gw-narrow"}
    {"id":2,...,"git_remote":"git@example.com:acme/app.git","trailing":"gw-wide"}

SELECT * is byte-identical before and after: the padded values are undefined, which JSON.stringify drops exactly as it dropped the missing key.

before: {"id":1,"gateway_id":"gw-narrow","date":"2026-05-26"}   [Object.keys = 3]
after : {"id":1,"gateway_id":"gw-narrow","date":"2026-05-26"}   [Object.keys = 57]

Three observable differences, all measured by review over a 23-query battery comparing master with this branch.

1. Four queries go from throwing to answering. SELECT * WHERE <absent> IS NULL, WHERE <absent> = ..., and ORDER BY <absent> in both directions raised ColumnNotFoundError on master, because a clause evaluated above the scan read the column off row.cells and missed on a short row. They now answer. This is a widening, not a regression: LLP 0015 already required that an absent column never throws. The answers were checked against the hinted form of each query, which never went short and never threw: IS NULL matches every row of a partition lacking the column, IS NOT NULL matches none, and ORDER BY over an all-undefined group is a stable no-op in scan order. No query moved from throwing to answering wrongly.

2. Key order. A star over a partition whose physical column order differs from the advertised list now renders keys in the advertised order, same values. That is the order QueryResults.columns already reported, so it settles a disagreement rather than creating one.

3. Object.keys(row).length. A consumer that enumerated a result row's keys to discover which columns a partition physically held loses that signal. It was never sound: the two halves of a drifted union answered it differently for the same query, and QueryResults.columns already reported the declared list. Called out in LLP 0241's Consequences.

The regression test, failing before and passing after

test/core/star-expansion-drifted-union.test.js (7 tests). Cell occupants are asserted by name with strictEqual, so a row of the right shape carrying the wrong values cannot pass.

Reverting only the two call sites (keeping the helper so the file still loads):

not ok 1 - star expansion: a trailing column keeps its own name over a lone drifted partition
not ok 2 - star expansion: a trailing column keeps its own name on both halves of a drifted union
not ok 3 - star expansion: SELECT *, git_remote reads git_remote, not a neighbour
not ok 4 - star expansion: a literal beside a star does not crash over a drifted partition
ok 5    - star expansion: a bare star still renders only the columns a partition holds
not ok 6 - union: a partition missing a unioned column still yields the union column list
ok 7    - alignRowColumns: pads a short row and leaves an already-aligned row untouched
# tests 7 / pass 2 / fail 5

The two that pass either way are the deliberate no-change pin (SELECT * rendering) and the helper's own unit test.

With the fix: # tests 7 / pass 7 / fail 0.

Checks

npm test           # 4091 tests, 4090 pass, 0 fail, 1 skipped
npm run typecheck  # clean
npm run smoke -- gateway_claude_capture             # ok
npm run smoke -- gateway_codex_capture              # ok
npm run smoke -- local_parquet_export               # ok
npm run smoke -- otel_loopback_capture              # ok
npm run smoke -- hypignore_capture_drop             # ok
npm run smoke -- local_only_export_withhold         # ok
npm run smoke -- status_diagnostics                 # ok
npm run smoke -- cli_bundled_plugins_activated      # ok
npm run smoke -- package_bin_boot                   # ok
npm run smoke -- daemon_foreground_start_stop       # ok
npm run smoke -- daemon_install_render              # ok
npm run smoke -- walkthrough_picker_to_first_query  # ok
npm run smoke -- client_attach_idempotent           # ok
npm run smoke -- core_boot_noop                     # ok

Based on origin/master with a real npm install. Nothing here depends on PR #787. #787's where-forwarding gate is on withSchemaColumns.scan too, so the two touch adjacent lines in that function and will likely need a trivial conflict resolution, but neither fix needs the other and the contract #787 documents is unchanged by this one.

LLP number

0241. Computed, not guessed: git ls-tree -r over every refs/remotes/origin/* ref plus llp/tombstones/. Highest claimed anywhere is 0240 (fix/issue-778); 0241 appears in no ref. LLP 0015 gains a forward-ref only, since it is Active and this extends rather than rewrites what it settled.

Not chased here

unionSources is imported by otel, gascity, s3, context-graph, and context-graph-enrich; the union fix covers them, but only the AI-gateway dataset has a schema wrapper, so a plugin that declares columns outside a union would need the same alignment. None currently does. No separate issue filed.

Fixes #788

test and others added 2 commits August 15, 2026 09:13
Over a drifted union, `SELECT *, git_remote` returned a row with another
column holding git_remote's value. Reproduced first, then explained.

The engine derives a query's output column names once, from the scan's
advertised list, then fills them by walking each ROW's own `columns` and
advancing a shared index. A partition that predates a declared column
yields a shorter row, which under-runs the index and slides every output
name after the star onto its neighbour's value. Isolated with a
hand-rolled source, no icebird and no HypAware in the picture.

Fix: align a scan's rows to `options.columns ?? source.columns` at the two
places a HypAware row can be narrower than what its source advertises,
`unionSources.scan` and the AI-gateway `withSchemaColumns.scan`. A padded
cell resolves to `undefined`, the same value the row path already read for
an absent column, so no existing query's answer changes and `SELECT *`
renders identically.

Co-Authored-By: Claude <noreply@anthropic.com>
Review measured a second observable delta the PR did not claim: on the
drifted two-partition fixture, `SELECT * FROM t WHERE git_remote IS NULL`,
`WHERE git_remote = 'zzz'` and `ORDER BY git_remote` all threw
`ColumnNotFoundError` on master and now answer. A clause the engine evaluates
above the scan reads the column off `row.cells`, which missed on a short row.
That is the behaviour LLP 0015 already required of a union, so it is a
widening of the fix rather than a regression, but "the one observable
difference is Object.keys(row).length" was not true as written.

LLP 0241 now records it, along with the measured cost of the per-row rebuild
(~25ms to ~150ms for a star over 20k rows of a 3-of-57 partition; unchanged
over a partition holding every declared column), and the test file pins the
three queries. The new test fails with the two call sites reverted.

Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Verdict: findings, one fixed in-branch, none blocking

Reviewed manually (no codex, code-review not invocable in this environment). Review started at de5fc92; the fix commit 2cd2307 below is mine and the marker is on the current head.

Everything below is a run in a clean worktree with a real npm install (squirreling 0.15.3), compared side by side against a second worktree at origin/master.

What I re-measured

The pure-squirreling isolation reproduces exactly as claimed, at both master and this head (the raw source bypasses both call sites, so it must):

declare [a,b,c,d], yield rows whose columns is [a,b]
SELECT *  FROM t   => {"a":1,"b":2}
SELECT *, b FROM t => {"a":1,"b":2,"c":2}     b's value, under the name c
SELECT *, 1 AS lit => THREW TypeError: asyncRow.cells[k] is not a function

I read the mechanism out of the installed engine rather than taking it on trust: executeProject (squirreling/src/execute/execute.js:540-620) names output columns once via selectColumnNames(plan.columns, child.columns) and advances colIdx only by walking row.columns inside the star branch. Short row, under-run index, slid names. The description in LLP 0241 §mechanism is accurate.

All three corrections to issue #788 hold. No union needed (the lone fixture reproduces through withSchemaColumns alone). The victim is positional, not gateway_id (schema_version on lone, git_remote on the narrow half of the drifted union). And the literal-trailing crash is real and is gone at this head: SELECT *, 1 AS lit threw TypeError: asyncRow.cells[k] is not a function on master for both fixtures, and returns {"id":1,...,"lit":1} here.

The reported defect is gone, on both fixtures, for SELECT *, gateway_id AS trailing, SELECT *, id AS n and SELECT *, git_remote AS gr.

Finding 1 (medium, accuracy) - "no existing query's answer changes" is not true as written. Fixed in 2cd2307.

llp/0241-scan-rows-carry-advertised-columns.decision.md:111 and the PR body both state the only observable delta is Object.keys(row).length. I ran a 23-query battery over the lone and drifted fixtures on master and on this head and diffed. Four queries changed by more than a key count, all on the drifted union:

query master this head
SELECT * FROM t WHERE git_remote IS NULL ORDER BY id THREW ColumnNotFoundError: Column "git_remote" not found. Available columns: id, gateway_id, date (row 1) returns the narrow row
SELECT * FROM t WHERE git_remote = 'zzz' ORDER BY id same throw returns []
SELECT * FROM t ORDER BY git_remote same throw returns both rows
SELECT * FROM t ORDER BY git_remote DESC same throw returns both rows

Reproduced independently of icebird at the core union too: SELECT COUNT(*) AS n FROM t WHERE c IS NULL over unionSources([{a,b},{a,b,c}]) threw on master and returns {"n":2} here.

Cause: a clause the engine evaluates above the scan reads the column off row.cells (squirreling/src/expression/evaluate.js:111-125), which missed on a short row and raised. On a padded row it reads undefined. The star matters - only a star scan carries no columns hint, so SELECT id FROM t WHERE git_remote IS NULL hints both columns and never reproduced this.

This is a widening of the fix, not a regression: LLP 0015 already requires that a union "reads as null, never throws", so master was violating it on this path. But an Accepted decision doc landing with an incomplete Consequences list is exactly the failure mode #778 was written about, so I corrected it rather than leaving it: LLP 0241 now records the delta and the test file pins the three queries. The new test fails with the two call sites reverted and passes with them.

Verified, no finding

Everything else in the battery is byte-identical. SELECT * JSON rendering (both halves), COUNT(*), COUNT(git_remote) (0/1), COUNT(gateway_id), COUNT(DISTINCT ...), MAX/MIN, GROUP BY git_remote (still null, so the LLP 0240 null/undefined split is untouched), DISTINCT git_remote, LIMIT/OFFSET, SELECT t.*, explicit projection of an absent column ([{}], key absent, both trees). The Object.keys(row).length 3 to 57 growth is real and is the only change to a query that already succeeded.

Memoization. alignedColumns is local to each alignRows generator, so it cannot leak between scans - I checked directly by running two alignRows over the same columns array identity with different targets; the second aligned correctly. It retains one string[] reference and never a row, so a long scan does not grow. The one way to get a stale alignment is a source that mutates its row columns array in place while keeping identity; I built that case and it does pass a short row through unaligned. No source in this repo does that (icebird builds from options.columns, parquetDataSource from Object.keys(data[0])), and squirreling's own cachedDataSource and memorySource already capture columns by reference, so treating these arrays as immutable is the existing house rule. Not a finding.

Scope of application. Every consumer (otel, gascity, s3, context-graph, context-graph-enrich) builds partitions from storage.dataSourceForTable or parquetDataSource, both of which derive columns from the physical schema, so union.columns is always a superset and the alignment never drops a column a partition really holds. The dropping of extras (a row wider than the advertised list) is safe because plan.hints.columns is built from identifiers in every clause, not just the projection - I verified with a deliberately hint-ignoring source across WHERE, ORDER BY, GROUP BY/HAVING, DISTINCT, a FROM subquery and a JOIN: all identical to master. The hints.columns = [] case (SELECT 1 FROM t, SELECT COUNT(*)) is also identical.

No double-align hazard, though there is redundant work - see finding 2.

LLP numbering. 0241 is free on origin/master, in llp/tombstones/, and across all 49 remote branches (only fix/issue-788 carries it). 0240 is on fix/issue-778 as stated. Both anchors (#mechanism, #alignment) exist and all three @refs resolve to #alignment.

LLP 0015 got an additive Extended-by: block and nothing it settled was rewritten.

Test split is exactly as claimed. Reverting only the two call sites (keeping the helper): # tests 7 / pass 2 / fail 5, and the two passers are the deliberate SELECT * no-change pin and the alignRowColumns unit test. With my added test: 6 fail / 2 pass.

Conventions. No semicolons, no U+2014 anywhere in the changed files including LLP prose, no @typedef, no inline import('...') types, JSDoc-only types. npm run typecheck clean. Full suite green: 4091 pass / 0 fail (4092 with mine).

Finding 2 (low, efficiency) - not fixed, preference

Padding is a per-row rebuild and it is not free on a drifted partition. Measured over 20k rows:

  • 3-of-57 partition, SELECT *: ~25ms on master, ~150ms here (6x).
  • Partition holding every declared column: ~550ms both ways, no regression - the content compare plus the per-stream memo does its job.

Also, hypaware-core/plugins-workspace/ai-gateway/src/dataset.js:154 is withSchemaColumns(unionSources(sources)), so a narrow row in a multi-partition cache is rebuilt twice: once to the union's physical list, again to the wrapper's declared list. Instrumented count over a 2x100-row drifted fixture: 300 alignRowColumns rebuilds for 200 rows.

Worth noting because src/core/cache/storage.js:378-385 removed a per-row rebuild in this exact path on the grounds that it "dominated scan-side garbage on large datasets". I did not change this: the cost is proportionate (the result rows genuinely have 57 keys now) and any dedupe would mean the union skipping alignment when a wrapper above it re-aligns, which is more coupling than the saving is worth. Recorded in LLP 0241's Consequences with the numbers so a future reader has them. Preference, not a blocker.

Fix pushed

2cd2307 - "Record the WHERE/ORDER BY change padding also makes (#788)". LLP 0241 Consequences + one test (test/core/star-expansion-drifted-union.test.js), no production code touched. Verified present in the pushed tree. npm test 4092/4091 pass 0 fail, npm run typecheck clean at the new head.

Nothing needs a human. The known trivial textual conflict with #787 over withSchemaColumns.scan still stands and is unaffected by my commit.

…788)

Round 2 measured two things round 1 left open.

Neither alignment pass is removable. The wrapper cannot defer to the union:
the union aligns to what its partitions PHYSICALLY hold, the wrapper to the
DECLARED schema, a strict superset whenever a column is absent from every
partition. Removing only the wrapper's pass puts the reported defect straight
back on the multi-partition path, including the `SELECT *, <literal>` crash.
The union cannot defer to the wrapper either, since `unionSources` is a core
export with no wrapper above it in otel, gascity, s3 and context-graph, and
dropping it measured ~178ms against ~173ms with it, inside the noise: the
union rebuild spans a handful of columns, the wrapper's spans all 57.

A star over a partition whose physical column order differs from the
advertised list now renders its keys in the advertised order. Measured at the
core union over `[a, b]` and `[b, a]`: `{"b":4,"a":3}` before, `{"a":3,"b":4}`
now, same values. That is a third change to a query that already succeeded,
so "Object.keys(row).length is the only one" was still not exact. The new
order is the one QueryResults.columns already reported. Pinned.

Also recorded that the widened WHERE/ORDER BY answers were checked against
the hinted form of each query, which never went short and never threw.

Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Verdict: findings, two fixed in-branch, one left for triage. No production blocker.

Round 2 of 2. Reviewed manually: codex is not installed and code-review is not invocable in this environment, so everything below is a run in a clean worktree with a real npm install, diffed against a second worktree at origin/master. Review started at 2cd2307; a407168 below is mine and the marker is on the current head.

Round 1's items are not re-litigated. This round did the three things it was asked to do and one more.

1. Round 1's own fix 2cd2307 is accurate and complete

  • The new test genuinely fails on revert. Reverting only the two call sites (keeping the helper): # tests 8 / pass 2 / fail 6, and not ok 6 - a clause above the scan reads a column some partition lacks fails with exactly the recorded cause, ColumnNotFoundError: Column "git_remote" not found. Available columns: id, gateway_id, date (row 1), raised at squirreling/src/expression/evaluate.js:125. It is pinning the real thing, not a proxy.
  • I also split the revert to see which pass each test pins. Reverting only the wrapper (dataset.js) and keeping the union: tests 1-4 fail and SELECT *, 1 AS lit throws TypeError again. Reverting only the union: tests 7-8 fail. Both call sites are load-bearing and each is pinned.
  • LLP 0241's Consequences describes the widening correctly, on the facts it states. It was still not exact about scope, see finding 3.

2. The widened answers are correct (item 3 of the brief)

Four queries went throw-to-answer. I checked the answers, not just that they exist, against the strongest available reference: the hinted form of each query (SELECT id FROM t WHERE ... carries a columns hint, so it never went short and never threw, on either tree). Fixture widened to 4 narrow rows staged out of id order (scan order 3, 1, 4, 2) plus 2 wide rows, so a scramble would be visible.

Drifted two-partition fixture, star form, at this head:

query master head hinted reference (both trees)
SELECT * WHERE git_remote IS NULL threw [1,2,3,4] [1,2,3,4]
SELECT * WHERE git_remote IS NOT NULL threw [5,6] [5,6]
SELECT * WHERE git_remote = 'zzz' threw [] []
SELECT * WHERE git_remote <> 'zzz' threw [5,6] [5,6]
SELECT * ORDER BY git_remote threw [3,1,4,2,5,6] [3,1,4,2,5,6]
SELECT * ORDER BY git_remote DESC threw [3,1,4,2,6,5] [3,1,4,2,6,5]

Every widened answer matches the reference exactly. Specifically:

  • IS NULL over a partition physically lacking the column matches all its rows (4 of 4), and IS NOT NULL matches none of them. undefined reads as NULL because compareForTerm and the evaluator both test == null (squirreling/src/execute/utils.js:16-17).
  • ORDER BY on the all-undefined group is a stable no-op, not a scramble: the four narrow rows come back 3, 1, 4, 2, their scan order, because compareForTerm returns 0 for two nulls and Array.prototype.sort is stable. ORDER BY git_remote, id correctly re-sorts within that tie to 1, 2, 3, 4. Nulls sort first in both directions, which is squirreling's own convention (nullsFirst = term.nulls !== 'LAST'), unchanged by this PR.
  • COUNT(git_remote), GROUP BY git_remote (still null), DISTINCT, COALESCE all match master.

Conclusion: no query answers wrongly. There is no throw-to-wrong-answer transition anywhere in the battery. The widening is desirable and LLP 0015 compliant.

One thing that does answer wrongly, and it is not this PR

On the lone (single-partition) fixture, every WHERE on the absent column matches every row, on master and at this head identically:

lone, master AND head:  WHERE git_remote IS NOT NULL  => [1,2,3,4]   (should be [])
lone, master AND head:  WHERE git_remote = 'zzz'      => [1,2,3,4]   (should be [])

Cause: withSchemaColumns.scan forwards where to a partition that physically lacks the predicate column with no canPushWhere gate (the union has one, the wrapper does not), and the result comes back appliedWhere: true, so the engine never re-filters. That is precisely the gate PR #787 adds. It is pre-existing, byte-identical before and after this PR, and outside its scope. Flagged only so triage does not read it as a regression from this change. Not a finding against #789.

3. Finding (low, accuracy) - a third change to an already-succeeding query. Fixed in a407168.

LLP 0241 still said Object.keys(row).length "is the only change to a query that already succeeded". It is not. A star over a partition whose physical column order differs from the advertised list now renders its keys in the advertised order:

unionSources([ declares [a,b] yielding {a:1,b:2}, declares [b,a] yielding {b:4,a:3} ])
SELECT * FROM t ORDER BY a
  master: [{"a":1,"b":2},{"b":4,"a":3}]
  head  : [{"a":1,"b":2},{"a":3,"b":4}]

Values are identical; only JSON key order moved, and it moved onto the order QueryResults.columns had already reported for the same query, so this settles a disagreement rather than creating one. Same class of miss round 1 was correcting, so I corrected it rather than leaving it: LLP 0241 records it, and a new test (union: a star renders keys in the advertised order) pins it and fails when the union call site is reverted.

4. Double-alignment: recommendation is do not remove it, in this PR or a later one. Not fixed because it should not be.

The brief asked whether withSchemaColumns.scan can skip its own alignRows when its child is a unionSources that already aligns. No, and not for a subtle reason. The union aligns rows to union.columns, the union of what its partitions physically hold. The wrapper advertises the declared schema, which is a strict superset whenever a declared column is absent from every partition, and that is the normal post-schema-bump state this wrapper exists for (LLP 0032). A union-aligned row is still short of the wrapper's list, so the wrapper's pass is not redundant at all. Demonstrated, not argued: with the wrapper's alignRows removed and the union's kept, the reported defect returns in full, tests 1-4 fail and SELECT *, 1 AS lit throws TypeError again on the drifted union.

The genuinely redundant pass is the union's, and it cannot be removed either:

  • unionSources is a core export with no wrapper above it in otel, gascity, s3, context-graph and context-graph-enrich, so it cannot skip unconditionally. Skipping conditionally means threading a flag down from the wrapper.
  • That coupling would buy nothing measurable. I removed the union pass and measured: SELECT * over 20k rows, drifted, ~178ms without it against ~173ms with it, i.e. inside the run-to-run noise (spread was 134-216ms). Round 1's "300 rebuilds for 200 rows" is a true count but a misleading cost signal: the union rebuild spans the 3-4 columns a partition physically holds, the wrapper rebuild spans all 57. The 6x is entirely the wrapper's 57-key rebuild, which is inherent to the fix, not the duplication.

Re-measured round 1's headline numbers and they hold: SELECT * over 20k rows of a 3-of-57 partition, ~23ms on master to ~160ms here; drifted, ~26ms to ~173ms. The comparison to src/core/cache/storage.js:378-385 is fair but does not apply: that removed a rebuild that bought nothing, this one buys the row shape the engine already promised.

So the 6x remains, and it is a preference, not a blocker - it is proportionate (the rows genuinely carry 57 keys now) and there is no cheaper way to get correct star expansion at the source. Recorded in LLP 0241 with the measurement, so this question is settled rather than left open for a future reader to re-open.

5. Finding (medium, accuracy) - the PR body still carries the disproved claim. Not fixed: needs a human or the CLI.

The PR body still says, twice:

A padded cell resolves to undefined ... and changes no existing query's answer.

The one observable difference is Object.keys(row).length.

Round 1 corrected LLP 0241 but not the body. The body becomes the squash-merge message, so as it stands this PR would land a permanently wrong claim in master's history, disproved by round 1's own measurement and again by finding 3 above. The review rung must not gh pr edit, so I am recording it rather than fixing it.

Suggested replacement for those two sentences, matching what LLP 0241 now says:

A padded cell resolves to undefined, the same value the row path already read for a declared-but-absent column, so no query that already returned a value returns a different one. Three things do change: Object.keys(row).length for a star over a drifted partition now equals the declared column count; a star over a partition whose physical column order differs from the advertised list now renders keys in the advertised order (same values); and a WHERE or ORDER BY naming a column some partition lacks stops throwing ColumnNotFoundError and answers, which is the behaviour LLP 0015 already required. See LLP 0241's Consequences.

Classification: preference for the code, but it must be actioned before squash-merge. It cannot regress runtime behaviour, so it is not a production blocker; it is a documentation-accuracy blocker on the merge commit message. Triage should either edit the body or accept an inaccurate squash message.

Verified, no finding

  • ScanResults has exactly three fields (rows, appliedWhere, appliedLimitOffset, squirreling/src/types.d.ts:89-93), so the wrapper rebuilding the scan result object instead of forwarding it drops nothing.
  • No row.columns mutation anywhere in squirreling (grepped for push/sort/splice/assignment), so aligned rows sharing one columns array by identity, and that array being the source's own columns, cannot corrupt the source. sort.js:81 does mutate row.cells, but aligned rows get a fresh cells object and untouched rows behave exactly as on master.
  • resolved propagation is consistent. executeProject copies resolved by name (execute.js:582,598) and collect's materialized fast path reads row.resolved[col] (execute/utils.js:75), so a padded column reads undefined on every path, matching its cell.
  • Full suite green at my head: 4093 tests, 4092 pass, 0 fail, 1 skipped. npm run typecheck clean. Smokes gateway_claude_capture, gateway_codex_capture, local_parquet_export all ok.
  • Conventions: no semicolons, no U+2014 anywhere in the files I touched including LLP prose.

Fix pushed

a407168 - "Settle the double-alignment question and record the key-order change (#788)". LLP 0241 Consequences plus one test. No production code touched. Verified present in the pushed tree (git show origin/fix/issue-788:...), and the new test fails when the union call site is reverted.

For triage

item classification
PR body still claims "changes no existing query's answer" and "the only observable difference is Object.keys(row).length" not a production blocker; must be actioned before squash-merge, replacement text above
6x slowdown for SELECT * over a drifted partition (~23ms to ~160ms, 20k rows) preference; measured, unavoidable at the source, no cheaper variant exists, recorded in LLP 0241
lone-partition WHERE on an absent column matches every row not this PR (identical on master); it is PR #787's gate

Nothing else needs a human. The known trivial textual conflict with #787 over withSchemaColumns.scan still stands and my commit does not touch that function.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

Triage: ship. No unresolved findings, no follow-up issue needed.

Triage at head a407168, the same SHA round 2's review marker is on. Verified independently in a fresh worktree with a real npm install: full suite 4092 of 4093 pass (1 skipped), npm run typecheck clean, all 9 regression tests pass, and with only the two alignment call sites reverted 7 of 9 fail (the two passers being the deliberate SELECT * no-change pin and the helper unit test), so the tests pin the real defect.

The 6x slowdown is accepted. SELECT * over a 20k-row drifted partition goes from ~25ms to ~150ms; a full-width partition is unchanged. Round 2 demonstrated the alternative (skipping the wrapper's alignment) reintroduces the silent wrong-answer defect and the SELECT *, <literal> crash, and that the only removable redundancy (the union's pass) measures inside run-to-run noise. The cost only applies to drifted partitions, is proportional to the declared width the caller asked for, and buys row shapes that match the schema QueryResults.columns already promised. Correctness over speed, and the numbers are recorded in LLP 0241 so the question stays settled.

The three behaviour changes are accepted.

  1. Throw-to-answer on WHERE/ORDER BY naming a column some partition lacks: this is compliance with LLP 0015's existing "reads as null, never throws" requirement, and round 2 verified every widened answer against the hinted form of the same query with no throw-to-wrong-answer transition anywhere.
  2. Star key order now follows the advertised list: same values, and it settles a disagreement with what QueryResults.columns already reported.
  3. Object.keys(row).length now equals the declared count: the lost signal (enumerating keys to probe physical columns) was never sound, since the two halves of a drifted union answered it differently for the same query. Recorded in LLP 0241's Consequences.

Findings ledger. Round 1 finding 1 (incomplete Consequences): fixed in 2cd2307. Round 1 finding 2 (per-row rebuild cost): settled as accepted, measured in LLP 0241. Round 2 finding 3 (key-order change undocumented): fixed in a407168. Round 2 finding 5 (PR body carried the disproved "changes no existing query's answer" claim): resolved, the body now acknowledges the earlier claim and states all three measured differences, verified by re-reading it. The lone-partition WHERE mis-match on an absent column is identical on master and head and is the pre-existing bug PR #787 (open) fixes; not this PR's defect. The "not chased here" note (a hypothetical future plugin declaring columns outside a union) describes no current code path and the duty is recorded in LLP 0241, so no follow-up issue is warranted.

Known trivial textual conflict with #787 in withSchemaColumns.scan remains for whoever merges second; neither fix depends on the other.

@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 15, 2026
@philcunliffe
philcunliffe merged commit 192d3f9 into master Aug 17, 2026
9 checks passed
@philcunliffe
philcunliffe deleted the fix/issue-788 branch August 17, 2026 05:35
philcunliffe added a commit that referenced this pull request Aug 18, 2026
… red (#820) (#821)

* Union absent-column tests pin the pre-LLP-0241 contract, so master is 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.

* Scope the union's absent-column agreement to the row path (#820 review)

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>

* ai-gateway's scanColumn null is not what the row path reads (#820 review)

`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>

---------

Co-authored-by: test <test@test.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SELECT *, <col> over a drifted union mis-assigns a value into a neighbouring column

1 participant