Uh oh!
There was an error while loading. Please reload this page.
Add partition by constructs in window functions and modify logical planning - #501
Conversation
Codecov Report
@@ Coverage Diff @@## master #501 +/- ##
==========================================
- Coverage 76.09% 76.03% -0.07%
==========================================
Files 157 157 Lines 26913 26990 +77 ==========================================
+ Hits 20480 20521 +41 - Misses 6433 6469 +36
Continue to review full report at Codecov.
|
partition by constructs and modify logical planningpartition by constructs and modify logical planningpartition by constructs in window functions and modify logical planning| let sql = "SELECT order_id, MAX(qty) OVER (PARTITION BY order_id) from orders"; | ||
| let expected = "\ | ||
| Projection: #order_id, #MAX(qty)\ | ||
| \n WindowAggr: windowExpr=[[MAX(#qty)]] partitionBy=[]\ |
There was a problem hiding this comment.
It seems like order_id should appear in the partitionBy list
There was a problem hiding this comment.
Let me remove the list printing altogether for now to keep parity with psql
I can always add it back later but for now the partition information is already captured by the child sort by plan
b6c6023 to
c2bfd60CompareUh oh!
There was an error while loading. Please reload this page.
alamb
commented
Jun 7, 2021
This is on my review queue for tomorrow |
alamb
left a comment
There was a problem hiding this comment.
Thanks @jimexist -- I think this PR looks reasonable. I had two suggestions I think are worth considering (handling errors in planner and including PARTITION BY in the explain plan)
I think it would also be fine to make those changes in a follow on PR if that would be easier for you
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| .map(|window_frame| window_frame.clone().try_into()) | ||
| .transpose()?; | ||
| let fun = window_functions::WindowFunction::from_str(&name); | ||
| if let Ok(window_functions::WindowFunction::AggregateFunction( |
There was a problem hiding this comment.
I may be misreading this, but it looks like the error message may have gotten lost.
Stylistically, this might be cleaner using a match rather than an if/else chain (and the compiler will tell you if you missed a case) something like
let fun = window_functions::WindowFunction::from_str(&name)?;// note question markuse window_functions::WindowFunction::*;match fun {AggregateFunction(aggregate_fun) =>{ .. code .. },BuiltInWindowFunction(window_fun) =>{ .. code .. },}Perhaps?
There was a problem hiding this comment.
it's a good point. i was previously thinking maybe deferring the else the subsequent parsing part but then it's with over clause so the allowed listed of function names shall be known to be limited.
There was a problem hiding this comment.
It is cool now to see the compiler ensuring all cases are covered.
| format!("{:?}", err) | ||
| ); | ||
| fn over_partition_by() { | ||
| let sql = "SELECT order_id, MAX(qty) OVER (PARTITION BY order_id) from orders"; |
There was a problem hiding this comment.
I think including the PARTITION BY information somewhere in this plan would be valuable -- maybe it could be added to WindowExpr formatting?
I may be missing a reason to not include it as well
There was a problem hiding this comment.
i think it can be added in a subsequent PR, where i can revisit all the planning related printing (to be less verbose perhaps)
Which issue does this PR close?
Add partition by constructs and modify logical planning.
Partly contributes to #299
Based on #492 so review that first
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?