Rewrite exotic LAST_VALUE/FIRST_VALUE to self-reference.#16063
Rewrite exotic LAST_VALUE/FIRST_VALUE to self-reference.#16063soumyava merged 7 commits intoapache:masterfrom
Conversation
* rewrite `LAST_VALUE(x) OVER (ORDER BY y)` to `LAG(x,0) OVER (ORDER BY y)` * not directly to `x` because some queries get unplannable that way * restrict `NTILE` from framing - as its not supported * add test to ensure that all of the `KNOWN_WINDOW_FNS`'s framing is accounted for
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java
Fixed
Show fixed
Hide fixed
| { | ||
| public RewriteFirstValueLastValueRule() | ||
| { | ||
| super(operand(RelNode.class, any())); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation
| { | ||
| public RewriteFirstValueLastValueRule() | ||
| { | ||
| super(operand(RelNode.class, any())); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java
Show resolved
Hide resolved
| { | ||
| // could remove `last_value( x ) over ( .... order by y )` | ||
| // best would be to: return over.getOperands().get(0); | ||
| // however that make some queries too good |
There was a problem hiding this comment.
What does this comment mean by making queries too good ?
There was a problem hiding this comment.
the best would be to just:
return over.getOperands().get(0);
however that will lead to plans which become unplannable as the query changes to be a window query directly on a dataset - and right now that's not allowed.
to not alter that for now: I've choosen to retain the Window function in this case but rewrite it to something which just references the current_row's value via LAG(x,0) instead.
sql/src/main/java/org/apache/druid/sql/calcite/rule/RewriteFirstValueLastValueRule.java
Show resolved
Hide resolved
| static { | ||
| NullHandling.initializeForTests(); | ||
| } | ||
| ) || developerIDEdetected(); |
There was a problem hiding this comment.
Why is this change needed ? Is there a special handling only for eclipse ?
There was a problem hiding this comment.
not closely related to this patch - I just had to enable this option every time I was updating results ....rather than doing that I would prefer to have it enabled for all IDE runs without having to realize that it needs to set; set it and re-run the test.
soumyava
left a comment
soumyava
left a comment
There was a problem hiding this comment.
LGTM ! Thanks for putting in the explanation as the javadocs. If not done recently, please merge with master once and then we can merge this in
3 participants
LAST_VALUE(x) OVER (ORDER BY y)toLAG(x,0) OVER (ORDER BY y)LAST_VALUElogic has no idea about theframeitself; and doesn't care about theorder byxbecause some queries get unplannable that wayNTILEfrom framing - as its not supportedKNOWN_WINDOW_FNS's framing is accounted forFixes #XXXX.
Description
Fixed the bug ...
Renamed the class ...
Added a forbidden-apis entry ...
Release note
Key changed/added classes in this PR
MyFooOurBarTheirBazThis PR has: