[GitHub] [calcite] danny0405 commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
danny0405 commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r440563114
##
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).
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] danny0405 commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
danny0405 commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r440563260
##
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.
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] danny0405 commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
danny0405 commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r440723990
##
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:
Reasonable, in `TypeCoercionImpl#querySourceCoercion` we may need to
take the array element nullability into account.
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] danny0405 commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
danny0405 commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r441940857
##
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)`
##
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.
##
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.
+ *
Review comment:
Fix the document.
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] danny0405 commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
danny0405 commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r443533534
##
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.
+ *
Review comment:
`array/multiset` -> `collection`
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1160,6 +1160,64 @@ 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) {
+assert isCollection(type1) && isCollection(type2);
+
+if (type1.equals(type2)) {
+ return true;
+}
+
+if (type1.getSqlTypeName() == type2.getSqlTypeName()
+&& isCollection(type1) && isCollection(type2)) {
+ return equalSansNullability(factory, type1.getComponentType(),
type2.getComponentType());
+} else {
+ return false;
+}
+ }
+
+ /**
+ * 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) {
+assert isMap(type1) && isMap(type2);
+
+if (type1.equals(type2)) {
+ return true;
+}
+
+if (isMap(type1) && isMap(type2)) {
+ MapSqlType mType1 = (MapSqlType) type1;
+ MapSqlType mType2 = (MapSqlType) type2;
+ return equalSansNullability(factory, mType1.getKeyType(),
mType2.getKeyType())
+ && equalSansNullability(factory, mType1.getValueType(),
mType2.getValueType());
+} else {
+ return false;
Review comment:
`return equalSansNullability(factory, mType1.getKeyType(),
mType2.getKeyType())
&& equalSansNullability(factory, mType1.getValueType(),
mType2.getValueType());` is enough.
##
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:
No need to have `isCollection(type1) && isCollection(type2)` which is
already in the assertion.
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1160,6 +1160,64 @@ 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) {
+assert isCollection(type1) && isCollection(type2);
+
+if (type1.equals(type2)) {
+ return true;
+}
+
+if (type1.getSqlTypeName() == type2.getSqlTypeName()
+&& isCollection(type1) && isCollection(type2)) {
+ return equalSansNullability(factory, type1.getComponentType(),
type2.getComponentType());
+} else {
+ return false;
+}
+ }
+
+ /**
+ * Returns whether two map types are equal, ignoring nullability.
+ *
+ * They need n
[GitHub] [calcite] danny0405 commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
danny0405 commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r446743935
##
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));
Review comment:
Add `type1 == type2` check to see if they are equals for fast check.
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] danny0405 commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
danny0405 commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r446743740
##
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.
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] danny0405 commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
danny0405 commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r446743961
##
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.
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] danny0405 commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
danny0405 commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r447636635
##
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:
Try to reuse the `Fixture`.
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] danny0405 commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
danny0405 commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r448083189
##
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.
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] danny0405 commented on a change in pull request #2019: [CALCITE-4059] SqlTypeUtil#equalSansNullability consider Array/Map type.
danny0405 commented on a change in pull request #2019:
URL: https://github.com/apache/calcite/pull/2019#discussion_r448727632
##
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:
Use `f.typeFactory` instead.
##
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:
Use `f.typeFactory` instead.
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
