SQL/JSON json_table plan clause

On 2024-12-17 Tu 10:11 AM, Nikita Malakhov wrote:

Hi hackers!

This thread is further work continued in [1], where Amit Langote
suggested starting discussion on the remaining SQL/JSON feature
'PLAN clause for JSON_TABLE' anew.

We'd like to help with merging SQL/JSON patches into vanilla,
and have adapted PLAN clause to recent changes in JSON_TABLE
function.

While doing this we've found that some tests with the PLAN clause
were incorrect, along with JSON_TABLE behavior with this clause.
We've corrected this behavior, but these corrections required reverting
some removed and heavily refactored code, so we'd be glad for review
and feedback on this patch.

Also, we've noticed that Oracle set the PLAN clause for JSON_TABLE
aside for some reason.

I noticed that several months ago, and I was somewhat surprised. I'm not
sure anyone else has it either. But that doesn't mean we shouldn't have
it. It's in the standard and seems useful, so I'm very glad to see this
effort.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, failed
Spec compliant: not tested
Documentation: not tested

Hi!
Proposed patch tested at Macosx, compiled and build without warnings.
All tests passed without errors.
JSON_TABLE clause PLAN tested, works as intended.

+1 to commit.

Hi hackers!

Patch has been rebased onto the current master.

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

Nikita Malakhov писал(а) 2025-02-03 02:43:

Hi hackers!

Patch has been rebased onto the current master.

--

Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

Hi!

Tested on Mac with 'make check', all tests passed OK.

--
Best regards,

Vladlen Popolitov.

Hi Nikita,

On Wed, Dec 18, 2024 at 12:11 AM Nikita Malakhov <hukutoc@gmail.com> wrote:

Hi hackers!

This thread is further work continued in [1], where Amit Langote
suggested starting discussion on the remaining SQL/JSON feature
'PLAN clause for JSON_TABLE' anew.

We'd like to help with merging SQL/JSON patches into vanilla,
and have adapted PLAN clause to recent changes in JSON_TABLE
function.

Thanks for working on this.

While doing this we've found that some tests with the PLAN clause
were incorrect, along with JSON_TABLE behavior with this clause.
We've corrected this behavior, but these corrections required reverting
some removed and heavily refactored code, so we'd be glad for review
and feedback on this patch.

Sorry, I don't fully understand this paragraph. Do you mean that
there might be bugs in the existing JSON_TABLE() functionality that
was committed into v17?

--
Thanks, Amit Langote

Hi Amit!

No, JSON_TABLE is ok. I've meant that in the previous approach to PLAN
clause
implementation there were some wrong tests.

On Tue, Feb 4, 2025 at 6:05 AM Amit Langote <amitlangote09@gmail.com> wrote:

Hi Nikita,

On Wed, Dec 18, 2024 at 12:11 AM Nikita Malakhov <hukutoc@gmail.com>
wrote:

Hi hackers!

This thread is further work continued in [1], where Amit Langote
suggested starting discussion on the remaining SQL/JSON feature
'PLAN clause for JSON_TABLE' anew.

We'd like to help with merging SQL/JSON patches into vanilla,
and have adapted PLAN clause to recent changes in JSON_TABLE
function.

Thanks for working on this.

While doing this we've found that some tests with the PLAN clause
were incorrect, along with JSON_TABLE behavior with this clause.
We've corrected this behavior, but these corrections required reverting
some removed and heavily refactored code, so we'd be glad for review
and feedback on this patch.

Sorry, I don't fully understand this paragraph. Do you mean that
there might be bugs in the existing JSON_TABLE() functionality that
was committed into v17?

--
Thanks, Amit Langote

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

Hi Nikita,

I was looking at the patch you posted on Feb 3. Two high-level comments:

* The documentation additions are missing.

* It looks like the patch mixes refactoring changes with new
functionality, which makes it harder to follow what's existing vs.
new. Could you separate these changes so the new additions are easier
to review?

More specifically on the 2nd point:

-/*
- * Check if the type is "composite" for the purpose of checking whether to use
- * JSON_VALUE() or JSON_QUERY() for a given JsonTableColumn.
- */
-static bool
-isCompositeType(Oid typid)
-{
- char typtype = get_typtype(typid);
-
- return typid == JSONOID ||
- typid == JSONBOID ||
- typid == RECORDOID ||
- type_is_array(typid) ||
- typtype == TYPTYPE_COMPOSITE ||
- /* domain over one of the above? */
- (typtype == TYPTYPE_DOMAIN &&
- isCompositeType(getBaseType(typid)));
}
...

Diffs like this one make me wonder whether this code actually needs to
change for PLAN clause support -- but I suspect it does not.

+
+    /**/
+    bool        cross;
+    bool        outerJoin;
+    bool        advanceNested;
+    bool        advanceRight;
+    bool        reset;

Incomplete comment.

@@ -4124,6 +4130,7 @@ JsonTableInitOpaque(TableFuncScanState *state, int natts)
* Evaluate JSON_TABLE() PASSING arguments to be passed to the jsonpath
* executor via JsonPathVariables.
*/
+
if (state->passingvalexprs)
@@ -4155,15 +4162,13 @@ JsonTableInitOpaque(TableFuncScanState *state,
int natts)
}

     cxt->colplanstates = palloc(sizeof(JsonTablePlanState *) *
-                                list_length(tf->colvalexprs));
-
+                               list_length(tf->colvalexprs));
     /*
      * Initialize plan for the root path and, recursively, also any child
      * plans that compute the NESTED paths.
      */
     cxt->rootplanstate = JsonTableInitPlan(cxt, rootplan, NULL, args,
-                                           CurrentMemoryContext);
-
+                                       CurrentMemoryContext);
     state->opaque = cxt;
 }

@@ -4201,8 +4206,9 @@ JsonTableInitPlan(JsonTableExecContext *cxt,
JsonTablePlan *plan,
if (IsA(plan, JsonTablePathScan))
{
JsonTablePathScan *scan = (JsonTablePathScan *) plan;
- int i;
+ int i;

Unnecessary whitespace addition/removal.

 /*
- * Transform JSON_TABLE column definition into a JsonFuncExpr
- * This turns:
+ * Transform JSON_TABLE column

Not sure why the comment was rewritten.

-JsonTableResetRowPattern(JsonTablePlanState *planstate, Datum item)
+JsonTableResetContextItem(JsonTablePlanState *planstate, Datum item)

I remember renaming ContextItem with RowPattern, but it seems this is
switching it back.

It makes it look like you're reverting to code from the old patch that
implemented the PLAN clause together with JSON_TABLE(), instead of
building on the committed code and adding just what's needed to
support PLAN clause in JsonTablePlan. Especially, the changes to
parse_jsontable.c and jsonpath_exec.c,

Hi Amit!

Thank you for your feedback, I'll check it out in a couple of days and
reply.

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

Hi hackers!

Thanks to Amit for reviewing the patch. I've reworked it according to Amit's
recommendations and divided it into 2 parts: part 1 contains PLAN clause
code and part 2 tests. Reworked code is much more compact, so I hope
it would not be so painful to review.

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

Hi hackers!

Patch was tested with the current master. All is good with minor changes in
messages.
The 1st part contains grammar changes with JSON_TABLE transformations
according
to given PLAN strategy, the 2nd contains corrected tests for JSON_TABLE and
new tests
for PLAN clause.

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

Hi hackers!

I've rebased patch set onto current master and have corrected
output according to recent changes in JSON array processing.
Awaiting review.

Thanks!

--
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/