[GitHub] [flink] twalthr commented on a change in pull request #12377: [FLINK-13782][table-api] Implement type strategies for comparison expressions

[GitBox <gi...@apache.org>]

twalthr commented on a change in pull request #12377:
URL: https://github.com/apache/flink/pull/12377#discussion_r432060612



##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/ComparableTypeStrategy.java
##########
@@ -97,100 +98,84 @@ public ArgumentCount getArgumentCount() {
 		return Optional.of(argumentDataTypes);
 	}
 
-	private boolean areComparable(LogicalType firstLogicalType, LogicalType secondLogicalType) {
+	private boolean areComparable(LogicalType firstType, LogicalType secondType) {
 		// A hack to support legacy types. To be removed when we drop the legacy types.
-		if (firstLogicalType instanceof LegacyTypeInformationType ||
-				secondLogicalType instanceof LegacyTypeInformationType) {
+		if (firstType instanceof LegacyTypeInformationType ||
+				secondType instanceof LegacyTypeInformationType) {
 			return true;
 		}
 
 		// everything is comparable with null, it should return null in that case
-		if (hasRoot(firstLogicalType, LogicalTypeRoot.NULL) || hasRoot(secondLogicalType, LogicalTypeRoot.NULL)) {
+		if (hasRoot(firstType, LogicalTypeRoot.NULL) || hasRoot(secondType, LogicalTypeRoot.NULL)) {
 			return true;
 		}
 
-		if (hasRoot(firstLogicalType, LogicalTypeRoot.STRUCTURED_TYPE) &&
-				hasRoot(secondLogicalType, LogicalTypeRoot.STRUCTURED_TYPE)) {
-			return areStructuredTypesComparable(firstLogicalType, secondLogicalType);
+		if (firstType.getTypeRoot() == secondType.getTypeRoot()) {
+			return areTypesOfSameRootComparable(firstType, secondType);
 		}
 
-		if (hasRoot(firstLogicalType, LogicalTypeRoot.DISTINCT_TYPE) &&
-				hasRoot(secondLogicalType, LogicalTypeRoot.DISTINCT_TYPE)) {
-			return areDistinctTypesComparable(firstLogicalType, secondLogicalType);
-		}
-
-		if (hasRoot(firstLogicalType, LogicalTypeRoot.RAW) && hasRoot(secondLogicalType, LogicalTypeRoot.RAW)) {
-			return areRawTypesComparable(firstLogicalType, secondLogicalType);
-		}
-
-		if (hasRoot(firstLogicalType, LogicalTypeRoot.ARRAY) && hasRoot(secondLogicalType, LogicalTypeRoot.ARRAY)) {
-			return areCollectionsComparable(firstLogicalType, secondLogicalType);
-		}
-
-		if (hasRoot(firstLogicalType, LogicalTypeRoot.MULTISET) &&
-			hasRoot(secondLogicalType, LogicalTypeRoot.MULTISET)) {
-			return areCollectionsComparable(firstLogicalType, secondLogicalType);
-		}
-
-		if (hasRoot(firstLogicalType, LogicalTypeRoot.MAP) &&
-				hasRoot(secondLogicalType, LogicalTypeRoot.MAP)) {
-			return areCollectionsComparable(firstLogicalType, secondLogicalType);
-		}
-
-		if (hasRoot(firstLogicalType, LogicalTypeRoot.ROW) && hasRoot(secondLogicalType, LogicalTypeRoot.ROW)) {
-			return areCollectionsComparable(firstLogicalType, secondLogicalType);
-		}
-
-		if (firstLogicalType.getTypeRoot() == secondLogicalType.getTypeRoot()) {
-			return true;
-		}
-
-		if (hasFamily(firstLogicalType, LogicalTypeFamily.NUMERIC) &&
-				hasFamily(secondLogicalType, LogicalTypeFamily.NUMERIC)) {
+		if (hasFamily(firstType, LogicalTypeFamily.NUMERIC) && hasFamily(secondType, LogicalTypeFamily.NUMERIC)) {
 			return true;
 		}
 
 		// DATE + ALL TIMESTAMPS
-		if (hasFamily(firstLogicalType, LogicalTypeFamily.DATETIME) &&
-				hasFamily(secondLogicalType, LogicalTypeFamily.DATETIME)) {
+		if (hasFamily(firstType, LogicalTypeFamily.DATETIME) && hasFamily(secondType, LogicalTypeFamily.DATETIME)) {
 			return true;
 		}
 
 		// VARCHAR + CHAR (we do not compare collations here)
-		if (hasFamily(firstLogicalType, LogicalTypeFamily.CHARACTER_STRING) &&
-				hasFamily(secondLogicalType, LogicalTypeFamily.CHARACTER_STRING)) {
+		if (hasFamily(firstType, LogicalTypeFamily.CHARACTER_STRING) &&
+				hasFamily(secondType, LogicalTypeFamily.CHARACTER_STRING)) {
 			return true;
 		}
 
 		// VARBINARY + BINARY
-		if (hasFamily(firstLogicalType, LogicalTypeFamily.BINARY_STRING) &&
-				hasFamily(secondLogicalType, LogicalTypeFamily.BINARY_STRING)) {
+		if (hasFamily(firstType, LogicalTypeFamily.BINARY_STRING) &&
+				hasFamily(secondType, LogicalTypeFamily.BINARY_STRING)) {
 			return true;
 		}
 
 		return false;
 	}
 
-	private boolean areRawTypesComparable(LogicalType firstLogicalType, LogicalType secondLogicalType) {
-		return firstLogicalType.copy(true).equals(secondLogicalType.copy(true)) &&
-			Comparable.class.isAssignableFrom(((RawType<?>) firstLogicalType).getOriginatingClass());
+	private boolean areTypesOfSameRootComparable(LogicalType firstType, LogicalType secondType) {
+		switch (firstType.getTypeRoot()) {
+			case ARRAY:
+			case MULTISET:
+			case MAP:
+			case ROW:
+				return areCollectionsComparable(firstType, secondType);
+			case DISTINCT_TYPE:
+				return areDistinctTypesComparable(firstType, secondType);
+			case STRUCTURED_TYPE:
+				return areStructuredTypesComparable(firstType, secondType);
+			case RAW:
+				return areRawTypesComparable(firstType, secondType);
+			default:
+				return true;
+		}
+	}
+
+	private boolean areRawTypesComparable(LogicalType firstType, LogicalType secondType) {
+		return firstType.equals(secondType) &&
+			Comparable.class.isAssignableFrom(((RawType<?>) firstType).getOriginatingClass());
 	}
 
-	private boolean areDistinctTypesComparable(LogicalType firstLogicalType, LogicalType secondLogicalType) {
-		DistinctType firstDistinctType = (DistinctType) firstLogicalType;
-		DistinctType secondDistinctType = (DistinctType) secondLogicalType;
-		return firstLogicalType.copy(true).equals(secondLogicalType.copy(true)) &&
+	private boolean areDistinctTypesComparable(LogicalType firstType, LogicalType secondType) {
+		DistinctType firstDistinctType = (DistinctType) firstType;
+		DistinctType secondDistinctType = (DistinctType) secondType;
+		return firstType.equals(secondType) &&
 			areComparable(firstDistinctType.getSourceType(), secondDistinctType.getSourceType());

Review comment:
       @dawidwys One last minute comment: We need to remove the nullability for all calls of `areComparable` here and for other types. Maybe it is better to introduce an intermediate function call.




----------------------------------------------------------------
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