Repository navigation
fix(postgres): keep an outer-joined row's own fields in to_jsonb - #74
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
ℹ️ No soundness issues found. Two minor points inline: a docs sentence that claims more than the PR does, and a probe-budget ordering issue.
Reviewed changes
I reviewed the full diff: the second "row present" read for an outer-joined to_jsonb(alias), how the resolver uses it, the new oracle statements and the docs line.
row_presentrewrite: each outer join above the alias becomes one that keeps the alias's row. LEFT and RIGHT become inner joins, and FULL becomes the one-sided join that preserves the alias. Joins that have their own alias are not entered. All three rewrites are valid SQL, since FULL's merge-joinable restriction does not apply to LEFT or RIGHT, and they keep the same set of rows where the alias is present. AUSINGmerged column has the same value on those rows.- Resolver: a field is nullable only when both the main probe and the present read say it can be NULL. The object's own nullability still comes from the main probe alone, and Postgres's PlaceHolderVar keeps a pulled-up subquery's whole row
NULLrather thanROW(NULL, …). Engine::shapes: the present read is described over the same text the main probe used (narrow or wider) and is checked to have the same number of columns.- Oracle and docs: two blessed cases have tighter fields, there are ten new
whole_row_json_*statements, and the "Whole rows" sentence inpostgres.mdxchanged. The committedengine.wasmmatches its sources (tests/fingerprint.test.tspasses atc54afea).
claude-opus-5-5 ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found. Both earlier points are fixed.
Reviewed changes
I reviewed the two commits pushed since the last review, 9d77016 and e465783.
- Probe order in
Engine::shapes: the function now describes every main probe (narrow, then wider) first, and only then each present read, so a present read can no longer use up budget that a later main probe needs. Each present read still uses the same text as its main probe: narrow if the narrow probe succeeded, otherwisewider. A probe that is still pending is skipped. Present reads still run before items are followed, just as they did before this change. - Docs: the "Whole rows" sentence in
postgres.mdxnow limits kept field nullability to rows turned into JSON. It no longer claims this for compositearray_agg(p). - Artifact: the committed
packages/postgres/engine.wasmmatches its sources (tests/fingerprint.test.tspasses ate465783).
claude-opus-5-5 ｜ 𝕏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An outer-joined table's whole row turned into JSON (
to_jsonb(p),to_json(p),row_to_json(p),p.*, andjson_agg(p.*)) was typed as a nullable object whose every field was nullable too, the table's ownNOT NULLcolumns included. The probe that types the fields reads them through the outer join, which nulls every column of an unmatched row. But an object that exists is a row that matched, so its fields hold what the row holds.order: { id: number | null; user_id: number | null; note: string | null } | nullorder: { id: number; user_id: number; note: string | null } | nullThe rule
A whole row that an outer join may null is read a second time with that row always present. Each outer join above its alias is changed so it keeps the row: a
LEFT JOINwith the alias on its nullable side becomes an inner join, and aFULL JOINbecomes the one-sided join that preserves the alias's side. Each field is nullable only where that read says it is. The object's own nullability doesn't change. The second read goes through the same analysis as any other statement. So a base table's field takes itsNOT NULL, and a view, derived table or WITH query's field takes that item's own analysis, neverattnotnull. The set of rows where the alias is present is the same under both joins, because an outer join pairs its nullable side's row only with a row whose ON condition held.Oracle
json_subqueries/row_left_join:j: { id: int8?, user_id: int8?, title: text? }?→j: { id: int8, user_id: int8, title: text }?merge_returning/json: aMERGE … RETURNING to_jsonb(s)source row that may be missing:row: { id: int4?, name: text?, label: text? }?→row: { id: int4, name: text, label: text }?inference_precision, each checked over seeded rows:jsonb_agg(p.*)u left join (posts p join users a …))?json?)