Re: [DEPARSE] Pretty-printed sublink test expressions lose needed parentheses

Hi Tom, Sho,

Thank you, Sho, for testing v1.

> Good catch. I do not like your proposed fix though: it adds yet more
> complexity that has to be kept in sync with gram.y and yet doesn't
> line up nicely with the grammar's rules. I think what we ought to
> do for now is your "simple fix"
>
>> 0002 prints the test expression with get_rule_expr_paren(),
>> as for the operands of any other operator. For a simple
>> fix, that alone would do.
>
> without the other stuff.

Agreed. The attached v2 keeps only the get_rule_expr_paren()
change. The tests are cut down to one case for each form that needs
parentheses, plus test expressions that need none and keep none.
Some test expressions that need no parentheses do get them now, as
in ((c + d) IN (SELECT 3)), which reads back as the same expression.
The full regression suite passes on master.

I also checked that the simple fix does not miss a place that needs
parentheses. Under a sublink, isSimpleNode() leaves them out only
for nodes printed as:

- a single word: c, $1, 42
- a function-like call: lower(t), COALESCE(c, 0)
- CASE ... END
- a field selection: (r).f
- a cast: c::text
- a postfix IS DOCUMENT: x IS DOCUMENT

All of these are closed on the right, so the IN or op ANY that follows
cannot bind into them; everything else gets parentheses. IS DOCUMENT
is closed on the right but not on the left, so it loses its own
parentheses when it is the right operand of another operator; that
is handled in CF 7451.

The code change applies cleanly to REL_15 through REL_19, and I built
each of them and ran the regression tests. Only the parentheses in
pretty output change, so I see no problem with back-patching.

> In the longer run, I'm interested in investigating a more invasive
> improvement such as you mentioned:
>
>> Longer term, it might be better to rework the parenthesization in
>> ruleutils.c around one table of the gram.y precedence levels for all
>> operators and expressions, instead of the special cases in
>> isSimpleNode().
>
> because otherwise we'll just keep having bugs of this sort.
> (I'm already wondering whether we correctly deparse all the random
> SQL/JSON syntax, for example.) I wonder if there is any way to
> get the necessary data built automatically from gram.y ...

I think so too. I became interested in deparse while resolving a
conflict between the existing deparse logic and the rule that a row
pattern recognition DEFINE clause cannot use qualifiers.

The ones of the same kind already posted are:

- IS DOCUMENT as an operand (CF 7451)
- operators on ROW() constructors (CF 7452)

On the SQL/JSON side you mention, and in XML, there are these:

- IS JSON in a JSON_OBJECT() value takes over WITH UNIQUE KEYS
- COLLATE on an XMLNAMESPACES URI loses its parentheses
- JSON_TABLE and XMLTABLE lose their alias and column aliases

Counting these, I have found about 30 candidates in deparse and am
going through them; some look as if they have no solution at all. I
will work through them one by one as time allows and post each once
I find a fix. Any that I still cannot find a fix for after a while, I
will report as they are, so that the community can help.

Looking at the candidates together, ruleutils.c is well divided along
node types: each node's printing lives in one place. What is spread
out is the cross-cutting part -- name selection, parentheses, lexical
details such as quoting, type labels and the resolution of operator
and function names -- which each printer and helper decides on its
own. One place that does not know a new node or syntax is then enough
to produce wrong text even when the others are right. Most of the
candidates fall on that side.

Names are the largest group, and the one most often giving silently
wrong results. ruleutils.c already has infrastructure for choosing
them, but names added later, such as RETURNING's OLD/NEW aliases,
EXCLUDED or a JOIN USING alias, are not all registered with it; there
the work is to make every kind of name go through it.

Parentheses, on the other hand, have no shared infrastructure at all.
What gram.y can give them directly is the precedence table. I have
a vague idea that, if that table were generated from gram.y the way
the node support functions are generated from the node definitions,
and the printed forms of analyzed nodes were tied to the gram.y
productions they print as, parentheses and keyword quoting could be
derived mechanically.

As I work through the cases and clues I have collected, a better
structure may become clearer; if a good idea comes to me, I will
make a proposal.

Best regards,
Henson