Hi Nikita! On Fri, May 15, 2026 at 2:53 PM Nikita Malakhov <[email protected]> wrote: > Again, Alexander and Amit, thanks for the review. I've rebased the patch and > made > some changes according to Alexander's notes. I've slightly rearranged files > between > patches for it to be easier to review, so now there are 3 patches: > v23-0001-json-table-plan-clause.patch - main code changes > v23-0002-json-table-plan-tests.patch - test cases and out file changes > v23-0003-json-table-plan-docs.patch - docs package > > <...> > >1. IsA(planstate, JsonTableSiblingJoin) is wrong. planstate is not a > >node, thus IsA() can't be applied to it. You should instead do > >IsA(planstate->plan, JsonTableSiblingJoin). It wasn't catched, > >because regression tests don't exercise this branch. So, you also > >need to improve the coverage. > - done; > > >2. get_json_table() with patch uses JSON_BEHAVIOR_EMPTY as the default > >value for deparsing, while parsing still uses > >JSON_BEHAVIOR_EMPTY_ARRAY. Looks plain wrong. I'm not sure what is > >intention here. > - looks like leftover from older version, changed to default behavior, so > unnecessary > emission of ERROR is omitted; > > >3. PLAN clause is always emitted during deparsing even if user didn't > >specify anything. I would prefer to skip PLAN clause in this case > >unless there is strong reason to do the opposite (this reason must be > >pointed if any). > - done, this made test cases much more readable; > > >4. Unused typedefs in src/tools/pgindent/typedefs.list: > >JsonTableScanState, JsonPathSpec, JsonTablePlanStateType, > >JsonTableJoinState. > - done; > > >5. Empty comment in JsonTablePlanState definition. Pointed by Amit, > >but not fixed. > - done; > > >6. Rename passingArgs to passing_Args for no reason in parse_jsontable.c. > - done; > > >7. Patch lacks documentation (also pointed by Amit) > - done, documentation is provided in separate patch; > > >8. Patch could use pgindent run. > - not done yet but would provide a newer version with it.
Thank you. I've made following additional changes. 1. Actually, I don't see my previous item 3 fully addressed. We still had PLAN clause deparsed in the case user didn't specify it. I see this as an undesirable behavior. And there are at least two ways to fix this: don't output PLAN clause if it's not the default, remember if user specified as a special flag or even original clause. I found first way (don't output default) more appealing as it don't require additional struct members and we already do this in other similar cases (NULLS FIRST/LAST, ON EMPTY/ON ERROR). So, I did this for PLAN clause. 2. Simplification for JsonTablePlanNextRow(): no reason to check (planstate->advanceNested || planstate->nested) if we already issued break for (planstate->nested == NULL). 3. Similar simplification for transformJsonTableNestedColumns(). ------ Regards, Alexander Korotkov Supabase
v24-0001-JSON_TABLE-PLAN-Clause-1-3.patch
Description: Binary data
v24-0002-JSON_TABLE-PLAN-Clause-2-3.patch
Description: Binary data
v24-0003-JSON_TABLE-PLAN-Clause-3-3.patch
Description: Binary data
