[CALCITE-4603] Least restrictive for collections of collections

[snuyanzin]

The PR implements least restrictive for collection of collections for instance

 select array[array['hello'], array['world'], array['!']] as "array"

and others mentioned in CALCITE-4603

[rubenada]

I have the impression that, with this code, when trying to calculate an "impossible" leastRestrictive (e.g. between MAP and ARRAY, or MULTISET and VARCHAR), instead of returning null (as it should per RelDataTypeFactory#leastRestrictive contract), it would get a NPE.
Maybe this method should add a pre-check to verify that all the types in the list have the "expected" SqlTypeName (either MAP or MULTISET or ARRAY), before going on (and if the pre-check fails, return null). WDYT?

[snuyanzin]

thanks for highlighting this.
Yes it definitely makes sense. The only thing is that it seems that casting from MULTISET to ARRAY and vice versa is allowed and even used in JdbcTest. So I would go with check that either all the types have non null component info or all the types have keytype and valuetype

[snuyanzin]

just added the changes

[rubenada]

Ok, seems good.
nitpick:

• I guess with the new code, the Objects.requireNonNull in the three Util.transform is not necessary, since the null case has been checked (and early-exited) before.
• The isNullable |= type.isNullable(); computation could be integrated into the new "null key+value/component loop", to just have one "pre-computation loop" instead of two.
• If keyType is null we could just return null without computing valueType
• You could add final to keyType, valueType and type variables

[snuyanzin]

I guess with the new code, the Objects.requireNonNull in the three Util.transform is not necessary, since the null case has been checked (and early-exited) before.

Checkerframework says that it is required for Util.transform to have @NonNull RelDataType, that's why it still makes sense to have Objects.requireNonNull there, otherwise ./gradlew with Objects.requireNonNull will fail at these lines.

For others makes sense, thanks
I updated the code

[vlsi]

The reason it complains is that RelDataType#getKeyType() is nullable, while MapSqlType#getKeyType() is non-nullable.
The checker is not sure if getKeyType() is guaranteed to return the same value every time.

If you cast t to MapSqlType, then it would work without requireNonNull.
An alternative approaches are:
a) Make leastRestrictive return null when null is among its arguments. Currently leastRestrictive would fail with NPE
b) Rework leastRestrictive into builder-like API, so it could indicate to the caller that no more types are needed. The user could instantiate a couple of builders (e.g. key builder, value builder), iterate over the types, and build the common type (e.g. right inside for (RelDataType type: types))

[snuyanzin]

Thanks @vlsi
Do you think it makes to create a separate issue for an alternate approach?

[vlsi]

That is up to you :)

A minor note on the current issue: what do you think of splitting leastRestrictiveCollectionType into leastRestrictiveMapType and leastRestrictiveArrayType ?

[snuyanzin]

yes, after moving of isNullable |= type.isNullable(); into key+value/component loop such splittiing becomes more logic

[snuyanzin]

however it would be leastRestrictiveMapType and leastRestrictiveArrayMultisetType as the latest one is both for Array and Multiset

[rubenada]

LGTM