yashmayya commented on code in PR #14233:
URL: https://github.com/apache/pinot/pull/14233#discussion_r1802367928
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -209,23 +244,37 @@ private void validateWindows(Window window) {
}
private void validateWindowAggCallsSupported(Window.Group windowGroup) {
- for (int i = 0; i < windowGroup.aggCalls.size(); i++) {
- Window.RexWinAggCall aggCall = windowGroup.aggCalls.get(i);
+ for (Window.RexWinAggCall aggCall : windowGroup.aggCalls) {
SqlKind aggKind = aggCall.getKind();
Preconditions.checkState(SUPPORTED_WINDOW_FUNCTION_KIND.contains(aggKind),
String.format("Unsupported Window function kind: %s. Only
aggregation functions are supported!", aggKind));
}
}
private void validateWindowFrames(Window.Group windowGroup) {
- // Has ROWS only aggregation call kind (e.g. ROW_NUMBER)?
- boolean isRowsOnlyTypeAggregateCall =
isRowsOnlyAggregationCallType(windowGroup.aggCalls);
+ // Has rank based aggregation call kind (e.g. ROW_NUMBER, RANK, DENSE_RANK)
+ boolean hasRankBasedAgg = hasRankBasedAgg(windowGroup.aggCalls);
+
+ // FOLLOWING lower bound or PRECEDING upper bound is not allowed, and
won't reach here
+ RexWindowBound lowerBound = windowGroup.lowerBound;
+ assert !lowerBound.isFollowing();
+ RexWindowBound upperBound = windowGroup.upperBound;
+ assert !upperBound.isPreceding();
+ boolean hasBound = (lowerBound.isPreceding() && !lowerBound.isUnbounded())
|| (upperBound.isFollowing()
+ && !upperBound.isUnbounded());
+
+ if (hasRankBasedAgg) {
+ Preconditions.checkState(!hasBound, "Window frame is not supported for
ROW_NUMBER, RANK, DENSE_RANK functions");
Review Comment:
Postgres seems to allow any window frame for these functions, but the result
is the same regardless of the defined window frame (i.e., following the
behavior of `ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING`).
Although I think the behavior here of imposing the unbounded window frame
condition is probably better to avoid user confusion.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -209,23 +244,37 @@ private void validateWindows(Window window) {
}
private void validateWindowAggCallsSupported(Window.Group windowGroup) {
- for (int i = 0; i < windowGroup.aggCalls.size(); i++) {
- Window.RexWinAggCall aggCall = windowGroup.aggCalls.get(i);
+ for (Window.RexWinAggCall aggCall : windowGroup.aggCalls) {
SqlKind aggKind = aggCall.getKind();
Preconditions.checkState(SUPPORTED_WINDOW_FUNCTION_KIND.contains(aggKind),
String.format("Unsupported Window function kind: %s. Only
aggregation functions are supported!", aggKind));
}
}
private void validateWindowFrames(Window.Group windowGroup) {
- // Has ROWS only aggregation call kind (e.g. ROW_NUMBER)?
- boolean isRowsOnlyTypeAggregateCall =
isRowsOnlyAggregationCallType(windowGroup.aggCalls);
+ // Has rank based aggregation call kind (e.g. ROW_NUMBER, RANK, DENSE_RANK)
+ boolean hasRankBasedAgg = hasRankBasedAgg(windowGroup.aggCalls);
+
+ // FOLLOWING lower bound or PRECEDING upper bound is not allowed, and
won't reach here
+ RexWindowBound lowerBound = windowGroup.lowerBound;
+ assert !lowerBound.isFollowing();
+ RexWindowBound upperBound = windowGroup.upperBound;
+ assert !upperBound.isPreceding();
+ boolean hasBound = (lowerBound.isPreceding() && !lowerBound.isUnbounded())
|| (upperBound.isFollowing()
+ && !upperBound.isUnbounded());
+
+ if (hasRankBasedAgg) {
+ Preconditions.checkState(!hasBound, "Window frame is not supported for
ROW_NUMBER, RANK, DENSE_RANK functions");
+ }
+ if (!windowGroup.isRows) {
+ Preconditions.checkState(!hasBound, "Bounded RANGE window frame is
currently not supported");
+ }
// For Phase 1 only the default frame is supported
- Preconditions.checkState(!windowGroup.isRows ||
isRowsOnlyTypeAggregateCall,
+ Preconditions.checkState(!windowGroup.isRows || hasRankBasedAgg,
Review Comment:
If it is a rank based aggregation, we already check above that the window
frame is unbounded (whether range or rows). So this condition of
`!windowGroup.isRows || hasRankBasedAgg` doesn't really seem to make sense here
unless I'm missing something?
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotWindowExchangeNodeInsertRule.java:
##########
@@ -234,8 +283,13 @@ private void validateWindowFrames(Window.Group
windowGroup) {
}
}
- private boolean
isRowsOnlyAggregationCallType(ImmutableList<Window.RexWinAggCall> aggCalls) {
- return aggCalls.stream().anyMatch(aggCall ->
aggCall.getKind().equals(SqlKind.ROW_NUMBER));
+ private boolean hasRankBasedAgg(ImmutableList<Window.RexWinAggCall>
aggCalls) {
+ for (Window.RexWinAggCall aggCall : aggCalls) {
+ if (RANK_BASED_FUNCTION_KIND.contains(aggCall.getKind())) {
+ return true;
+ }
+ }
+ return false;
Review Comment:
I just discovered that we probably don't need any of the `hasRankBasedAgg`
checks at all because Calcite itself ensures that functions like `RANK` /
`DENSE_RANK` / `ROW_NUMBER` aren't used with a custom `ROWS` / `RANGE` window
frame -
https://github.com/apache/calcite/blob/df2328b7c2834cf7ffbf7305fe2c7aec5d07fe64/core/src/main/java/org/apache/calcite/sql/SqlWindow.java#L669-L674.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
