Re: remaining sql/json patches

On Tue, Nov 21, 2023 at 4:09 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> I looked a bit at the parser additions, because there were some concerns
> expressed that they are quite big.

Thanks Peter.

> It looks like the parser rules were mostly literally copied from the BNF
> in the SQL standard. That's probably a reasonable place to start, but
> now at the end, there is some room for simplification.
>
> Attached are a few patches that apply on top of the 0003 patch. (I
> haven't gotten to 0004 in detail yet.) Some explanations:
>
> 0001-Put-keywords-in-right-order.patch
>
> This is just an unrelated cleanup.
>
> 0002-Remove-js_quotes-union-entry.patch
>
> We usually don't want to put every single node type into the gram.y
> %union. This one can be trivially removed.
>
> 0003-Move-some-code-from-gram.y-to-parse-analysis.patch
>
> Code like this can be postponed to parse analysis, keeping gram.y
> smaller. The error pointer loses a bit of precision, but I think that's
> ok. (There is similar code in your 0004 patch, which could be similarly
> moved.)
>
> 0004-Remove-JsonBehavior-stuff-from-union.patch
>
> Similar to my 0002. This adds a few casts as a result, but that is the
> typical style in gram.y.

Check.

> 0005-Get-rid-of-JsonBehaviorClause.patch
>
> I think this two-level wrapping of the behavior clauses is both
> confusing and overkill. I was trying to just list the on-empty and
> on-error clauses separately in the top-level productions (JSON_VALUE
> etc.), but that led to shift/reduce errors. So the existing rule
> structure is probably ok. But we don't need a separate node type just
> to combine two values and then unpack them again shortly thereafter. So
> I just replaced all this with a list.

OK, a List of two JsonBehavior nodes does sound better in this context
than a whole new parser node.

> 0006-Get-rid-of-JsonCommon.patch
>
> This is an example where the SQL standard BNF is not sensible to apply
> literally. I moved those clauses up directly into their callers, thus
> removing one intermediate levels of rules and also nodes. Also, the
> path name (AS name) stuff is only for JSON_TABLE, so it's not needed in
> this patch. I removed it here, but it would have to be readded in your
> 0004 patch.

OK, done.

> Another thing: In your patch, JSON_EXISTS has a RETURNING clause
> (json_returning_clause_opt), but I don't see that in the standard, and
> also not in the Oracle or Db2 docs. Where did this come from?

TBH, I had no idea till I searched the original SQL/JSON development
thread for a clue and found one at [1]:

===
* Added RETURNING clause to JSON_EXISTS() ("side effect" of
implementation EXISTS PATH columns in JSON_TABLE)
===

So that's talking of EXISTS PATH columns of JSON_TABLE() being able to
have a non-default ("bool") type specified, as follows:

JSON_TABLE(
vals.js::jsonb, 'lax $[*]'
COLUMNS (
exists1 bool EXISTS PATH '$.aaa',
exists2 int EXISTS PATH '$.aaa',

I figured that JSON_EXISTS() doesn't really need a dedicated RETURNING
clause for the above functionality to work.

Attached patch 0004 to fix that; will squash into 0003 before committing.

> With these changes, I think the grammar complexity in your 0003 patch is
> at an acceptable level.

The last line in the chart I sent in the last email now look like this:

17-sqljson 670262 2.57 2640912 1.34

meaning the gram.o text size changes by 2.57% as opposed to 2.97%
before your fixes.

> Similar simplification opportunities exist in
> the 0004 patch, but I haven't worked on that yet. I suggest that you
> focus on getting 0001..0003 committed around this commit fest and then
> deal with 0004 in the next one.

OK, I will keep polishing 0001-0003 with the intent to push it next
week barring objections / damning findings.

I'll also start looking into further improving 0004.

> (Also split up the 0005 patch into the
> pieces that apply to 0003 and 0004, respectively.)

Done.

[1] https://www.postgresql.org/message-id/cf675d1b-47d2-04cd-30f7-c13830341347%40postgrespro.ru

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com