-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Preceding with negative indices in window function #15783
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seen the topic of the PR - didn't notice it was a draft and taken a quick look.... :)
int lowerOffset = figureOutOffset(group.lowerBound); | ||
int upperOffset = figureOutOffset(group.upperBound); | ||
if (group.lowerBound.isPreceding()) { | ||
lowerOffset = -lowerOffset; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of adding if
-s for both lower
/upper
- can we make these changes inside figureOutOffset
?
second = val + " FOLLOWING"; | ||
} | ||
throw InvalidInput.exception( | ||
"The first value of range in the window [%s] should be lesser than the second value [%s]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this check should be in DruidSqlValidator
; it will need similar things like #15746
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
1e826db
to
f8d283a
Compare
This pull request has been marked as stale due to 60 days of inactivity. |
This pull request/issue has been closed due to lack of activity. If you think that |
This aims to solve #15739 by introducing a change in the native query plan for Windowing query where a query like
will be planned in the
between 1 PRECEDING and 2 FOLLOWING
part asThis would allow us to support all the cases which postgres supports such as
This will also handle cases with a meaningful error where the upper offset is of a lesser value than the lower offset such as
This PR has: