Repository navigation
feat(deparser): support JSON_QUERY, JSON_VALUE, JSON_EXISTS and JSON_TABLE - #369
pyramation wants to merge 1 commit into
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟡 1 medium 💬 Inline comments (1)
This PR extends the deparser with visitors for the JSON expression family:
Reviewed commit: d30d6c2 |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds deparser support for SQL/JSON functions (JSON_QUERY/JSON_VALUE/JSON_EXISTS behaviors and JSON_TABLE columns) plus kitchen-sink fixtures and tests, with a name_location noop in the fixture cleaning script.
Key findings
- 🟡 ON SCALAR STRING silently dropped from JSON quote clauses — deparser.ts:11149
| formatJsonQuotes(quotes: t.JsonQuotes | undefined): string | null { | ||
| switch (quotes) { | ||
| case 'JS_QUOTES_KEEP': | ||
| return 'KEEP QUOTES'; | ||
| case 'JS_QUOTES_OMIT': | ||
| return 'OMIT QUOTES'; | ||
| default: | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
ON SCALAR STRING silently dropped from JSON quote clauses
The quotes field is a bare enum (JS_QUOTES_KEEP/JS_QUOTES_OMIT), so formatJsonQuotes can only emit KEEP QUOTES/OMIT QUOTES and silently drops the ON SCALAR STRING modifier (packages/deparser/src/deparser.ts:11149). Upstream PostgreSQL carries the ON SCALAR STRING flag as a separate boolean on the JSON quotes node and its ruleutils prints it back, but this repo's libpg-query protobuf (JsonQuotes enum in __fixtures__/proto/17-latest.proto:2900) has no such field, so the flag is already lost in the parse tree. JSON_QUERY(x, '$' OMIT QUOTES ON SCALAR STRING) therefore deparses as OMIT QUOTES, which omits quotes unconditionally instead of only for scalar strings — semantically different SQL with no error. The kitchen-sink fixture __fixtures__/kitchen-sink/misc/json-query-functions.sql:12 exercises exactly this clause, and round-trip tests pass only because reparse matches the already-lossy AST.
📋 Prompt for AI Agents
In packages/deparser/src/deparser.ts around lines 11149-11158 (formatJsonQuotes) and its callers JsonFuncExpr (line ~11196) and JsonTableColumn (line ~11245), handle the PostgreSQL ON SCALAR STRING modifier that upstream attaches to the JSON quotes node. First inspect the libpg-query version used at runtime to see whether it exposes a scalar_string/on_scalar field next to the JsonQuotes enum (the fixture protos in fixtures/proto/17-latest.proto lack it); if the runtime parse tree carries it, thread it through the types and emit ON SCALAR STRING when set. If the proto cannot represent the flag, add a test/comment documenting that OMIT QUOTES ON SCALAR STRING round-trips lossily (e.g. in fixtures/kitchen-sink/misc/json-query-functions.sql line 12) instead of silently emitting semantically different SQL.
|
Folded into #371 |
Summary
JsonFuncExprandJsonTablehad no deparser visitor, so any statement using the SQL/JSON query functions threwDeparser does not handle node type: JsonFuncExpr/JsonTable.Adds visitors for
JsonFuncExpr,JsonTable,JsonTableColumn,JsonTablePathSpec,JsonBehaviorandJsonArgument, covering:JSON_EXISTS/JSON_VALUE/JSON_QUERYwithFORMAT JSON [ENCODING ...]context items,PASSING ... AS name,RETURNING type [FORMAT JSON],WITH [CONDITIONAL|UNCONDITIONAL] WRAPPER/WITHOUT WRAPPER,KEEP|OMIT QUOTES, and every... ON EMPTY/... ON ERRORbehavior (includingDEFAULT expr).JSON_TABLEwith a named root path (AS name),PASSING,FOR ORDINALITY/ regular /EXISTS/FORMAT JSONcolumns,NESTED [PATH] ... [AS name] COLUMNS (...)(recursively), table-levelON ERROR,LATERALand aliases with column lists.EXISTScolumns never emit wrapper/quotes clauses: the parser fills inwrapper: JSW_NONEfor them but the grammar rejectsWITHOUT WRAPPERthere.cleanTree(used by the round-trip tests) now also stripsname_location, the only location field not already ignored. Without it anyJSON_TABLE ... AS namepath whose name shifts position on deparse (e.g.jsonb '...'→'...'::jsonb) is reported as an AST mismatch.Tests
New kitchen-sink fixtures
misc/json-query-functions.sqlandmisc/json-table.sql(mostly from the PG18sqljson_queryfuncs/sqljson_jsontableregression tests), regenerated withnpm run kitchen-sink.src/deparser.tschecked out), both generated tests failed withDeparser does not handle node type: JsonFuncExpr/JsonTable.misc-pg18-sqljsonsuite.tscis clean and lint reports 0 errors.This is independent of #368 (no shared hunks).
Link to Devin session: https://app.devin.ai/sessions/cb07c6509db64478af655f52611468f4
Open in Devin Desktop: https://app.devin.ai/desktop/session/cb07c6509db64478af655f52611468f4?variant=devin
Requested by: @pyramation