rubenada commented on code in PR #5393:
URL: https://github.com/apache/calcite/pull/5393#discussion_r4482831521
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/PhysTypeImpl.java:
##########
@@ -544,6 +530,37 @@ private Expression generateComparator(RelCollation
collation,
});
}
+ private static boolean isComposite(RelDataType type) {
+ return type.isStruct() || type.getComponentType() != null;
+ }
+
+ /** ROW and ARRAY sort keys are not Comparable.
+ * Use the same element-wise comparator as the single-key sort path. */
+ private static Expression generateComparison(RelDataType type, String method,
Review Comment:
I find this code (and the dynamically generated code that it leads to) a but
hard to follow. What do you think of making it a bit more readable and arguably
easier to maintain by doing something like (code not verified):
In `EnumerableDefaults` (I guess next to `BothValuesAreNullException`
definition):
```
/** Variant of {@link #compareNullsLastForMergeJoin(Comparable, Comparable,
Comparator)}
* for values that are not {@link Comparable}, such as ROW and ARRAY values.
* Throws {@link BothValuesAreNullException} if both values are null. */
public static <T> int compareObjectsNullsLastForMergeJoin(@Nullable T v0,
@Nullable T v1, Comparator<? super T> comparator) {
if (v0 == null && v1 == null) {
throw new BothValuesAreNullException();
}
return v0 == null ? 1
: v1 == null ? -1
: comparator.compare(v0, v1);
}
```
In `Utilities`:
```
/** Compares two non-null ROW, ARRAY or MULTISET values element-wise. */
public static int compareComposite(Object v0, Object v1) {
return Functions.compareListItems(v0, v1);
}
/** Compares two ROW, ARRAY or MULTISET values element-wise, nulls first. */
public static int compareCompositeNullsFirst(@Nullable Object v0, @Nullable
Object v1) {
return v0 == v1 ? 0
: v0 == null ? -1
: v1 == null ? 1
: Functions.compareListItems(v0, v1);
}
/** Compares two ROW, ARRAY or MULTISET values element-wise, nulls last. */
public static int compareCompositeNullsLast(@Nullable Object v0, @Nullable
Object v1) {
return Functions.compareListItems(v0, v1); // already sorts nulls last
}
/** Merge join variant of {@link #compareCompositeNullsLast}: throws if both
* values are null, so that merge join does not match two null keys. */
public static int compareCompositeNullsLastForMergeJoin(@Nullable Object v0,
@Nullable Object v1) {
return EnumerableDefaults.compareObjectsNullsLastForMergeJoin(v0, v1,
Functions::compareListItems);
}
```
Here in `PhysTypeImpl.generateComparison`:
```
/** ROW and ARRAY sort keys are not Comparable; compare them element-wise,
* as the single-key sort path does. */
private static Expression generateComparison(RelDataType type, String method,
Expression left, Expression right, @Nullable Expression collator) {
if (isComposite(type)) {
final String compositeMethod;
switch (method) {
case "compare":
compositeMethod = "compareComposite";
break;
case "compareNullsFirst":
compositeMethod = "compareCompositeNullsFirst";
break;
case "compareNullsLast":
compositeMethod = "compareCompositeNullsLast";
break;
case "compareNullsLastForMergeJoin":
compositeMethod = "compareCompositeNullsLastForMergeJoin";
break;
default:
throw new AssertionError("unexpected comparison method: " + method);
}
return Expressions.call(Utilities.class, compositeMethod, left, right);
}
return Expressions.call(Utilities.class, method,
Expressions.list(left, right).appendIfNotNull(collator));
}
```
--
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]
