Support PostgreSQL array created using array internal type name

[guyco33]

Fixing failure when selecting Postgres Array column that was created with native type name (eg: _int4)

Postgres: create table t (a _int4);
Presto: select a from t;

io.prestosql.spi.PrestoException: No array dimensions for ARRAY type: JdbcTypeHandle{jdbcType=2003, jdbcTypeName=_int4, columnSize=10, decimalDigits=0}
	at io.prestosql.plugin.postgresql.PostgreSqlClient.lambda$null$0(PostgreSqlClient.java:248)
	at java.util.Optional.orElseThrow(Optional.java:290)
	at io.prestosql.plugin.postgresql.PostgreSqlClient.lambda$toPrestoType$1(PostgreSqlClient.java:248)

Explanation:
pg_attribute.attndims is 0 when creating columns with the native type names

create table t (a _int4); --> pg_attribute.attndims = 0 for column a
create table t (a int[]); --> pg_attribute.attndims = 1 for column a

Fix potential issue for #317

[findepi]

@guyco33 i checked in Postgres 11.2 and confirmed your findings:

test=# create table t(a int4[], b _int4);
CREATE TABLE

test=# \d t
                  Table "public.t"
 Column |   Type    | Collation | Nullable | Default
--------+-----------+-----------+----------+---------
 a      | integer[] |           |          |
 b      | integer[] |           |          |

test=# SELECT attname,  attndims FROM pg_attribute att JOIN pg_class tbl ON tbl.oid = att.attrelid WHERE tbl.relname = 't';
 attname  | attndims
----------+----------
...
 a        |        1
 b        |        0

However

• https://www.postgresql.org/docs/11/arrays.html#ARRAYS-DECLARATION does not mention that you can use _<typename> to declare an array
• if i understand correctly and int4[], _int4 should be treated equivalent in Postgres, they should also have the same attndims.
  Is this a bug in Postgres? Is it reported?

[findepi]

@guyco33 for now, let's get #668 in.

let's discuss whether we need this change or not.
i found an old thread (https://www.postgresql.org/message-id/8C5B026B51B6854CBE88121DBF097A8651DB95%40ehost010-33.exch010.intermedia.net)
and i am under impression that we don't and this is on Postgres side.

But maybe there is something newer that I should refer to?
Especially -- is there a bug/issue in Postgres tracking undesirable difference in behavior between eg int4[] and _int4?

[guyco33]

• if i understand correctly and int4[], _int4 should be treated equivalent in Postgres, they should also have the same attndims.
  Is this a bug in Postgres? Is it reported?

@findepi Maybe attndims has other internal purpose. For example to be able to reproduce the original DDL statement.

[guyco33]

let's discuss whether we need this change or not.
i found an old thread (https://www.postgresql.org/message-id/8C5B026B51B6854CBE88121DBF097A8651DB95%40ehost010-33.exch010.intermedia.net)
and i am under impression that we don't and this is on Postgres side.

I think that our main motivation should be to map all Postgres array columns although they might be created with native names - I guess that there are plenty of automated tools that construct Postgres tables from their native names and we might loose them without this change (I encountered lots of Postgres sources with such tables)

But maybe there is something newer that I should refer to?
Especially -- is there a bug/issue in Postgres tracking undesirable difference in behavior between eg int4[] and _int4?

From https://www.postgresql.org/docs/11/catalog-pg-attribute.html we can learn that attndims is not enforced:
Presently, the number of dimensions of an array is not enforced, so any nonzero value effectively means “it's an array”.

[findepi]

I asked for clarification on PostgreSQL user group https://www.postgresql.org/message-id/CAMrFzbt_DjD1BSLOhAUgNApu8MV3CCPmbEac9FfMxOOq6GAD0g%40mail.gmail.com

[martint]

And here's the response: https://www.postgresql.org/message-id/31659.1556226223%40sss.pgh.pa.us

[findepi]

And a follow up: https://www.postgresql.org/message-id/32678.1556228173%40sss.pgh.pa.us

In particular:

the Postgres type system doesn't distinguish
arrays of different numbers of dimensions, ie, int4[][] is not
really different from int4[]. So you can't attach any very strong
meaning to attndims = 2 vs attndims = 1.

I therefore propose that we go with #682

[findepi]

@guyco33 let's discuss under #682

[findepi]

Now that we agreed to the way forward in #682 (comment), I feel this is a good change -- let's treat arrays with undeclared dimensions as single-dimension arrays.

I have some review comments.
Also, please rebase to accommodate for my #687.

Please change the commit message to something like

Support PostgreSQL array created using array internal type name

[findepi]

Would this be following be enough

Suggested change

	"AND CASE WHEN att.attndims = 0 AND attyp.typcategory = 'A' THEN 1 ELSE att.attndims END > 0 ";
	"AND attyp.typcategory = 'A' ";

(if not sufficient, then this should be AND (attyp.typcategory = 'A' OR att.attndims > 0), but I would strongly not have such an OR until I understand why one would be needed)

?

[guyco33]

Seems that your suggestion is sufficient unless there are cases that attndims is NULL.
Anyway, selecting COALESCE(att.attndims,1) will cover this as well.

[findepi]

Would that work?

Suggested change

	"SELECT att.attname, CASE WHEN att.attndims = 0 AND attyp.typcategory = 'A' THEN 1 ELSE att.attndims END AS attndims " +
	"SELECT att.attname, max(att.attndims, 1) " +

[guyco33]

Should be greatest(att.attndims, 1)

[findepi]

Please rebase. You will need to remove testInternalArray.

[findepi]

Use DataTypeTest with arrayDataType(integerDataType(), "_int4")

[guyco33]

Is there away to use DataTypeTest with a predefined table?
_int4 can't be created by CTAS from presto.

[findepi]

Please see how TestPostgreSqlTypeMapping#postgresCreateAndInsert is used. When using this method, the table is created and populated directly in PostgreSQL.

[guyco33]

Got it. Thanks!

[findepi]

Merged, thanks!