You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/05/11 22:15:25 UTC
[GitHub] [calcite] snuyanzin opened a new pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
snuyanzin opened a new pull request #2413:
URL: https://github.com/apache/calcite/pull/2413
The PR implements least restrictive for collection of collections for instance
```sql
select array[array['hello'], array['world'], array['!']] as "array"
```
and others mentioned in CALCITE-4603
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] snuyanzin commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r630981313
##########
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java
##########
@@ -83,6 +83,37 @@
assertThat(leastRestrictive.isNullable(), is(true));
}
+ @Test void testLeastRestrictiveForArrays() {
Review comment:
thanks, more tests are added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] snuyanzin closed pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
snuyanzin closed pull request #2413:
URL: https://github.com/apache/calcite/pull/2413
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] snuyanzin commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r630931521
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r631838455
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
That is up to you :)
A minor note on the current issue: what do you think of splitting `leastRestrictiveCollectionType` into `leastRestrictiveMapType` and `leastRestrictiveArrayType` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r632340367
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,52 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveArrayMultisetType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY || sqlTypeName == SqlTypeName.MULTISET;
+ boolean isNullable = false;
+ for (RelDataType type: types) {
+ if (type.getComponentType() == null) {
+ return null;
+ }
+ isNullable |= type.isNullable();
+ }
+ final RelDataType type = leastRestrictive(
+ Util.transform(types,
+ t -> t instanceof ArraySqlType
+ ? ((ArraySqlType) t).getComponentType()
+ : ((MultisetSqlType) t).getComponentType()));
+ if (type == null) {
+ return null;
+ }
+ return sqlTypeName == SqlTypeName.ARRAY
+ ? new ArraySqlType(type, isNullable)
+ : new MultisetSqlType(type, isNullable);
+ }
+
+ protected @Nullable RelDataType leastRestrictiveMapType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type: types) {
+ if (type.getKeyType() == null || type.getValueType() == null) {
Review comment:
```suggestion
if (!(type instanceof MapSqlType)) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#issuecomment-841068715
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] snuyanzin commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r630981540
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
just added the changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r631681726
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r630814848
##########
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java
##########
@@ -83,6 +83,37 @@
assertThat(leastRestrictive.isNullable(), is(true));
}
+ @Test void testLeastRestrictiveForArrays() {
Review comment:
Maybe we should add tests where leastRestrictive is not computable, to verify it returns the expected result (`null`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r631740972
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
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)`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r631681726
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] snuyanzin commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r631850102
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
yes, after moving of `isNullable |= type.isNullable();` into key+value/component loop such splittiing becomes more logic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r630817348
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
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 `RelDataTypeFactoryleastRestrictive` contract), it would get a NPE.
Maybe this method should add a pre-check to verify that all the types in the list are the "expected" sql type (either MAP or MULTISET or ARRAY), before going on (and if the pre-check fails, return `null`). WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r632340367
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,52 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveArrayMultisetType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY || sqlTypeName == SqlTypeName.MULTISET;
+ boolean isNullable = false;
+ for (RelDataType type: types) {
+ if (type.getComponentType() == null) {
+ return null;
+ }
+ isNullable |= type.isNullable();
+ }
+ final RelDataType type = leastRestrictive(
+ Util.transform(types,
+ t -> t instanceof ArraySqlType
+ ? ((ArraySqlType) t).getComponentType()
+ : ((MultisetSqlType) t).getComponentType()));
+ if (type == null) {
+ return null;
+ }
+ return sqlTypeName == SqlTypeName.ARRAY
+ ? new ArraySqlType(type, isNullable)
+ : new MultisetSqlType(type, isNullable);
+ }
+
+ protected @Nullable RelDataType leastRestrictiveMapType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type: types) {
+ if (type.getKeyType() == null || type.getValueType() == null) {
Review comment:
```suggestion
if (!(type instanceof MapSqlType) || type.getKeyType() == null || type.getValueType() == null) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] snuyanzin commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r631730503
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] snuyanzin commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r631730503
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r630817348
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] snuyanzin closed pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
snuyanzin closed pull request #2413:
URL: https://github.com/apache/calcite/pull/2413
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] snuyanzin commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r631833702
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
Thanks @vlsi
Do you think it makes to create a separate issue for an alternate approach?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] snuyanzin commented on a change in pull request #2413: [CALCITE-4603] Least restrictive for collections of collections
Posted by GitBox <gi...@apache.org>.
snuyanzin commented on a change in pull request #2413:
URL: https://github.com/apache/calcite/pull/2413#discussion_r631851742
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java
##########
@@ -257,6 +260,35 @@ private RelDataType createStructType(
return createTypeWithNullability(builder.build(), isNullable);
}
+ protected @Nullable RelDataType leastRestrictiveCollectionType(
+ final List<RelDataType> types, SqlTypeName sqlTypeName) {
+ assert sqlTypeName == SqlTypeName.ARRAY
+ || sqlTypeName == SqlTypeName.MULTISET || sqlTypeName == SqlTypeName.MAP;
+ boolean isNullable = false;
+ for (RelDataType type : types) {
+ isNullable |= type.isNullable();
+ }
+ if (sqlTypeName == SqlTypeName.MAP) {
+ RelDataType keyType = leastRestrictive(
+ Util.transform(types, t -> Objects.requireNonNull(t.getKeyType())));
Review comment:
however it would be `leastRestrictiveMapType` and `leastRestrictiveArrayMultisetType` as the latest one is both for `Array` and `Multiset`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org