tree.FmtExport does not wrap strings with quotes

[rohany]

Found during investigation in #44322.

tree.DNull and "NULL" serialize to the same string because tree.FmtExport does not wrap strings with quotes.

ARRAY['hel, lo'] serializes to "ARRAY[hel,lo]", causing a future parse error.

[rohany]

Once this is fixed, the array parser pkg/sql/sem/tree/parse_array.go will have to be updated to handle single quotes on the values.

It seems like the following should be done:

• When formatting an array, all strings within the array should be quoted.
• When the array parser reads a NULL, it outputs tree.DNull, because NULLs won't be quoted.
• If the parser reads "NULL" or 'NULL' it knows that this is a string, not NULL.

[knz]

I think it's more complicated than that.
We are following pg's array serialization rules for the pgwire text format, because that is what is needed for several ORMs.

We used to always quote strings-in-arrays, but then things would break. It turns out that some clients expect strings that don't contain special characters to not get a quote on serialization.

Rohan I fear the problem you were set to solve initially is not completely trivial. You're trying to find shortcuts around a fundamental design mistake in our SQL code: we're trying to share parsing routines between at least 3 different "channels" of data:

1. the pgwire text protocol
2. the pgwire dump/restore format
3. possibly other things, perhaps CDC (I'm not sure)

I am not 100% sure that (1) and (2) even in pg use the same routines. This needs to be double checked. FYI when crdb's own pgwire code and dump/restore were implemented, we did not carry out this analysis and things happened to "just work" (until they didn't. We were in luck perhaps?).

I'm also pretty sure that (3) should not be using the same code.

In an ideal world, we'd have a library of pairs of builtin functions in SQL like in pg, for byte and text serialization, then we'd use these routines the way they are used in pg. But I'm not sure how much work there is between where we are now and there.

[knz]

NB: my comment about what we were originally doing with array refers to the {...} format when converting an array to string (for pgwire and dump).

I don't actually know what part of the code would get you ARRAY[hel,lo]. That sounds like something that wasn't there. On my machine I see this:

[email protected]:15490/movr> explain(verbose) select array['hel','lo'];
   tree  |     field     |    description    |  columns  | ordering
---------+---------------+-------------------+-----------+-----------
         | distributed   | false             |           |
         | vectorized    | false             |           |
  values |               |                   | ("array") |
         | size          | 1 column, 1 row   |           |
         | row 0, expr 0 | ARRAY['hel','lo'] |           |

See the expression at the bottom, the ARRAY[] is properly pretty-printed so I'm expecting this should work already.

[rohany]

Ok, let me try to investigate this more.

I don't actually know what part of the code would get you ARRAY[hel,lo]. That sounds like something that wasn't there. On my machine I see this:

If you format an array with FmtExport that will happen (i tried it in pkg/sql/sem/tree/parse_array_test.go). I'm not sure if the explain verbose uses that flag.

[rohany]

Rohan I fear the problem you were set to solve initially is not completely trivial. You're trying to find shortcuts around a fundamental design mistake in our SQL code: we're trying to share parsing routines between at least 3 different "channels" of data:

I believe I'm looking at a simpler problem though -- Just in the case of FmtExport. We only need to ensure that our own output is round trippable with ourselves, not postgres. As context, this work is being done to ensure that we have a format that we can always round trip with ourselves for 2DC and other things.

[knz]

all right carry on 👍