Uh oh!
There was an error while loading. Please reload this page.
A bound written through a CTE or derived table is typed too - #915
Conversation
…IMESTAMP (#915) A relation can expose one column name more than once: `select *` over a join expands both sides, and two of them may declare the same name with different types. The new column list keeps both occurrences, which is right, but two of its readers took the wrong one out. `timestampScope` built a relation's `declared` set as the *union* of its TIMESTAMP names, so a qualified reference to an ambiguous name resolved to TIMESTAMP whichever side declared it. `declaresTimestamp` took the *first* occurrence, so an inner select's own expression resolved by join order. Both type a bound against a column the engine does not hand it. squirreling flattens a CTE's duplicate last-wins, so with c as (select * from ai_gateway_messages m join node n on m.id = n.id) select c.id from c where c.message_created_at >= '2026-08-18' reads `node`'s STRING column and returned the two matching rows before this branch; after it, the literal was cast to a TIMESTAMP and the bound went silently empty on matching data - issue #860's own signature, one join in. Said loudly, `>= 'n4'` on that same string column stopped being a comparison at all and became a TimestampLiteralError. Both readers now collapse a relation's list by name first, keeping a name only when every occurrence of it is a TIMESTAMP, which is the `#complete` doctrine already stated for the list as a whole: never mis-type, and pay only the pruning when unsure. LLP 0280 records the ambiguous-name case under the same anchor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
philcunliffe
commented
Aug 19, 2026
Review round |
`relationKeys` recorded an aliased relation under both its table name and its alias, so `timestampScope` wrote the same relation into `byRelation` twice. A second relation whose table name equals the first one's alias then overwrote that entry, and a qualified bound was typed from the wrong relation's columns: select m.id from node ai_gateway_messages join ai_gateway_messages m on m.id = ai_gateway_messages.id where ai_gateway_messages.message_created_at >= '2026-08-18' The qualifier names `node`, whose `message_created_at` is a STRING, but the literal was cast to a TIMESTAMP from the joined dataset. The query returned no rows where the same query without the rewrite returns two, and the same shape with a bound that is not a timestamp at all (`>= 'n4'`) was refused outright with TimestampLiteralError instead of running as the ordinary string comparison it is. Both are issue #860's own failure produced by its fix. SQL hides the base name once a relation is aliased, and squirreling agrees: it keys every relation by `alias ?? table` and rejects `ai_gateway_messages.id` under `from ai_gateway_messages m` outright ("Table not found ... Available tables: m"). So the base name under an alias is not a second spelling of the same relation and typing through it can only ever type a different one. `relationKeys` now yields the one name a qualifier can reach the relation by, which is what `byRelation`, the `bound` stop for the correlated walk, and `inferColumns`' star and prefix lookups all wanted. The test that pinned the old behaviour asserted only on the rewritten AST, never on rows, for a query the engine refuses to run; it now pins the two spellings that are real (unaliased by name, aliased by alias), and a new test covers the shadowing shape end to end in both directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
philcunliffe
commented
Aug 19, 2026
Neutral review recordReviewed: The design is sound. The "complete list or nothing" rule and the 1. medium - FIXED - |
without coerceTimestampLiterals | with it (pre-fix) | |
|---|---|---|
>= '2026-08-18' | [{id:1}] | [] |
>= 'n4' | rows | TimestampLiteralError |
The qualifier names node, a STRING column, yet the literal was cast to a
TIMESTAMP from the joined dataset: issue #860's own silent-empty signature,
produced by its fix, plus a hard error on an ordinary string comparison.
SQL hides the base name once a relation is aliased, and squirreling agrees. It
keys every relation by alias ?? table (plan/columns.js:36,75,163) and
rejects the base name outright under an alias:
Table "ai_gateway_messages" not found in "ai_gateway_messages.message_created_at". Available tables: m
So the base name under an alias is not a second spelling of the same relation,
and typing through it can only ever type a different one. relationKeys now
yields the one name a qualifier can reach the relation by. Nothing real is
lost: every query the old key made typeable is one the engine refuses to run.
Verified landed:git show HEAD:src/core/query/timestamp-literals.js hasconst key = relation.alias ?? relation.table; the new testan alias that shadows another relation table name keeps its own columns
fails on e0a110e4 (expected 'literal', actual 'cast') and passes on3dbb467a. The test that pinned the old behaviour
(a qualified reference to the base table (by alias or name) is still typed)
asserted only on the rewritten AST, never on rows, for a query the engine
refuses to run; it now pins the two spellings that are real.
2. low - NOT FIXED (settled by LLP 0280) - src/core/query/timestamp-literals.js:310 (inferColumns)
inferColumns bails out on the whole relation when an output column has
neither an alias nor a bare identifier expression. squirreling does derive a
name for those, via its exported derivedAlias (expression/alias.js,
re-exported from squirreling): count(*) -> count_all. So
with c as (select message_created_at, count(*) from ai_gateway_messages group by message_created_at)
select*from c where message_created_at >='2026-08-18'still returns [] on matching data with exit 0, while adding as n to thecount(*) makes it work. Confirmed by direct execution.
Left as-is deliberately. LLP 0280 #complete settles exactly this ("any output
column cannot be given a name (an unaliased expression) ... the walk returns
nothing"), #not-settled-inference names it as out of scope and asks for a
separate design call made against evidence, and the doc is Accepted. It also
fails in the safe direction: no wrong rows, only lost typing and lost pruning,
which is strictly better than the pre-#860 behaviour. Widening it belongs in a
new LLP, not in a review fix that edits a settled decision. Worth noting for
that document: #not-settled-inference's phrase "an output column that has no
name" is imprecise for this engine, which always derives one - the real rule is
"an unaliased expression column".
Checked and cleared
Rewrite-before-infer ordering in the with branch; recursion cost
(O(depth x subtree), not exponential); compound positional pairing against
squirreling's actual select * flattening; BETWEEN (desugars to two
comparisons, so it is covered - and pinned by a test); childExprs coverage
against the full ExprNode union; bound parity for subquery and
table-function joins; column-name and dataset-name case sensitivity; passing
the mutated Statement to executeSql (accepted, execute/execute.js:31);TRY_CAST typing; and scaffold.js's schema: [...] -> schema: { columns: [...] }
correction, which is a real bug fix against DatasetSchema
(src/core/registry/datasets.js:14).
One gap observed and judged not worth acting on: rewriteStatement never walks
a compound statement's own orderBy (SetOperationStatement.orderBy), so a
comparison written in a UNION-level ORDER BY is not typed. It fails safe
(no coercion) and the shape is vanishingly rare.
Checks
npm test 4527 pass / 0 fail / 1 skipped. npm run typecheck clean. Advisory;
CI is authoritative.
philcunliffe
commented
Aug 19, 2026
Neutral triage recordTriaged: head Both actionable review findings (round 1 high: duplicated-name typing; round 2 medium: alias/base-name relation keys) are verified fixed at this head, each with a failing-then-passing regression test. The remaining residuals all fail toward "no coercion", which is the pre-#860 status quo: a missed pushdown, never a wrong row and never a wrong coercion. Verified against the diff ( Deferred findings are enumerated with file:line evidence in follow-up issue #936:
Not merged, not marked ready - a human decides that. |
LLP 0272 typed a string literal from the dataset schema of the relation it was compared against, and named the relations the registry cannot name as an open residual. That residual is issue #860's own failure one relation further in: `with c as (select * from ai_gateway_messages) select id from c where message_created_at >= '...'` compared a Date to a string and returned zero rows on matching data, exit 0, while the same bound written `TIMESTAMP '...'` returned the rows. A relation now supplies an ordered `{ name, isTimestamp }` list whatever kind of relation it is: a dataset from its schema, a CTE from the list its body was proved to expose, a derived table walked in place. Everything LLP 0272 built on a dataset schema (`agreed`, `byRelation`, the outward walk, the type-carrying calls) works over those unchanged. The list is complete or it is nothing, so an inner select the walk cannot read end to end contributes no types rather than a partial list a later name could be mis-typed against. The second residual LLP 0272 named, an unqualified correlated reference, is not a coercion defect: squirreling does not resolve an unqualified name into an enclosing select at all, and raises identically whether the literal is bare or typed. The suite pins that rejection in both spellings rather than fixing what does not fail. LLP 0280 records the decision and extends LLP 0272. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…IMESTAMP (#915) A relation can expose one column name more than once: `select *` over a join expands both sides, and two of them may declare the same name with different types. The new column list keeps both occurrences, which is right, but two of its readers took the wrong one out. `timestampScope` built a relation's `declared` set as the *union* of its TIMESTAMP names, so a qualified reference to an ambiguous name resolved to TIMESTAMP whichever side declared it. `declaresTimestamp` took the *first* occurrence, so an inner select's own expression resolved by join order. Both type a bound against a column the engine does not hand it. squirreling flattens a CTE's duplicate last-wins, so with c as (select * from ai_gateway_messages m join node n on m.id = n.id) select c.id from c where c.message_created_at >= '2026-08-18' reads `node`'s STRING column and returned the two matching rows before this branch; after it, the literal was cast to a TIMESTAMP and the bound went silently empty on matching data - issue #860's own signature, one join in. Said loudly, `>= 'n4'` on that same string column stopped being a comparison at all and became a TimestampLiteralError. Both readers now collapse a relation's list by name first, keeping a name only when every occurrence of it is a TIMESTAMP, which is the `#complete` doctrine already stated for the list as a whole: never mis-type, and pay only the pruning when unsure. LLP 0280 records the ambiguous-name case under the same anchor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`relationKeys` recorded an aliased relation under both its table name and its alias, so `timestampScope` wrote the same relation into `byRelation` twice. A second relation whose table name equals the first one's alias then overwrote that entry, and a qualified bound was typed from the wrong relation's columns: select m.id from node ai_gateway_messages join ai_gateway_messages m on m.id = ai_gateway_messages.id where ai_gateway_messages.message_created_at >= '2026-08-18' The qualifier names `node`, whose `message_created_at` is a STRING, but the literal was cast to a TIMESTAMP from the joined dataset. The query returned no rows where the same query without the rewrite returns two, and the same shape with a bound that is not a timestamp at all (`>= 'n4'`) was refused outright with TimestampLiteralError instead of running as the ordinary string comparison it is. Both are issue #860's own failure produced by its fix. SQL hides the base name once a relation is aliased, and squirreling agrees: it keys every relation by `alias ?? table` and rejects `ai_gateway_messages.id` under `from ai_gateway_messages m` outright ("Table not found ... Available tables: m"). So the base name under an alias is not a second spelling of the same relation and typing through it can only ever type a different one. `relationKeys` now yields the one name a qualifier can reach the relation by, which is what `byRelation`, the `bound` stop for the correlated walk, and `inferColumns`' star and prefix lookups all wanted. The test that pinned the old behaviour asserted only on the rewritten AST, never on rows, for a query the engine refuses to run; it now pins the two spellings that are real (unaliased by name, aliased by alias), and a new test covers the shadowing shape end to end in both directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…put column (#936) The compound ORDER BY walk typed only an unqualified name, on the reasoning that the compound binds no relation and so has no qualifier to resolve. The engine disagrees: squirreling evaluates that clause against the combined output row, whose column names carry no qualifier, and its identifier lookup falls through to the bare name. So `m.message_created_at >= '...'` read the same output column the unqualified spelling reads, kept comparing a Date to a string, and came back in the wrong order with exit 0 - the exact failure this change set exists to end, one character away from the spelling it fixed. An uncoercible literal in that spelling was swallowed too, where LLP 0272 #refuse-uncoercible calls for an error. A qualifier written in the clause is now registered against the compound's output columns, with the two shapes that reach a different value first held back: one an enclosing select binds is correlated, resolved by the ordinary outward walk (so `outer` is threaded in rather than dropped), and one an output column name is or ends with is struct field access, recorded in `bound` so the outward walk stops rather than borrowing an enclosing relation of the same name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3dbb467 to
de82e2eComparephilcunliffe
commented
Aug 19, 2026
Consolidation update: this branch is now rebuilt directly on current master with only #915's three own commits plus #944's two reviewed commits. The old #868 stack and unrelated merge-history commits are gone. Focused query tests (37/37), typecheck, LLP reference hygiene, and GitHub Node 22/24 CI are green. #944 is fully contained here and will be closed after this merges. |
Uh oh!
There was an error while loading. Please reload this page.
Closes the deferred review residuals from PR #868, recorded in LLP 0272 under
#not-settled-derived.Finding 1: a string bound through a CTE or derived table is not coerced
Gate met: behavioural (failing-then-passing regression test).
What was wrong. LLP 0272 fixed a bound written directly on a dataset column. A bound written one relation further in still returned zero rows on matching data, exit 0 - issue #860's exact signature. Reproduced against the four-row parquet fixture already in
test/core/query-timestamp-literals.test.js; all ten cases returned[]:Each returns rows the moment the literal is written
TIMESTAMP '...', which is the tell: the plan is fine, only the literal's type is wrong.Root cause.
timestampScopebuilt itsagreed/byRelationsets only from relationsregistry.getDataset(table)could answer for. A CTE or derived table landed inboundalone (src/core/query/timestamp-literals.js:221on the old head), so no name in that scope resolved to a TIMESTAMP and no literal was rewritten. Downstream, squirreling compares aDateto a string (false for every row) and icebird's converter prunes every row group, so both halves agree on the same wrong answer.Fix. Every relation now resolves to the same currency, an ordered
{ name, isTimestamp }[]:Everything LLP 0272 built on a dataset schema (
agreed,byRelation, the outward correlated walk,TYPE_PRESERVING_ARGS) works over those unchanged. The carried type is the inner expression's, never the outer name's:WITH c AS (SELECT id, date AS message_created_at FROM m)exposes aSTRINGand keeps its string comparison, because typing that from the dataset that shares the name is the same silently-empty answer pointed the other way.The list is complete or it is nothing. If any inner relation cannot be read, or any output column has no derivable name, the walk returns nothing and the relation is treated exactly as LLP 0272 treated every CTE. A partial list would be worse than none:
agreedis an intersection, so a name a partial list omitted could be typed from an unrelated relation that declares it - a wrong coercion, wrong rows.Regression test.
test/core/query-timestamp-literals.test.js, three new tests running end to end throughexecuteQuerySqlover a real parquet partition (the same pushdown-then-scan pathhyp query sqlruns):a string bound on a CTE or derived table column selects the rows it names- before: fails, all 10 cases return[]. After: passes.a CTE that renames another type onto a TIMESTAMP column name keeps its string comparison- passes before and after; it is the guard against over-coercing.an inner select that cannot be read completely types nothing- the declared boundary (unregistered inner table, unnameable output column, table function); passes before and after.Finding 2: an unqualified correlated reference is not typed
Gate met: behavioural, and the reproduction disproves the finding. No fix, by evidence.
The residual assumed the same silent-empty-result failure. It is not one. squirreling does not resolve an unqualified name into an enclosing select at all:
It raises before any comparison is evaluated, and raises identically whether the literal is typed or bare. There is no zero-row answer to fix and typing the literal could not change the outcome; it would only trade one error for another. Closing this needs an engine that can evaluate the resolved reference, which is upstream of this repo.
Pinned rather than fixed, so a future squirreling that does resolve it fails this suite instead of passing silently:
an unqualified correlated reference is rejected by the engine, typed or bare. The machinery a real fix would need (the inner relations' full column lists) is exactly what finding 1 now computes.Finding 3: calls that do not carry an argument's type
Deferred, unchanged - it is not a defect. The issue lists it "only so the boundary is visible".
epoch,extract,date_diff,castand the string functions are absent fromTYPE_PRESERVING_ARGSon purpose: typing a literal from a value-transforming call returns wrong rows rather than none, which is worse than what it prevents. LLP 0272 settled this and the existing testa call that does not carry the column type leaves the literal alonealready pins it. No change.Design record
New LLP 0280 (
llp/0280-inner-select-columns-type-a-bound.decision.md) records the decision, its#completeboundary, and why the unqualified correlated reference is not this defect. LLP 0272 is Accepted and settled, so it is not edited: it gains only anExtended-by: LLP 0280forward-ref on its#not-settled-derivedsection, which the LLP rules allow as editorial.@refannotations point atLLP 0280#carry,#complete,#unqualified-correlated, andtest/core/llp-ref-hygiene.test.jsresolves all of them.Checks
npm test- 4507 pass, 0 fail, 1 skipped (4506 pass before this change, plus the new cases). Every LLP 0272 test still passes untouched, including the three that guard against over-coercion through a CTE or derived table.npm run typecheck- clean.npm run smoke -- cache_roundtrip- ok.local_only_query_withholdfails identically on79f1c98cwith this change stashed, so it is pre-existing and unrelated.Residual risk
USING/natural-join column merging, table-function outputs, or unaliased expression columns. Each gives up (no coercion), which is the pre-existing behaviour, never a wrong type. Named in LLP 0280#not-settled-inference.Fixes#906