[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r448732591
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -117,6 +120,34 @@
SqlTypeCoercionRule.THREAD_PROVIDERS.set(defaultRules);
}
+ @Test void testEqualAsCollectionSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+
+// case array
+assertThat(
+equalAsCollectionSansNullability(factory, f.arrayBigInt,
f.arrayBigIntNullable),
+is(true));
+
+// case multiset
+assertThat(
+equalAsCollectionSansNullability(factory, f.multisetBigInt,
f.multisetBigIntNullable),
+is(true));
+
+// multiset and array are not equal.
+assertThat(
+equalAsCollectionSansNullability(factory, f.arrayBigInt,
f.multisetBigInt),
+is(false));
+ }
+
+ @Test void testEqualAsMapSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+
Review comment:
Thanks for your suggestion, updated already.
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -117,6 +120,34 @@
SqlTypeCoercionRule.THREAD_PROVIDERS.set(defaultRules);
}
+ @Test void testEqualAsCollectionSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
Review comment:
Thanks for your suggestion, updated already.
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r448440265
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -117,6 +120,45 @@
SqlTypeCoercionRule.THREAD_PROVIDERS.set(defaultRules);
}
+ @Test void testEqualAsCollectionSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+
Review comment:
> I would prefer add the types to `SqlTypeFixture` and give it a good
name.
Thanks for your suggestion, updated already.
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r448439014
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -117,6 +120,47 @@
SqlTypeCoercionRule.THREAD_PROVIDERS.set(defaultRules);
}
+ @Test void testEqualAsCollectionSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+final RelDataType bigint = factory.createSqlType(SqlTypeName.BIGINT);
+
+// case array
+final RelDataType arrayNullable =
+new ArraySqlType(factory.createTypeWithNullability(bigint, true),
false);
+final RelDataType arrayNotNull =
+new ArraySqlType(factory.createTypeWithNullability(bigint, false),
false);
+assertThat(equalAsCollectionSansNullability(factory, arrayNullable,
arrayNotNull),
+is(true));
+
+// case multiset
+final RelDataType setNullable =
+new MultisetSqlType(factory.createTypeWithNullability(bigint, true),
false);
+final RelDataType setNotNull =
+new MultisetSqlType(factory.createTypeWithNullability(bigint, false),
false);
+assertThat(equalAsCollectionSansNullability(factory, setNullable,
setNotNull),
+is(true));
+
+// multiset and array are not equal.
+assertThat(equalAsCollectionSansNullability(factory, arrayNotNull,
setNotNull),
+is(false));
+ }
+
+ @Test void testEqualAsMapSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+final RelDataType bigint = factory.createSqlType(SqlTypeName.BIGINT);
Review comment:
Thanks for your suggestion, updated already.
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r448438918
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -117,6 +120,45 @@
SqlTypeCoercionRule.THREAD_PROVIDERS.set(defaultRules);
}
+ @Test void testEqualAsCollectionSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+
Review comment:
> I would prefer add the types to `SqlTypeFixture` and give it a good
name.
Thanks for your suggestion, updated already.
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r448079966
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -117,6 +120,45 @@
SqlTypeCoercionRule.THREAD_PROVIDERS.set(defaultRules);
}
+ @Test void testEqualAsCollectionSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+
Review comment:
The basic type is replaced, but nested type in SqlTypeFixture does not
meet the requirements(map type/nullability), should I add these to
SqlTypeFixture? Or just leave them in this ut.
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r447369330
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -117,6 +120,47 @@
SqlTypeCoercionRule.THREAD_PROVIDERS.set(defaultRules);
}
+ @Test void testEqualAsCollectionSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+final RelDataType bigint = factory.createSqlType(SqlTypeName.BIGINT);
+
+// case array
+final RelDataType arrayNullable =
+new ArraySqlType(factory.createTypeWithNullability(bigint, true),
false);
+final RelDataType arrayNotNull =
+new ArraySqlType(factory.createTypeWithNullability(bigint, false),
false);
+assertThat(equalAsCollectionSansNullability(factory, arrayNullable,
arrayNotNull),
+is(true));
+
+// case multiset
+final RelDataType setNullable =
+new MultisetSqlType(factory.createTypeWithNullability(bigint, true),
false);
+final RelDataType setNotNull =
+new MultisetSqlType(factory.createTypeWithNullability(bigint, false),
false);
+assertThat(equalAsCollectionSansNullability(factory, setNullable,
setNotNull),
+is(true));
+
+// multiset and array are not equal.
+assertThat(equalAsCollectionSansNullability(factory, arrayNotNull,
setNotNull),
+is(false));
+ }
+
+ @Test void testEqualAsMapSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+final RelDataType bigint = factory.createSqlType(SqlTypeName.BIGINT);
Review comment:
> Use type from `SqlTypeFixture` if possible.
The basic type is replaced, but nested type in SqlTypeFixture does not meet
the requirements(map type/nullability), should I add these to SqlTypeFixture?
Or just leave them in this ut.
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r447369330
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -117,6 +120,47 @@
SqlTypeCoercionRule.THREAD_PROVIDERS.set(defaultRules);
}
+ @Test void testEqualAsCollectionSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+final RelDataType bigint = factory.createSqlType(SqlTypeName.BIGINT);
+
+// case array
+final RelDataType arrayNullable =
+new ArraySqlType(factory.createTypeWithNullability(bigint, true),
false);
+final RelDataType arrayNotNull =
+new ArraySqlType(factory.createTypeWithNullability(bigint, false),
false);
+assertThat(equalAsCollectionSansNullability(factory, arrayNullable,
arrayNotNull),
+is(true));
+
+// case multiset
+final RelDataType setNullable =
+new MultisetSqlType(factory.createTypeWithNullability(bigint, true),
false);
+final RelDataType setNotNull =
+new MultisetSqlType(factory.createTypeWithNullability(bigint, false),
false);
+assertThat(equalAsCollectionSansNullability(factory, setNullable,
setNotNull),
+is(true));
+
+// multiset and array are not equal.
+assertThat(equalAsCollectionSansNullability(factory, arrayNotNull,
setNotNull),
+is(false));
+ }
+
+ @Test void testEqualAsMapSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+final RelDataType bigint = factory.createSqlType(SqlTypeName.BIGINT);
Review comment:
> Use type from `SqlTypeFixture` if possible.
Type in SqlTypeFixture does not meet the requirements(map type/nullability),
should I add these to SqlTypeFixture? Or just leave them in this ut.
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r447369330
##
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##
@@ -117,6 +120,47 @@
SqlTypeCoercionRule.THREAD_PROVIDERS.set(defaultRules);
}
+ @Test void testEqualAsCollectionSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+final RelDataType bigint = factory.createSqlType(SqlTypeName.BIGINT);
+
+// case array
+final RelDataType arrayNullable =
+new ArraySqlType(factory.createTypeWithNullability(bigint, true),
false);
+final RelDataType arrayNotNull =
+new ArraySqlType(factory.createTypeWithNullability(bigint, false),
false);
+assertThat(equalAsCollectionSansNullability(factory, arrayNullable,
arrayNotNull),
+is(true));
+
+// case multiset
+final RelDataType setNullable =
+new MultisetSqlType(factory.createTypeWithNullability(bigint, true),
false);
+final RelDataType setNotNull =
+new MultisetSqlType(factory.createTypeWithNullability(bigint, false),
false);
+assertThat(equalAsCollectionSansNullability(factory, setNullable,
setNotNull),
+is(true));
+
+// multiset and array are not equal.
+assertThat(equalAsCollectionSansNullability(factory, arrayNotNull,
setNotNull),
+is(false));
+ }
+
+ @Test void testEqualAsMapSansNullability() {
+final SqlTypeFactoryImpl factory =
+new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+final RelDataType bigint = factory.createSqlType(SqlTypeName.BIGINT);
Review comment:
> Use type from `SqlTypeFixture` if possible.
ok
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1160,6 +1161,49 @@ public static boolean equalSansNullability(
factory.createTypeWithNullability(type2, type1.isNullable()));
}
+ /**
+ * Returns whether two array/multiset types are equal, ignoring nullability.
+ *
+ * They need not come from the same factory.
+ *
+ * @param factory Type factory
+ * @param type1 First type
+ * @param type2 Second type
+ * @return Whether types are equal, ignoring nullability
+ */
+ public static boolean equalAsCollectionSansNullability(
+ RelDataTypeFactory factory,
+ RelDataType type1,
+ RelDataType type2) {
+Preconditions.checkArgument(isCollection(type1));
+Preconditions.checkArgument(isCollection(type2));
+return type1.getSqlTypeName() == type2.getSqlTypeName()
+&& equalSansNullability(factory, type1.getComponentType(),
type2.getComponentType());
+ }
+
+ /**
+ * Returns whether two map types are equal, ignoring nullability.
+ *
+ * They need not come from the same factory.
+ *
+ * @param factory Type factory
+ * @param type1 First type
+ * @param type2 Second type
+ * @return Whether types are equal, ignoring nullability
+ */
+ public static boolean equalAsMapSansNullability(
+ RelDataTypeFactory factory,
+ RelDataType type1,
+ RelDataType type2) {
+Preconditions.checkArgument(isMap(type1));
Review comment:
> Add `type1 == type2` check to see if they are equals for fast check.
ok
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r443140920
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1160,6 +1160,59 @@ public static boolean equalSansNullability(
factory.createTypeWithNullability(type2, type1.isNullable()));
}
+ /**
+ * Returns whether two array types are equal, ignoring nullability.
+ *
+ * They need not come from the same factory.
+ *
+ * @param factory Type factory
+ * @param type1 First type
+ * @param type2 Second type
+ * @return Whether types are equal, ignoring nullability
+ */
+ public static boolean equalAsCollectionSansNullability(
+ RelDataTypeFactory factory,
+ RelDataType type1,
+ RelDataType type2) {
+if (type1.equals(type2)) {
+ return true;
+}
+
+if (isCollection(type1) && isCollection(type2)) {
+ return equalSansNullability(factory, type1.getComponentType(),
type2.getComponentType());
Review comment:
> And should a `MULTISET` equals to `ARRAY` ? I would say no.
Good point, that's a bug, thanks for your review, I've updated my PR.
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r443140543
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1160,6 +1160,59 @@ public static boolean equalSansNullability(
factory.createTypeWithNullability(type2, type1.isNullable()));
}
+ /**
+ * Returns whether two array types are equal, ignoring nullability.
+ *
+ * They need not come from the same factory.
+ *
+ * @param factory Type factory
+ * @param type1 First type
+ * @param type2 Second type
+ * @return Whether types are equal, ignoring nullability
+ */
+ public static boolean equalAsCollectionSansNullability(
+ RelDataTypeFactory factory,
+ RelDataType type1,
+ RelDataType type2) {
+if (type1.equals(type2)) {
+ return true;
+}
+
+if (isCollection(type1) && isCollection(type2)) {
+ return equalSansNullability(factory, type1.getComponentType(),
type2.getComponentType());
Review comment:
> we should add assertion instead in the first line: `assert
isCollection(type1) && isCollection(type2)`
I'll add an assertion. Cuz assert is not enforce in Java, I'll still keep
the condition in if clause.
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r443140543
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1160,6 +1160,59 @@ public static boolean equalSansNullability(
factory.createTypeWithNullability(type2, type1.isNullable()));
}
+ /**
+ * Returns whether two array types are equal, ignoring nullability.
+ *
+ * They need not come from the same factory.
+ *
+ * @param factory Type factory
+ * @param type1 First type
+ * @param type2 Second type
+ * @return Whether types are equal, ignoring nullability
+ */
+ public static boolean equalAsCollectionSansNullability(
+ RelDataTypeFactory factory,
+ RelDataType type1,
+ RelDataType type2) {
+if (type1.equals(type2)) {
+ return true;
+}
+
+if (isCollection(type1) && isCollection(type2)) {
+ return equalSansNullability(factory, type1.getComponentType(),
type2.getComponentType());
Review comment:
> we should add assertion instead in the first line: `assert
isCollection(type1) && isCollection(type2)`
I'll add an assertion. Cuz assert is not enforce in Java, so I still keep
the condition in if clause.
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r441301149
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1510,6 +1563,31 @@ public static boolean isMap(RelDataType type) {
return type.getSqlTypeName() == SqlTypeName.MAP;
}
+ /**
+ * @return true if type is MULTISET
+ */
+ public static boolean isMultiset(RelDataType type) {
Review comment:
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r441298797
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1510,6 +1563,31 @@ public static boolean isMap(RelDataType type) {
return type.getSqlTypeName() == SqlTypeName.MAP;
}
+ /**
+ * @return true if type is MULTISET
+ */
+ public static boolean isMultiset(RelDataType type) {
Review comment:
Indeed, I did not use this method, but basically all RelDataType have a
similar utility method, so I added this for future use. If you think this is
unnecessary, I can remove this.
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r441298797
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1510,6 +1563,31 @@ public static boolean isMap(RelDataType type) {
return type.getSqlTypeName() == SqlTypeName.MAP;
}
+ /**
+ * @return true if type is MULTISET
+ */
+ public static boolean isMultiset(RelDataType type) {
Review comment:
Indeed, I did not use this method, but basically, all RelDataType have a
similar utility method, so I added this for the future use. If you think this
is unnecessary, I can remove this.
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r440571689
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1151,13 +1151,22 @@ public static boolean equalSansNullability(
return true;
}
-if (type1.isNullable() == type2.isNullable()) {
+if (isAtomic(type1) && isAtomic(type2) && (type1.isNullable() ==
type2.isNullable())) {
// If types have the same nullability and they weren't equal above,
// they must be different.
return false;
}
-return type1.equals(
-factory.createTypeWithNullability(type2, type1.isNullable()));
+if (isArray(type1) && isArray(type2)) {
+ return equalSansNullability(factory, type1.getComponentType(),
type2.getComponentType());
+} else if (isMap(type1) && isMap(type2)) {
+ MapSqlType mType1 = (MapSqlType) type1;
Review comment:
> BTW, can you share why you need to compare map and array types ? Seems
the case is rare from my side.
A case like this: array(12L) will be recognized as "INTEGER ARRAY, NOT
NULL", but the col i_ids is "BIGINT ARRAY NULLABLE", so calcite will add a
cast. But the source/targe nullability is still different, and we don't want to
consider nullability.
```
INSERT TABLE new_t
SELECT id,
array(12) AS i_ids,
FROM t
```
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:
us...@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
Aaron commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r440569282
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1151,13 +1151,22 @@ public static boolean equalSansNullability(
return true;
}
-if (type1.isNullable() == type2.isNullable()) {
+if (isAtomic(type1) && isAtomic(type2) && (type1.isNullable() ==
type2.isNullable())) {
// If types have the same nullability and they weren't equal above,
// they must be different.
return false;
}
-return type1.equals(
-factory.createTypeWithNullability(type2, type1.isNullable()));
+if (isArray(type1) && isArray(type2)) {
+ return equalSansNullability(factory, type1.getComponentType(),
type2.getComponentType());
+} else if (isMap(type1) && isMap(type2)) {
+ MapSqlType mType1 = (MapSqlType) type1;
Review comment:
> I would suggest to add two new methods `equalAsArraySansNullability`
and `equalsAsMapSansNullability`, because for most of the cases when we invoke
these methods, we already know if the type to compare is a map/array/struct,
add new methods simplify the original impl(which is the most common case).
Thanks for your advice, I'll change this latter.
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:
us...@infra.apache.org
