Uh oh!
There was an error while loading. Please reload this page.
The union's absent-column contract is undefined-or-throws, not null (#731) - #740
Conversation
…ed or throws (#731) `LLP 0015#multi-partition-union` and the `unionSources` header comment both promised "projecting an absent column reads as null, never throws". The code never did that. Measured on a real two-partition parquet union with additive drift (`extra` only in the newer partition): - a bare identifier projection (`SELECT extra FROM t`, aliased, with a `LIMIT`, or with a predicate on a shared column) yields the key with the value `undefined`, not `null`, so `JSON.stringify` drops it; - anything that evaluates the absent column (`WHERE` on it, an expression or function over it, `ORDER BY` / `GROUP BY` / `DISTINCT`, an aggregate) throws squirreling's `ColumnNotFoundError` at the first row from the partition that lacks it; - `SELECT *` is unaffected: each partition's rows keep their own shape, so the key is simply absent. Correct the doc and the comment to state that contract. Runtime behaviour is untouched: the union throws loudly rather than returning silently-wrong data, and null-padding the union layer is a deliberate behaviour change for the maintainer to make, not a doc repair. Pin both halves with tests over real parquet partitions so the doc and the code cannot drift apart again. Also drop the inline `import('squirreling/src/types.js').ScanColumnResults` type in the same test file (CLAUDE.md forbids it) in favour of the existing top-of-file `@import` block.
…rvived on ai_gateway_messages (#731) The previous pass corrected the observable contract but described a mechanism that does not exist: "copies the value through and yields `undefined`". Nothing copies anything, and the projected cell does throw. `executeProject` takes its copy path only `if (sourceName in row.cells)`. For a partition lacking the column that is false, so it installs a throwing `evaluateExpr` thunk and writes no `resolved[alias]`. Measured on the projected `AsyncRow` for `SELECT extra FROM t` over two real parquet partitions: resolvedHasKey=false cells.extra() -> ColumnNotFoundError resolvedHasKey=false cells.extra() -> ColumnNotFoundError resolvedHasKey=true cells.extra() -> "x" The `undefined` and the present own key come entirely from `collect()`, whose "all rows pre-materialized" fast path builds `item[col] = row.resolved[col]` over `row.columns` and never invokes the cell. So the real seam is *consumer reads `resolved` vs invokes `cells`*, and the fast path is load-bearing: built from partitions that hand-roll rows without `resolved` (a legal `AsyncDataSource`), `SELECT extra FROM t`, `SELECT extra AS e FROM t` and `SELECT extra FROM t LIMIT 1` all throw. The doc's claim holds today only because every in-repo partition goes through squirreling's `asyncRow`. State that seam and its precondition in LLP 0015 and the `unionSources` header, and pin both halves: a test that inspects the unresolved cell directly, and one that removes `resolved` and shows the same shapes throwing. Also: - `withSchemaColumns` in `@hypaware/ai-gateway` still promised the old, false invariant ("a row object that lacks the key simply reads as null") on the dataset a user chasing this symptom actually queries. Correct it to the same contract, and say that normalizing the holes to null is what the `scanColumn` forwarding below is for. - Qualify the plan-failure claim: it is true of a bare `unionSources`, but every production dataset wraps the union in a layer advertising the declared schema (LLP 0032), under which a column no partition has plans fine and gets the same undefined-or-throws treatment. - Note the correction inline, so the Active spec records that it once said otherwise rather than reading as if it always said this. - Add the alias and `LIMIT` shapes the LLP names to the pinned query loop, so "Pinned by" is honest, and lift the duplicated parquet fixture out of `union-source.test.js` / `parquet-source.test.js` into `test/helpers/parquet_source_fixture.js`. No runtime behaviour changes.
philcunliffe
commented
Aug 13, 2026
Review round 1 of The reviewer re-derived the contract on its own fixtures rather than re-running the PR's, probed roughly 40 SQL shapes, and read 1. major - the mechanism was backwards, and it hid a load-bearing precondition. FIXEDBoth texts said a bare identifier projection "copies the value through and yields
The Two consequences the old text denied:
Fixed in both places, now naming the resolved-versus-cells seam and stating the 2. major - the same false invariant survived on the flagship dataset. FIXED
After this PR the repo would have held two comments about the same code path flatly contradicting each other, with the surviving wrong one on 3. minor - the plan-failure claim was false of the shipped path. FIXED"When no partition has it, planning fails with the same error" is true of a bare Fixed with the 4. minor - the LLP edit needed a provenance note. FIXEDLLP 0015 is 5. minor - test helper duplication, and "Pinned by" was not honest. FIXED
Also checked, clean
One nuance from the fix worth recording: typecheck initially failed on the new hand-rolled source because The head has moved to |
…e false invariant survived in three more places (#731) The `undefined` read had two documented preconditions and actually has three. `executeProject` computes, up front and over the whole output column list: const resolveable = plan.columns.every(col => col.type === 'star' || col.type === 'derived' && col.expr.type === 'identifier') and emits no `resolved` map at all when it is false. So one non-identifier sibling column collapses `collect()`'s fast path for the entire result and the drifted thunk is invoked. Measured on the drifted parquet union this PR already builds: SELECT extra FROM t -> extra=UNDEF | UNDEF | "x" SELECT score, extra FROM t -> UNDEF | UNDEF | "x" SELECT *, extra FROM t -> UNDEF | UNDEF | "x" SELECT extra, 1 AS lit FROM t -> THROW ColumnNotFoundError SELECT extra, score * 2 AS d FROM t -> THROW ColumnNotFoundError The old text's dichotomy was "bare identifier projection reads undefined" vs "anything that evaluates the column throws", and everything on the throwing side was an operation on the absent column. `SELECT extra, 1 AS n` evaluates nothing on `extra` and throws, so a maintainer using the paragraph to judge a query got the wrong answer for a common shape. State the third condition in LLP 0015 and the `unionSources` header, and pin both the identifier-sibling shape that stays on the fast path and the two sibling shapes that do not. Also drop the `asyncRow` provenance from the second condition. Nothing checks where `resolved` came from: `collect()` tests `if (!rows[i].resolved)` and stops. A hand-rolled `AsyncDataSource` that never calls `asyncRow` but attaches its own `resolved` keeps the fast path (verified). `asyncRow` is why the condition holds in this repo, not what the condition is. And sweep the false "reads as null" invariant out of the three places round 1 left it: - `message_projector.js`, the comment a developer reads while adding a nullable column. It is a hole, not a null: the projection hands back `undefined`, which `JSON.stringify` drops; only the `scanColumn` path normalizes to null. - LLP 0055's `withSchemaColumns` bullet glossed its nulls as "the same additive schema-drift rule `withSchemaColumns` already applies to row reads", and `dataset.js` `@ref`s straight at it, so a reader following the corrected comment landed on the uncorrected claim. Drop the parenthetical and record the correction; what the decision settled is unchanged. - `ai-gateway-dataset.test.js` asserted `seen[0].git_remote ?? null === null` with the message "absent column reads as null". The `?? null` laundered the hole into the null the doc used to promise. Assert the absent own property. `dataset.js`'s `withSchemaColumns` header overclaimed twice. The `scanColumn` forwarding exists for LLP 0055's streaming aggregates; null normalization is a correctness duty inside it, not its motivation. And the throwing half of LLP 0015 does not reach this dataset: its partitions are icebird-backed, and icebird answers a scan for a column it lacks with a cell resolving to `undefined` and no `resolved` entry, where a parquet-backed partition omits the cell entirely and throws. Measured over two drifted cache partitions, none of `WHERE`, `ORDER BY`, `DISTINCT`, `coalesce`, `upper`, `max` or `count` on `git_remote` throws. No runtime behaviour changes.
philcunliffe
commented
Aug 13, 2026
Review round 2 of 1. major - the |
Keeps both intents in the union's `columns`-forwarding comment: this branch's corrected absent-column contract (undefined-or-throws, with the three `executeProject`/`collect()` conditions and the `resolveable` gate) and #724's clause on the narrowed projection determining what the engine re-filters on. Both test files converge on the shared `test/helpers/parquet_source_fixture.js` rather than keeping master's re-copied inline parquet fixtures. Re-verified the documented contract against the post-merge tree: with #724's `readColumns = hints.columns`, a partition asked for a column it lacks still emits only the columns it has, so a bare projection still reads `undefined` even under a pushed-down WHERE.
philcunliffe
commented
Aug 13, 2026
Triage after the review budget (LLP 0017). Two rounds ran; both cleared. The triage re-derived the whole contract itself and found the icebird-facing text added in round 2's fix is false. This is a doc-accuracy PR, so a false documented claim is its entire defect surface. The blockerThree sentences are demonstrably false at this head:
Measured on a real icebird-backed The cause: squirreling routes any single-column scan through A maintainer acting on the LLP sentence would predict undefined-or-throws for the flagship dataset and observe null-or-null. Why two rounds missed itThe PR's icebird tests pin only the raw What is verified goodThe parquet half is right, and every sentence of it survived independent re-derivation: the The merge is clean: #724's narrowed projection is intact, a parquet partition asked for a column it lacks still emits its rows rather than throwing, #730's NULL-guard tests pass through the shared fixture refactor, and both comment-block edits to The decision neededHow to dispose of the false icebird text before merge.
Neutral is not choosing between these unilaterally. This PR has now described the mechanism wrongly five times - the original claim, and four successive corrections, each of which passed a review round before the next one caught it. Whether the icebird half is worth a fifth attempt or should be descoped is a judgement about how much more effort this document deserves, and that is yours. Option 3, shipping as-is, is rejected on neutral's own standard: for a documentation PR a false documented claim is a production defect, and this one sits on the dataset users actually query. How to unstickReply with a comment on this PR (or push to the branch). Neutral monitors this thread and will re-engage with your guidance on its next tick. |
philcunliffe
commented
Aug 14, 2026
Decision on the triage question: option 2, descope. Revert the icebird-facing additions to neutral wording ("stays addressable; exact value depends on the read path, see LLP 0015") and ship only the parquet-union correction, which is verified true. After five wrong descriptions of the icebird mechanism, this document has earned the smaller scope; the icebird contract can be documented later in its own change, with the executeSql + collect SQL-surface tests on the icebird fixture landing alongside it so the claim is pinned where it was previously untested. |
philcunliffe
commented
Aug 14, 2026
Re-engaging with your guidance - taking option 2, descope: revert the icebird-facing additions to neutral wording ("stays addressable; exact value depends on the read path, see LLP 0015") and ship only the verified parquet-union correction, leaving the icebird contract to a later change that lands its executeSql plus collect SQL-surface tests on the icebird fixture alongside the claim. The stuck label is removed. Neutral re-runs the rung at the current head on its next tick and the worker gets this decision as its input. |
…731) Maintainer decision on the triage question (PR #740): option 2, descope. The icebird-facing text added while fixing review round 2 was measured false, and this document has now described that mechanism wrongly five times, so it is reverted to neutral wording rather than revised a sixth time. Neutralized (the column stays addressable; the exact value depends on the read path, per LLP 0015 "Multi-partition union"): - ai-gateway `dataset.js` `withSchemaColumns` block comment - ai-gateway `message_projector.js` schema comment - the `withSchemaColumns` clause in LLP 0015's union paragraph - the two ai-gateway dataset tests that asserted on that wording LLP 0055 is restored to its accepted text: its corrected-by note made the same icebird claim, and with the icebird contract deferred there is nothing left for this change to say about that decision. Kept intact: the parquet-union correction (LLP 0015, `union-source.js`, `test/core/union-source.test.js`, the shared parquet fixture helper, and the `@import` fix), which independent re-derivation confirmed true. Deferred to a later change, per the maintainer: documenting the icebird contract, and the `executeSql` plus `collect` SQL-surface tests on the icebird fixture that would pin it where it is currently untested. Co-Authored-By: Claude <noreply@anthropic.com>
philcunliffe
commented
Aug 14, 2026
Applied your decision: option 2, descope. Pushed as Only the icebird-facing text is gone. The parquet-union correction, which independent re-derivation confirmed true, ships unchanged. Neutralized to "stays addressable; exact value depends on the read path"
Kept, untouchedThe parquet-union correction in full: the LLP 0015 union paragraph and its corrected-by note, the Deliberately deferred, per your noteDocumenting the icebird contract at all, and the Checks
|
philcunliffe
commented
Aug 14, 2026
Triage of Descope verified at head
Findings ledger
One deferred item, now trackedThe maintainer's decision defers documenting the icebird contract to its own later change, with executeSql plus collect SQL-surface tests on the icebird fixture landing alongside the claim. Nothing tracked that direction, and after five consecutive wrong descriptions of the mechanism it should not live only in a PR thread: #778 now records the deferral, the measured behaviour to re-verify, and the exact test gap ( Non-blocking, so the PR ships. Issue #731's two items are both delivered (option (a) doc correction with pinning tests, and the inline-import fix) and it closes on merge. |
Uh oh!
There was an error while loading. Please reload this page.
… 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>
Corrects a documented invariant that was never true of the code, and pins the real contract with tests. No runtime behaviour changed.
What the docs claimed
LLP 0015#multi-partition-unionand the comment atsrc/core/query/union-source.js:28both said:Neither half was ever true.
What actually happens, measured
Built a fixture of two real parquet partitions unioned by
unionSources, with additive drift: partition A{id, score}, partition B{id, score, extra}. Instrumented the sub-scans to see the options each partition received and therow.columnsit yielded.The issue's headline claim is directionally right but wrong in one important detail: the projected key is not missing. It is present with the value
undefined.JSON.stringifydrops undefined-valued keys, which is how the reporter saw[{}, {extra:"x"}].The boundary is copy-versus-evaluate.
executeProjecttakes a fast path for bare identifier projections: it copiesrow.resolved[name], which isundefinedfor a partition lacking the column, so nothing ever invokes the cell that would throw. Any other use goes throughevaluateExpr, whose identifier lookup missesrow.cellsand throws at the first row from the lacking partition.Mechanically:
parquetDataSourcebuilds each row's advertised columns fromObject.keys(data[0])(what hyparquet actually returned), and hyparquet returns{}for a file lacking a requested column, so the drifted partition's rows never carry anextracell at all. The column is addressable in the first place only becauseunion.columnsadvertises the superset; with no partition having it, planning fails.The choice made
Option (a) from the issue: correct the doc and the comment to describe the real contract. Not option (b), changing the union layer to null-pad.
The reasoning: the code throws loudly rather than returning silently-wrong data, so current behaviour is defensible; correcting a doc that states a false invariant is strictly an improvement regardless of which option is ultimately chosen; and null-padding is a behaviour change a maintainer should make deliberately, not one slipped in under a doc-accuracy issue. If you would rather have (b), this PR does not foreclose it - it makes the current contract explicit, which is what a behaviour change would then have to supersede.
The LLP 0015 edit is an in-place factual repair rather than a superseding doc, since it corrects a statement that was never true of the code rather than changing anything the spec settled. Minimal and surgical; the section is not restructured.
Also fixed
The inline
import('squirreling/src/types.js')type CLAUDE.md forbids. Note it is at line 366 on current master, not the 439 the issue cites - the file has shifted since filing.ScanColumnResultsmoved into the file's existing@importblock.Tests
Three added, two carrying
@ref LLP 0015#multi-partition-union [tests], all over real parquet partitions:undefined, nevernull(asserts own-key present, values,!== null, and the exactJSON.stringifyoutput)ColumnNotFoundError(five shapes: WHERE, WHERE-not-projected,coalesce,ORDER BY, aggregate)SELECT *keeps each partition's row shape, so a drifted key is absent rather than undefinedSuite 3978 pass / 0 fail / 1 pre-existing skip (+3 new); typecheck clean;
llp-ref-hygiene11/11, resolving both new anchors.Fixes#731