raminqaf commented on code in PR #29411:
URL: https://github.com/apache/flink/pull/29411#discussion_r4208609167
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/logical/utils/LogicalTypeChecks.java:
##########
@@ -257,6 +257,17 @@ public static boolean areComparable(
firstType.copy(true), secondType.copy(true),
requiredComparison);
}
+ /**
+ * Checks whether values of a (possibly nested) logical type can be used
as grouping, join,
+ * partition or sort keys and as operands of comparison operators.
+ *
+ * <p>VARIANT is not a comparable key type, because one VARIANT value has
many valid binary
+ * encodings.
+ */
+ public static boolean isComparableKeyType(LogicalType logicalType) {
Review Comment:
`areComparable` answers a different question: whether a function can compare
two values. It says MAP is comparable and even orderable, although batch can't
sort MAP keys. It says a structured type with the default `EQUALS` comparison
isn't orderable, although `ORDER BY` on such a type works today in batch. So
using it for keys would reject queries that work today and still let MAP keys
through. #29416 goes the other way: `areComparable` calls this method, so
VARIANT has one rule in both places.
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/logical/utils/LogicalTypeChecks.java:
##########
@@ -257,6 +257,17 @@ public static boolean areComparable(
firstType.copy(true), secondType.copy(true),
requiredComparison);
}
+ /**
+ * Checks whether values of a (possibly nested) logical type can be used
as grouping, join,
+ * partition or sort keys and as operands of comparison operators.
+ *
+ * <p>VARIANT is not a comparable key type, because one VARIANT value has
many valid binary
+ * encodings.
Review Comment:
Good point, VARIANT is not the only type with this problem. I checked on
this branch. `MAP['a', 1, 'b', 2] = MAP['b', 2, 'a', 1]` returns `true`, so
comparing two maps is correct. Grouping is not: streaming `GROUP BY m` returns
two groups for these maps, and batch rejects MAP keys during code generation.
MULTISET is stored as a map, so it should behave the same.
Structured keys compare attribute by attribute, so they only break with a
VARIANT or MAP attribute, and a VARIANT attribute is already rejected.
RAW keys compare the serialized bytes.
The MAP and MULTISET grouping bug exists without VARIANT, and Spark fixes it
by sorting map entries instead of rejecting them.
So I'd keep it out of this PR and open a JIRA for it. I'll reword the
Javadoc so it states what every key needs, without claiming the method decides
every key type.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
