mihaibudiu commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1945587325
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer>
mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame
frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
+ // rewrite the Sort as Aggregate using MIN/MAX function.
+ // Example:
+ // Sort(sort0=[$0], dir0=[ASC], fetch=[1])
+ // input
+ // Rewrite to:
+ // Aggregate(group=(corVar), agg=[min($0))
+ //
+ // Note: MIN/MAX is not strictly equivalent to LIMIT 1. When the input has
0 rows,
+ // MIN/MAX returns NULL, while LIMIT 1 returns 0 rows.
+ // However, in the decorrelate, we add correlated variables to the group
list
+ // to ensure equivalence when Correlate is transformed to Join. When the
group list
+ // is non-empty, MIN/MAX will also return 0 rows if input with 0
rows.(This behavior
Review Comment:
"if the input has 0 rows."
I think that the references to the other calcite issue should not be in this
comment - they will become obsolete if the issue is closed, and they don't
really clarify what is going on here.
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer>
mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame
frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
+ // rewrite the Sort as Aggregate using MIN/MAX function.
+ // Example:
+ // Sort(sort0=[$0], dir0=[ASC], fetch=[1])
+ // input
+ // Rewrite to:
+ // Aggregate(group=(corVar), agg=[min($0))
+ //
+ // Note: MIN/MAX is not strictly equivalent to LIMIT 1. When the input has
0 rows,
+ // MIN/MAX returns NULL, while LIMIT 1 returns 0 rows.
+ // However, in the decorrelate, we add correlated variables to the group
list
+ // to ensure equivalence when Correlate is transformed to Join. When the
group list
+ // is non-empty, MIN/MAX will also return 0 rows if input with 0
rows.(This behavior
+ // also leads to the issue described in [CALCITE-5743]) So in this case,
the transformation
+ // is legal.
+ if (sort.getCollation().getFieldCollations().size() == 1
+ && sort.getRowType().getFieldCount() == 1
+ && !frame.corDefOutputs.isEmpty()) {
+ RelFieldCollation collation =
Util.first(sort.getCollation().getFieldCollations());
+
+ SqlAggFunction aggFunction;
+ switch (collation.getDirection()) {
+ case ASCENDING:
+ case STRICTLY_ASCENDING:
+ aggFunction = SqlStdOperatorTable.MIN;
+ break;
+ case DESCENDING:
+ case STRICTLY_DESCENDING:
+ aggFunction = SqlStdOperatorTable.MAX;
+ break;
+ default:
+ return null;
+ }
+
+ final int newIdx =
requireNonNull(frame.oldToNewOutputs.get(collation.getFieldIndex()));
+ RelBuilder.AggCall aggCall = relBuilder.push(frame.r)
Review Comment:
I am wondering: sorting has collation, but min/max do not.
For example, sorting can specify what happens to nulls.
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer>
mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame
frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
Review Comment:
the check for fetch == 1 is not here (so you can mention that it's enforced
by the caller).
Moreover, for symmetry, should this comment be within the if branch?
--
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]
