Repository navigation
fix(postgres): scope the names json shapes read as postgres does - #53
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
ℹ️ No correctness issues found. One formatting nit is inline.
Reviewed changes
I reviewed all 27 files: the Postgres engine's scoping rules for JSON shapes and expression probes, the settle borrow fix, the oracle's tighter JSON checks and the new json_scoping case.
- WITH scoping in
Items::read: a non-recursive CTE body's item query now carries only the queries before it.arm_textrefuses an arm whose own WITH would hide an outer one, where it used to drop the outer one. Both changes only lose typing or match Postgres, so they stay sound. with_shadows_target: a DML statement whose WITH reuses its unqualified target's name now gets no expression probe and no shapes. This is correct: Postgres never resolves a DML target to a WITH query.- Grouping sets in
jsonshape.rs: under grouping sets, a leaf is now nullable and a whole row reads asUnknown. The exception is the argument of a plain aggregate of the top statement, which reads the rows before grouping. Window calls and nested scalar subqueries take the nullable path, which costs precision but stays sound. default_name/figure: these followFigureColnameInternal's strength rules: a cast's operand name outranks the type's name, and?column?is the fallback. The nodes it leaves out (A_Indirection,NULLIF, the SQL/JSON constructors) are never constructor targets underis_constructor, so nothing depends on them.- WITH queries dropped for a placeholder: a probe that reads one of those names, in its FROM, items or filter, now gets no SQL. Before, it would have read a table of that name.
settleborrow: the code now dropsknownbeforeitem_columnsand takes it again after, so a prefetch can write the relation cache without aRefCell already borrowedpanic. The values computed from the first borrow are still valid afterwards.- Oracle
seed.rs: three things are now contradictions: a top-level JSONnullin a non-null column, a missing object key, and a non-array where an array is typed.
claude-opus-5-5 ｜ 𝕏
|
Added 2bff318 |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
I reviewed the two commits added since the last Pullfrog review: a new grouping-set rule in jsonshape.rs and the re-indent of Builder::aggregate.
- Nulled what a grouping set may group by:
walkis now a thin wrapper overbuild. Under grouping sets, it runs every built shape throughforce_nullable, except inside a plain aggregate's argument at the top level. A built object or array can be a grouping expression itself — by alias (ROLLUP (o)), by position (ROLLUP (1)) or as a nested constructor — so its super-aggregate rows holdNULL. The rule doesn't check which expressions are grouped, soROLLUP (name)also drops the object's non-null (rollup_leafbecomes{ n: text? }?). That loses precision but stays sound, and a plainGROUP BY ostays non-null, as the new unit test asserts. Values inside an aggregate's argument are not forced, becausein_aggregateis set acrossaggregate. The aggregate's own result is still forced once the flag is restored. - Re-indented
aggregate: the FILTER body now sits at the same indentation as the rest ofimpl Builder. This resolves the earlier nit. - Rebuilt the engine artifact: both commits carry a rebuilt
packages/postgres/engine.wasm. Three new oracle statements (rollup_by_alias,rollup_by_ordinal,rollup_nested) cover the new rule.
claude-opus-5-5 ｜ 𝕏
da21187 to
10dff95
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the last Pullfrog review, the branch was rebased onto main after #45. I reviewed how this PR's grouping-set rule merges with #45's grouped probes in jsonshape.rs.
- Renamed this PR's flag to
under_aggregate: #45 addsin_aggregate: usize, which counts the no-FILTER aggregate arguments the walk is inside so thatoutside_aggregates()can choose the probe that carries the GROUP BY. This PR's bool, which exempts a plain top-level aggregate's argument from grouping-set nulling, is nowunder_aggregate. The two flags still mean different things, and each is set and restored around the same calls as before the rebase. - Kept the two rules apart:
groupisSomeonly for a plain GROUP BY (wheregroupedalready excludes grouping sets) or for an ungrouped aggregating statement. A statement with grouping sets therefore never gets a probe that carries a GROUP BY, and the nulling rule alone decides its leaves. - Merged the probe text: the
textclosure inplan_withnow checks the probe's GROUP BY too when it looks for a WITH query dropped for holding a placeholder (reads_relation(&(from, items, filter, group), …)), and builds the SQL with #45'sgrouped_select_text. - Rebuilt the engine artifact: the last commit carries a rebuilt
packages/postgres/engine.wasm. The rebase changed no oracleexpected.txt, and the 23json_scopingstatements read the same as before.
claude-opus-5-5 ｜ 𝕏
|
Rebased onto main past #45, which rewrote the same aggregate and probe code. The merge keeps #45's carried GROUP BY and its |
d9cc88c to
8bb2025
Compare
|
Rebased past #58, which covers two pieces of this PR by its own mechanism; the merge takes #58's for both:
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the last Pullfrog review, the branch was rebased onto main after #58, which now covers part of this PR's probe logic. One new commit re-blesses a statement whose type that change improves.
- Dropped the DML-target rule: #58's
target_named_by_withchecks the same thing as this PR'swith_shadows_target: an unqualified target that a WITH query has the name of. It also runs in bothplan_hereandplan_with, so the duplicate and itscte_nameshelper are gone.dml_target*.sqlread as before. - Dropped the placeholder-WITH refusal: #58 now carries the WITH clause into any probe that reads one of its queries (
reads_with), andEngine::pinpins and renumbers its placeholders. This PR'sdroppedlist,reads_relationand the empty probe they produced are no longer needed. Thetextclosure inplan_withnow callsgrouped_select_textdirectly. No helper or import is left unused (contains_paramis still read indraft). - Re-blessed
param_with_table: the field went from{ n: unknown? }to{ n: int4 }. That is sound::nis a non-nullint4parameter, and the probe now reads the WITH query instead of the tableteam. - Rebuilt the engine artifact:
tests/fingerprint.test.tspasses for all three committedengine.wasmfiles.
ℹ️ The PR description still lists rules the rebase removed
The description's jsonshape.rs bullets still say "A DML statement whose WITH names its target gets no shapes" and "A probe that reads a WITH query left out for holding a placeholder gets no probe". Its table still shows { n: unknown? } for the :n::int row. All three now come from #58 or no longer apply. A squash merge can carry the description into the commit body, so updating it would keep the history accurate.
claude-opus-5-5 ｜ 𝕏
A WITH query's body read the queries after it and its own name, a subquery's own WITH was ignored, a DML target named like a WITH query took its columns, a grouping set's nulls were missed, and a cast was named by its type, so some JSON was typed as what the row does not hold. The oracle now checks a JSON value's kind and keys too.
8bb2025 to
8a52f50
Compare
Several ways Postgres's name scoping was ignored when typing JSON, each of which typed a value as something the row doesn't hold. The new oracle case
json_scopinghas 20 statements; onmain, 12 of them contradict their types against real rows.mainwith a as (… from tally), tally as (select 'one'::text as x) select o from a(a WITH query's body reads a later sibling's name){ n: text }, holds0{ n: int4 }(the table)with snap as (select jsonb_build_object('n', snap.o) as o from snap) select o from snap{ n: { n: { n: … } } }{ n: int4? }(the table)select (with team as (select 1 as name) select jsonb_agg(team.name) from team)[text], holds1jsonwith team as (select 1 as motto) update team set … returning jsonb_build_object('m', motto){ m: int4 }, holds textjsonwith team as (select 'x'::text as motto) update team … returning motto || '!'text, holdsNULLtext?select jsonb_build_object('n', name) from team group by rollup (name){ n: text }, holdsnull{ n: text? }select jsonb_build_object('k', 1)::jsonb, '"s"'::jsonb as jsonb …jsonb: { k: int4 }, holds"s"jsonb_build_object: { k: int4 }, jsonb: jsonwith team as (select :n::int as name) select jsonb_build_object('n', team.name) from team{ n: text }, holds1{ n: unknown? }What changed:
infer.rs: a non-recursive WITH query's statement carries only the queries before it. A recursive WITH keeps the whole clause. An arm or subquery with a WITH of its own is no statement of its own where an outer WITH exists, rather than silently dropping the outer one. A DML statement whose WITH names its target gets no expression probe.jsonshape.rs:unknown.default_namefollows Postgres'sFigureColname(f(…)::jsonbis namedf).postgres.rs:settleheld a borrow of the relation cache acrossitem_columns. Following an item could prefetch a relation and panic (RefCell already borrowed). The scoping change reaches that path inderived_tables, so the borrow is released around the call.oracle/src/seed.rs): a JSON value of the wrong kind for an array, a missing key, and a top-level JSONnullin a non-null column are now contradictions. Main already checked an object's and a list's kind.Tests: engine unit tests for each rule, the new oracle case
json_scoping, and the full oracle with--strict-coverage(101 cases, no expected output changed elsewhere). Gates: engine and oracle tests, the wasm fingerprint, typecheck, package and site. The loader tests fail onlytests/representation.test.ts› "SQLite: JSON a statement builds…", which fails onmainunder Node 26.11's SQLite 3.54.1 and which #51 fixes.