Re: SQL/JSON revisited

On 2023-03-05 Su 22:18, Amit Langote wrote:
> On Tue, Feb 28, 2023 at 8:36 PM Amit Langote <amitlangote09@gmail.com> wrote:
>> On Mon, Feb 27, 2023 at 4:45 PM Amit Langote <amitlangote09@gmail.com> wrote:
>>> On Tue, Feb 21, 2023 at 2:25 AM Andres Freund <andres@anarazel.de> wrote:
>>>> Evaluating N expressions for a json table isn't a good approach, both memory
>>>> and CPU efficiency wise.
>>> Are you referring to JsonTableInitOpaque() initializing all these
>>> sub-expressions of JsonTableParent, especially colvalexprs, using N
>>> *independent* ExprStates?  That could perhaps be made to work by
>>> making JsonTableParent be an expression recognized by execExpr.c, so
>>> that a single ExprState can store the steps for all its
>>> sub-expressions, much like JsonExpr is.  I'll give that a try, though
>>> I wonder if the semantics of making this work in a single
>>> ExecEvalExpr() call will mismatch that of the current way, because
>>> different sub-expressions are currently evaluated under different APIs
>>> of TableFuncRoutine.
>> Hmm, the idea to turn JSON_TABLE into a single expression turned out
>> to be a non-starter after all, because, unlike JsonExpr, it can
>> produce multiple values.  So there must be an ExprState for computing
>> each column of its output rows.  As I mentioned in my other reply,
>> TableFuncScanState has a list of ExprStates anyway for
>> TableFunc.colexprs.  What we could do is move the ExprStates of
>> TableFunc.colvalexprs into TableFuncScanState instead of making that
>> part of the JSON_TABLE opaque state, as I've done in the attached
>> updated patch.
> Here's another version in which I've also moved the ExprStates of
> PASSING args into TableFuncScanState instead of keeping them in
> JSON_TABLE opaque state.  That means all the subsidiary ExprStates of
> TableFuncScanState are now initialized only once during
> ExecInitTableFuncScan().  Previously, JSON_TABLE related ones would be
> initialized on every call of JsonTableInitOpaque().
>
> I've also done some cosmetic changes such as renaming the
> JsonTableContext to JsonTableParseContext in parse_jsontable.c and to
> JsonTableExecContext in jsonpath_exec.c.
>
>

Hi, I have just spent some time going through the first five patches 
(i.e. those that precede the JSONTABLE patches) and Andres's comments in

<https://postgr.es/m/20230220172456.q3oshnvfk3wyhm5l@awork3.anarazel.de>


AFAICT there are only two possible matters of concern that remain, both 
regarding the grammar.


First is this general complaint:


> This stuff adds quite a bit of complexity to the parser. Do we realy need like
> a dozen new rules here?

I mentioned that more than a year ago, I think, without anybody taking 
the matter up, so I didn't pursue it. I guess I should have.

There are probably some fairly easy opportunities to reduce the number 
of non-terminals introduced here (e.g. I think json_aggregate_func could 
possibly be expanded in place without introducing 
json_object_aggregate_constructor and json_array_aggregate_constructor). 
I'm going to make an attempt at that, at least to pick some low hanging 
fruit. But in the end I think we are going to be left with a significant 
expansion of the grammar rules, more or less forced on us by the way the 
SQL Standards Committee rather profligately invents new ways of 
contorting the grammar.

Second is this complaint:


> +json_behavior_empty_array:
> +            EMPTY_P ARRAY    { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
> +            /* non-standard, for Oracle compatibility only */
> +            | EMPTY_P        { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
> +        ;
> Do we really want to add random oracle compat crud here?
>

I think this case is pretty harmless, and it literally involves one line 
of code, so I'm inclined to leave it.

These both seem like things not worth holding up progress for, and I 
think it would be good to get these patches committed as soon as 
possible. My intention is to commit them (after some grammar 
adjustments) plus their documentation in the next few days. That would 
leave the JSONTABLE patches still to go. They are substantial, but a far 
more manageable chunk of work for some committer (not me) once we get 
this foundational piece in.


cheers


andrew

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