mihaibudiu commented on code in PR #4633:
URL: https://github.com/apache/calcite/pull/4633#discussion_r2624170646
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -1045,29 +1037,31 @@ private static void shiftMapping(Map<Integer, Integer>
mapping, int startIndex,
}
protected @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame
frame) {
- Frame aggFrame = decorrelateSortAsAggregate(sort, frame);
- if (aggFrame != null) {
- return aggFrame;
+ if (sort.offset == null
Review Comment:
the name of this function is no longer appropriate, but it is protected, so
I am not sure you can change it.
you may need to create a new one.
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -1045,29 +1037,31 @@ private static void shiftMapping(Map<Integer, Integer>
mapping, int startIndex,
}
protected @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame
frame) {
- Frame aggFrame = decorrelateSortAsAggregate(sort, frame);
- if (aggFrame != null) {
- return aggFrame;
+ if (sort.offset == null
+ && sort.fetch != null
+ && RexLiteral.intValue(sort.fetch) == 0) {
Review Comment:
is 0 even allowed?
Also, intValue seems unsafe; there have been some bug fixes about this in
the past.
BigInt is probably safest.
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -1099,29 +1093,60 @@ private static void shiftMapping(Map<Integer, Integer>
mapping, int startIndex,
for (RelDataTypeField field : sort.getRowType().getFieldList()) {
final int newIdx =
requireNonNull(frame.oldToNewOutputs.get(field.getIndex()));
-
- RelBuilder.AggCall aggCall =
- relBuilder.aggregateCall(SqlStdOperatorTable.FIRST_VALUE,
- RexInputRef.of(newIdx, fieldList));
-
- // Convert each field from the sorted output to a window function that
partitions by
- // correlated variables, orders by the collation, and return the
first_value.
- RexNode winCall = aggCall.over()
- .orderBy(sortExprs)
- .partitionBy(corVarProjects.leftList())
- .toRex();
mapOldToNewOutputs.put(newProjExprs.size(), newProjExprs.size());
- newProjExprs.add(winCall, field.getName());
+ newProjExprs.add(RexInputRef.of(newIdx, fieldList), field.getName());
}
newProjExprs.addAll(corVarProjects);
- RelNode result = relBuilder.push(frame.r)
- .project(newProjExprs.leftList(), newProjExprs.rightList())
- .distinct().build();
+ relBuilder.push(frame.r);
+
+ RexNode rowNumberCall =
relBuilder.aggregateCall(SqlStdOperatorTable.ROW_NUMBER)
+ .over()
+ .partitionBy(corVarProjects.leftList())
+ .orderBy(sortExprs)
+ .let(c -> c.rowsBetween(RexWindowBounds.UNBOUNDED_PRECEDING,
RexWindowBounds.CURRENT_ROW))
+ .toRex();
+ newProjExprs.add(rowNumberCall, "rn"); // Add the row number column
+ relBuilder.project(newProjExprs.leftList(), newProjExprs.rightList());
+
+ List<RexNode> conditions = new ArrayList<>();
+ if (sort.offset != null) {
+ RexNode greaterThenLowerBound =
+ relBuilder.call(
+ SqlStdOperatorTable.GREATER_THAN,
+ relBuilder.field(newProjExprs.size() - 1),
+ sort.offset);
+ conditions.add(greaterThenLowerBound);
+ }
+ if (sort.fetch != null) {
+ RexNode upperBound = sort.offset == null
+ ? sort.fetch
+ : relBuilder.call(SqlStdOperatorTable.PLUS, sort.offset, sort.fetch);
+ RexNode lessThenOrEqualUpperBound =
+ relBuilder.call(
+ SqlStdOperatorTable.LESS_THAN_OR_EQUAL,
+ relBuilder.field(newProjExprs.size() - 1),
+ upperBound);
+ conditions.add(lessThenOrEqualUpperBound);
+ }
+
+ RelNode result;
+ if (!conditions.isEmpty()) {
+ result = relBuilder.filter(conditions).build();
+ } else {
+ result = relBuilder.build();
+ }
return register(sort, result, mapOldToNewOutputs, corDefOutputs);
}
protected @Nullable Frame decorrelateSortAsAggregate(Sort sort, final Frame
frame) {
+ if (sort.offset != null) {
+ return null;
+ }
+ if (sort.fetch == null || RexLiteral.intValue(sort.fetch) != 1) {
Review Comment:
same about intValue
##########
core/src/test/java/org/apache/calcite/sql2rel/RelDecorrelatorTest.java:
##########
@@ -584,4 +584,203 @@ public static Frameworks.ConfigBuilder config() {
+ " LogicalTableScan(table=[[scott, DEPT]])\n";
assertThat(decorrelatedNoRules, hasTree(planDecorrelatedNoRules));
}
+
+ @Test void testDecorrelateCorrelatedOrderByLimitToRowNumber() {
Review Comment:
Frankly, there are already lots of existing tests which cover these changes,
but since you wrote them, let's keep them
##########
core/src/test/java/org/apache/calcite/sql2rel/RelDecorrelatorTest.java:
##########
@@ -584,4 +584,203 @@ public static Frameworks.ConfigBuilder config() {
+ " LogicalTableScan(table=[[scott, DEPT]])\n";
assertThat(decorrelatedNoRules, hasTree(planDecorrelatedNoRules));
}
+
+ @Test void testDecorrelateCorrelatedOrderByLimitToRowNumber() {
Review Comment:
on the other hand, I don't recall seeing the MIN/MAX case, is it covered?
--
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]
