Re: remaining sql/json patches - Mailing list pgsql-hackers

From Peter Eisentraut
Subject Re: remaining sql/json patches
Date
Msg-id 1d211324-4a9c-4129-b2bf-d4b754167aef@eisentraut.org
Whole thread Raw
In response to Re: remaining sql/json patches  (Amit Langote <amitlangote09@gmail.com>)
Responses Re: remaining sql/json patches
List pgsql-hackers
I looked a bit at the parser additions, because there were some concerns 
expressed that they are quite big.

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.

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.

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.

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?

With these changes, I think the grammar complexity in your 0003 patch is 
at an acceptable level.  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.  (Also split up the 0005 patch into the 
pieces that apply to 0003 and 0004, respectively.)

Attachment

pgsql-hackers by date:

Previous
From: Jeff Davis
Date:
Subject: Re: simplehash: SH_OPTIMIZE_REPEAT for optimizing repeated lookups of the same key
Next
From: Laurenz Albe
Date:
Subject: Re: Use of backup_label not noted in log