Repository navigation
fix(sqlite): type a scalar subquery's json inside other json as unknown - #52
colinhacks wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Important
json(…) now trusts type_of's built type, and type_of drops the default argument of lag/lead. So json(lag(x, 1, '<other json>') over …) nested in JSON gets a concrete type that its first row contradicts. On main this was unknown. Details inline.
Reviewed changes
The full PR: the SQLite engine's handling of scalar subqueries and json() re-parsing, the sqlite_json oracle case, the rebuilt artifact, the SQLite docs page and the representation test.
- Scalar subquery's JSON is
unknowninside other JSON:carry'sselectarm now returnsMaybeinstead ofYes, andjson_fact'sselectarm becomesany. This matches 3.54.1 dropping the subtype. json()/jsonb()of built JSON is that JSON again: the newAnalyzer::reparsedtypesjson(x)fromtype_of(x)whenever that type is built JSON, keepingx's nullability.built_textnow returns aFact, so a top-leveljson((select …))column is that JSON, parsed, and nullable where the subquery is.- Oracle, docs and representation test:
kept.sqlandprofile.sqlwrap their subqueries injson().dropped.sqladds the bare forms asunknown. The docs say to wrap a scalar subquery injson(…)to nest it. The test accepts either the array or its text for a bare subquery. - Artifact:
packages/sqlite/engine.wasmmatches its sources (tests/fingerprint.test.tspasses).
ℹ️ The driver matrix's builtJson still nests bare scalar subqueries
lab/driver-matrix/lib/harness.ts:313 builds 'scores', (select json_group_array(…)) and 'none', (select json_group_array(…)) without json(). Its check S10j (:897) expects arrays. On any SQLite 3.54 driver, both come back as strings, so S10j fails. The comment above the query says it follows the types tests/representation.test.ts pins, and that test now wraps its profile in json(). CI doesn't run the matrix, so nothing turns red until someone runs it.
Technical details
# Driver matrix builtJson nests bare scalar subqueries
## Affected sites
- lab/driver-matrix/lib/harness.ts:313 — `scores` and `none` are bare scalar subqueries inside `json_object`
- lab/driver-matrix/lib/harness.ts:897 — S10j expects `scores: [...]`, `none: []` on every driver
## Required outcome
- The query nests the subqueries the way the docs now recommend (`json((select …))`), so S10j holds on SQLite 3.53 and 3.54 drivers alike; `RESULTS.md` regenerated if the row changesℹ️ Artifact built alone
The PR body says packages/sqlite/engine.wasm was rebuilt with engine/build.sh sqlite. According to AGENTS.md, a single-artifact build has the right fingerprint but different bytes from a build of all three. Building all three in one run gives the bytes any checkout reproduces.
claude-opus-5-5 ｜ 𝕏
|
Thanks. On the two notes in the review summary:
|
There was a problem hiding this comment.
✅ No new issues found. Both points from the last review are fixed.
Reviewed changes
This review covers the two commits pushed since the last Pullfrog review, aba29f8 and 8301f23.
lag/leadtyped from their default too: with three arguments, the window arm now types the call withsharedoverxandd, skipping a NULL literal default. Built JSON next to a text default becomestext, sojson()of it isunknown. This fixes the nested case the last review flagged and the top-level column that was already wrong onmain. The engine tests and the new oracle statementlag_default.sqlcover the three-argument form at the top level and insidejson_object(… json(…)), andengine.wasmwas rebuilt with it.- Driver matrix's S10j nests through
json():builtJsonnow wraps both scalar subqueries injson(), so S10j holds on SQLite 3.53 and 3.54 drivers. The README row is updated. The Turso 0.4 skip still applies, sincejson(NULL)is still NULL.
claude-opus-5-5 ｜ 𝕏
SQLite 3.54 drops the JSON subtype a scalar subquery kept on 3.53, so its built JSON nests as a string inside another builder on one version and as JSON on the other. json() around the value nests it on both, and now keeps the shape it builds.
8301f23 to
ea73cfc
Compare
|
Closing: #51 landed the scalar-subquery fix and #49 the lag/lead default fix. Checked against main (b480b81): every construct that changes on SQLite 3.54 (coalesce/ifnull/nullif/iif/case/likely over a scalar subquery, a scalar subquery inside json_array/json_group_array/json_group_object, json_extract or -> in a scalar subquery) is already unknown, and json_set/json_insert/json_replace columns are strings, so nothing in this PR is still needed. |
What SQLite 3.54 changed
SQLite marks the JSON its functions build with a value subtype, and a JSON builder nests a value that carries it as JSON rather than as a string. SQLite 3.53.4 kept that subtype through a scalar subquery; SQLite 3.54.1 drops it. So a scalar subquery's JSON passed into another builder nests as JSON on 3.53 and as a string on 3.54, while Reylo typed it as the JSON it builds on both: on 3.54 (Node 26's
node:sqlite, and every driver as it upgrades) the value contradicted its type.tests/representation.test.tsfailed under Node 26.11 onmainfor this reason.Measured with
node:sqlite(SQLite 3.54.1) against better-sqlite3 (SQLite 3.53.4), 66 constructs. Every one that differs reads a scalar subquery:json_object('p', (select json_group_array(1))){"p":"[1]"}{"p":[1]}json_object('p', (select json_array(1))), a correlated one,limit 1,group by,distinct,union all,values, nestedjson_array((select …)),json_group_array((select …)),json_group_object,json_set/json_insert/json_replacecoalesce,ifnull,nullif,iif,case,likelyover a scalar subqueryjson_extractor->json_object('p', json((select json_group_array(1)))){"p":[1]}{"p":[1]}json_object('p', jsonb((select …))), a scalar subquery ofjsonb_arrayjson_object('p', json((select json_array(x) from t where 0))){"p":null}{"p":null}Unchanged between the two:
coalesce,case,iif,ifnull,nullif,likely,COLLATE, unary plus, a cast to text,min/max(scalar, aggregate, with a FILTER) over built JSON directly; a subquery's, WITH query's or view's column,lagandfirst_value(dropped on both); window calls; a sorted scalar subquery (dropped on both); and a scalar subquery returned as a column of its own, whose text Reylo parses by the column's type.What the types are now
coalesce,case,iifand the rest, isunknown, the convention for a value that may or may not keep the marker.json(…)orjsonb(…)of a value that is built JSON, with or without the marker (a scalar subquery, a subquery's or view's column,lag, acoalesceof them), is typed as the JSON it builds, and is nullable where the value is:json()parses the text back into the same JSON on every version. A columnjson((select json_group_array(…)))is now that array, parsed, where it was a string.lag(x, n, d)andlead(x, n, d)are typed from bothxand their defaultd. They had been typed fromxalone, whichjson()now trusts:lag(json_array(id), 1, '{"a":1}')isstring | null, not[number] | null.json(). With bare subqueries it fails on SQLite 3.54.The SQLite page's JSON section now says to wrap a scalar subquery in
json(…)to nest it, and lists the scalar subquery among what loses the marker.Test plan
cargo test -p reylo-engine-core -p reylo-engine-sqlite: pass (new cases for each construct above,json()over subqueries, columns andlag)packages/sqlite/engine.wasmrebuilt from these sources in one run ofengine/build.shcargo run -p reylo-oracle -- --strict-coverageon all 24 SQLite cases: 0 failed (sqlite_jsonblessed, andlag_default.sqladded: the new scalar-subquery members areunknown;kept.sqlandprofile.sqlwrap their subqueries injson()and keep their shapes)nub run typecheck: passnub run testunder Node 26.11 with Postgres 18 and MySQL 8.4 test servers: 209 pass, 0 fail, 8 skipped (labs not installed: connection tests and Jest). The representation test now runs the profile throughjson()on every driver (better-sqlite3,node:sqlite, libSQL, Turso, sqlite3) and checks that a bare subquery's member, typedunknown, is the array or its textrunner.ts --only), on Node 26.11.1 and Bun 1.4.3: S10j passes on every SQLite driver. Turso 0.4.4 skips it as before. With the old, bare query it fails onnode:sqlite26.11.1nub run buildandnub run captureinsite/: pass, no captured type changed