Horton

Re: remaining sql/json patches

amit <amitlangote09@gmail.com>

View thread
From: Amit Langote <amitlangote09@gmail.com>
To: Peter Eisentraut <peter@eisentraut.org>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Andres Freund <andres@anarazel.de>, Andrew Dunstan <andrew@dunslane.net>, PostgreSQL-development <pgsql-hackers@postgresql.org>, jian he <jian.universality@gmail.com>, Erik Rijkers <er@xs4all.nl>
Date: 2023-11-22T06:09:36Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. SQL/JSON: Various improvements to SQL/JSON query function docs

  2. SQL/JSON: Fix some obsolete comments.

  3. SQL/JSON: Fix issues with DEFAULT .. ON ERROR / EMPTY

  4. JSON_TABLE: Add support for NESTED paths and columns

  5. Fix JsonExpr deparsing to emit QUOTES and WRAPPER correctly

  6. Fix typo introduced in 6185c9737

  7. Add basic JSON_TABLE() functionality

  8. Avoid splitting errmsg string to span multiple lines

  9. Add SQL/JSON query functions

  10. Implement various jsonpath methods

  11. Add soft error handling to some expression nodes

  12. Adjust populate_record_field() to handle errors softly

  13. Refactor code used by jsonpath executor to fetch variables

  14. Test EXPLAIN (FORMAT JSON) ... XMLTABLE

  15. Simplify productions for FORMAT JSON [ ENCODING name ]

  16. Add trailing commas to enum definitions

  17. doc: add missing <returnvalue> and whitespace

  18. Add more SQL/JSON constructor functions

  19. Rename a nonterminal used in SQL/JSON grammar

  20. Some refactoring to export json(b) conversion functions

  21. Don't include CaseTestExpr in JsonValueExpr.formatted_expr

  22. Code review for commit b6e1157e7d

  23. Pass constructName to transformJsonValueExpr()

  24. Unify JSON categorize type API and export for external use

  25. Make some indentation in gram.y consistent

  26. Allow most keywords to be used as column labels without requiring AS.

  27. Reduce size of backend scanner's tables.

  28. Use perfect hashing, instead of binary search, for keyword lookup.

Attachments

On Tue, Nov 21, 2023 at 4:09 PM Peter Eisentraut <peter@eisentraut.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