kgyrtkirk commented on code in PR #15783:
URL: https://github.com/apache/druid/pull/15783#discussion_r1470595347
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java:
##########
@@ -358,12 +359,43 @@ public WindowFrame getWindowFrame()
if (group.lowerBound.isUnbounded() && group.upperBound.isUnbounded()) {
return WindowFrame.unbounded();
}
+ int lowerOffset = figureOutOffset(group.lowerBound);
+ int upperOffset = figureOutOffset(group.upperBound);
+ if (group.lowerBound.isPreceding()) {
+ lowerOffset = -lowerOffset;
+ }
Review Comment:
instead of adding `if`-s for both `lower`/`upper` - can we make these
changes inside `figureOutOffset` ?
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultFramedOnHeapAggregatable.java:
##########
@@ -75,7 +75,7 @@ public RowsAndColumns aggregateAll(
int lowerOffset = frame.getLowerOffset();
int upperOffset = frame.getUpperOffset();
- if (numRows < lowerOffset + upperOffset + 1) {
+ if (numRows < upperOffset - lowerOffset + 1) {
Review Comment:
I think you should also probably take a look at:
`WindowFrame#getLowerOffsetClamped`
and the callsites of that method in this file
by the way: I think we should retire the old `rows` processing logic and
leave that as well to the one handling the range stuff - it should be on-par in
performance; but could handle some edge cases a bit better.
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java:
##########
@@ -358,12 +359,43 @@ public WindowFrame getWindowFrame()
if (group.lowerBound.isUnbounded() && group.upperBound.isUnbounded()) {
return WindowFrame.unbounded();
}
+ int lowerOffset = figureOutOffset(group.lowerBound);
+ int upperOffset = figureOutOffset(group.upperBound);
+ if (group.lowerBound.isPreceding()) {
+ lowerOffset = -lowerOffset;
+ }
+ if (group.upperBound.isPreceding()) {
+ upperOffset = -upperOffset;
+ }
+ if (lowerOffset >= upperOffset) {
+ final String first, second;
+ int val;
+ if (group.lowerBound.isPreceding()) {
+ val = Math.abs(lowerOffset);
+ first = val + " PRECEDING";
+ } else {
+ val = Math.abs(upperOffset);
+ first = val + " FOLLOWING";
+ }
+ if (group.upperBound.isPreceding()) {
+ val = Math.abs(upperOffset);
+ second = val + " PRECEDING";
+ } else {
+ val = Math.abs(upperOffset);
+ second = val + " FOLLOWING";
+ }
+ throw InvalidInput.exception(
+ "The first value of range in the window [%s] should be lesser than
the second value [%s]",
Review Comment:
I think this check should be in `DruidSqlValidator` ; it will need similar
things like https://github.com/apache/druid/pull/15746
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
