Cockroach: sqlfmt: Consistency on keeping native pg-types

[lopezator]

Describe the problem

I'm trying to keep my crdb schema & seed files pg-compat by using pg-types instead of crdb aliases.

e.g:

JSONB instead of JSON
VARCHAR instead of STRING
...

I am also using sqlfmt to format my SQL files and was working perfect with the usecase above until now.

When using the BYTEA pg-type, sqlfmt replaces it for crdb alias BYTES.

@justinj found another example where sqlfmt does the same, replacing:

TEXT in favor of STRING

Please describe the issue you observed, and any steps we can take to reproduce it:

sqlfmt <<< "CREATE TABLE test (a TEXT, b BYTEA);"

prints:

CREATE TABLE test (a STRING, b BYTES)

Expected behavior

Respecting pg-compatible types, printing:

CREATE TABLE test (a TEXT, b BYTEA)

CC\ @mjibson @knz

[mjibson]

There's a bunch of other various types like this too. double precision, real, bigint, int64, etc. Currently the parser accepts those words but immediately converts them to their more canonical type without holding onto the original name (https://github.com/cockroachdb/cockroach/blob/f4a9098cefd1ce72c18b8f2739004029b609965e/pkg/sql/parser/sql.y#L6391). This might not change soon. sqlfmt has always been mostly-postgres-compatible, and this is one of those areas where it's not 100%. sqlfmt doesn't have a goal of being the formatter for everything, including postgres. I don't think we're going to change this soon, but we'll keep it in mind.

[lopezator]

I understand. What confused me was the different behavior.

e.g. Not replacing varchar(n) for string(n) but do it for other types, like the listed above.

Thanks for the response.

[mjibson]

Good point. I'm bringing this up internally. We do save the type name for char and varchar, but not text, even though all representations of these are identical under the covers. Maybe we'll do this.

[lopezator]

Any news on this?

[mjibson]

For the short term this is not going to change. Undecided for longer term.

[knz]

1) FYI cockroachdb 19.1 will preservs varchar as a separate type from text/string, for better compatibility with postgres

2) generally renaming bytes -> bytea and string -> text internally throughout CockroachDB, is, IMHO, a worthwhile investment to better position crdb in the postgres ecosystem. I think it may be worth elevating this discussion above and beyond the limited scope of sqlfmt.

[lopezator]

Related to this, if we add a new GIN index in the following form, in order to keep our SQL as much pg-compat as possible:

CREATE INDEX foo__bar__idx ON table USING GIN (bar);

sqlfmt rewrites it as:

CREATE INVERTED INDEX foo__bar__idx ON foo (bar)

Does it make sense to rewrite a "valid" statement? Which is the reasoning behind this?

[jordanlewis]

I think we took a shortcut and don't have separate AST forms for these two statements, so sqlfmt (and cockroach) think of both as the CREATE INVERTED INDEX AST form. We can fix this, and in fact I think should.