mihaibudiu commented on code in PR #4633:
URL: https://github.com/apache/calcite/pull/4633#discussion_r2629011884
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -574,16 +575,30 @@ protected RexNode removeCorrelationExpr(
}
if (isCorVarDefined && (rel.fetch != null || rel.offset != null)) {
- if (rel.fetch != null
- && rel.offset == null
- && RexLiteral.intValue(rel.fetch) == 1) {
- return decorrelateFetchOneSort(rel, frame);
- }
- // Can not decorrelate if the sort has per-correlate-key attributes like
- // offset or fetch limit, because these attributes scope would change to
- // global after decorrelation. They should take effect within the scope
- // of the correlation key actually.
- return null;
+ if (rel.offset == null
+ && rel.fetch != null
+ && RexLiteral.longValue(rel.fetch) == 0) {
Review Comment:
long is probably safe -- I don't expect more than 2^64 elements in a
collection very soon
bigInt would have been safest. See
https://issues.apache.org/jira/browse/CALCITE-7181 and the associated PR.
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -1044,30 +1059,7 @@ private static void shiftMapping(Map<Integer, Integer>
mapping, int startIndex,
return null;
}
- protected @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame
frame) {
- Frame aggFrame = decorrelateSortAsAggregate(sort, frame);
- if (aggFrame != null) {
- return aggFrame;
- }
- //
- // Rewrite logic:
- //
- // If sorted without offset and fetch = 1 (enforced by the caller),
rewrite the sort to be
- // Aggregate(group=(corVar.. , field..))
- // project(first_value(field) over (partition by corVar order by (sort
collation)))
- // input
- //
- // 1. For the original sorted input, apply the FIRST_VALUE window function
to produce
- // the result of sorting with LIMIT 1, and the same as the decorrelate
of aggregate,
- // add correlated variables in partition list to maintain semantic
consistency.
- // 2. To ensure that there is at most one row of output for
- // any combination of correlated variables, distinct for correlated
variables.
- // 3. Since we have partitioned by all correlated variables
- // in the sorted output field window, so for any combination of
correlated variables,
- // all other field values are unique. So the following two are
equivalent:
- // - group by corVar1, covVar2, field1, field2
- // - any_value(fields1), any_value(fields2) group by corVar1, covVar2
- // Here we use the first.
+ protected @Nullable Frame decorrelateSortWithRowNumber(Sort sort, final
Frame frame) {
Review Comment:
My comment was that protected functions are part of the public API, so they
cannot be modified (unless you change the major version). Perhaps the function
should have been private?
I think it's safer to leave the function unchanged and add a new function.
Does the function have no other callers except this one? In that case you
may need to add a description of a breaking change to the release notes:
"Function RelDecorrelator.decorrelateFetchOneSort is no longer called by the
decorrelator; instead consider using ..."
--
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]
