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.
|
jimexist
changed the title
partition by constructs and modify logical planning
jimexist
changed the title
partition by constructs and modify logical planningpartition by constructs in window functions and modify logical planning
jimexist
force-pushed
the
add-partition-by
branch
from
1912ec6 to
8bf1ca2
Compare
datafusion/src/sql/planner.rs
Outdated
| 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
jimexist
force-pushed
the
add-partition-by
branch
2 times, most recently
from
b6c6023 to
c2bfd60
Compare
jimexist
force-pushed
the
add-partition-by
branch
from
67c3ac8 to
1a96fd7
Compare
|
This is on my review queue for tomorrow |
jimexist
force-pushed
the
add-partition-by
branch
from
1a96fd7 to
afd174d
Compare
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
datafusion/src/sql/planner.rs
Outdated
| .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 mark
use 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)
jimexist
force-pushed
the
add-partition-by
branch
from
8b07134 to
1b38ae2
Compare
jimexist
force-pushed
the
add-partition-by
branch
from
1b38ae2 to
b9d943e
Compare
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?