-
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
Strict window frame checks #15746
Strict window frame checks #15746
Conversation
* introduce check to ensure that window frame is supported * currently RANGE frames are only supported correctly if both endpoints are unbounded or current row * add `windowingStrictValidation` context key to provide a way to override the check
fyi: @317brian we need to document this context flag. |
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.
Suggest style improvement
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
Outdated
Show resolved
Hide resolved
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.
style suggestion
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java
Outdated
Show resolved
Hide resolved
Can we put some more context in the description as to what the issue with range we are facing and also a github issue with what needs to be fix in the long run so that we can retire this custom query context when the time comes |
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java
Fixed
Show fixed
Hide fixed
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.
LGTM. The checks will be updated once the fix for #15739 is merged in. The failure here is unrelated but we should merge master into it
introduce checks to ensure that window frame is supported added check to ensure that no expressions are set as bounds added logic to detect following/following like cases - described in Window function fails to demarcate if 2 following are used apache#15739 currently RANGE frames are only supported correctly if both endpoints are unbounded or current row Offset based window range support apache#15767 added windowingStrictValidation context key to provide a way to override the check (cherry picked from commit 8f5b752)
introduce checks to ensure that window frame is supported added check to ensure that no expressions are set as bounds added logic to detect following/following like cases - described in Window function fails to demarcate if 2 following are used apache#15739 currently RANGE frames are only supported correctly if both endpoints are unbounded or current row Offset based window range support apache#15767 added windowingStrictValidation context key to provide a way to override the check (cherry picked from commit 8f5b752)
introduce checks to ensure that window frame is supported added check to ensure that no expressions are set as bounds added logic to detect following/following like cases - described in Window function fails to demarcate if 2 following are used #15739 currently RANGE frames are only supported correctly if both endpoints are unbounded or current row Offset based window range support #15767 added windowingStrictValidation context key to provide a way to override the check (cherry picked from commit 8f5b752)
windowingStrictValidation
context key to provide a way to override the checkDescription
Fixed the bug ...
Renamed the class ...
Added a forbidden-apis entry ...
Release note
Key changed/added classes in this PR
MyFoo
OurBar
TheirBaz
This PR has: