Remove Option from window frame #4516
Conversation
github-actions
bot
added
core
|
This is mostly a refactor PR that simplifies downstream window-processing code by handling empty (or default) window frames only once at the beginning. I already reviewed it and it LGTM. Let us know if there are any concerns; we will be happy to address them and/or improve the code further. |
There was a problem hiding this comment.
This is a nice change -- thank you @mustafasrepo and @metesynnada
I found https://github.com/apache/arrow-datafusion/pull/4516/files?w=1 easier to review
cc @Ted-Jiang as I believe you are working on window function related things in Ballista
| @@ -53,25 +53,25 @@ async fn csv_query_window_with_partition_by() -> Result<()> { | |||
| register_aggregate_csv(&ctx).await?; | |||
| let sql = "select \ | |||
| c9, \ | |||
There was a problem hiding this comment.
these sure look nicer -- BTW if you are looking for easier to maintain tests, you may want to check out the new sqllogictests runner we are working on
| start_bound: WindowFrameBound::Preceding(ScalarValue::Utf8(None)), | ||
| end_bound: WindowFrameBound::CurrentRow, | ||
| impl WindowFrame { | ||
| /// Creates a new, default window frame (with the meaning of default depending on whether the |
There was a problem hiding this comment.
Thank you for the clear comments 👍
|
Benchmark runs are scheduled for baseline = 5bb9a18 and contender = b6146b9. b6146b9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
| impl WindowFrame { | ||
| /// Creates a new, default window frame (with the meaning of default depending on whether the | ||
| /// frame contains an `ORDER BY` clause. | ||
| pub fn new(has_order_by: bool) -> Self { |
There was a problem hiding this comment.
Thanks for this fix, i also want to change this default behavior.
| oneof window_frame { | ||
| WindowFrame frame = 8; | ||
| } | ||
| WindowFrame window_frame = 8; |
Which issue does this PR close?
Closes #.
Rationale for this change
After parsing the window expression. If window frame is
None, we can construct its equivalent (in terms of behavior).For instance when someone enters the query
OVER(ORDER BY a). It is equivalent toOVER(ORDER BY a) RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW. Similarly, for the queryOVER(PARTITION BY a)its equivalent isOVER(PARTITION BY a ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING).After parsing the necessary information, we can construct the corresponding window frame. Hence in the downstream pipeline we don't need to use Option for
window frame.What changes are included in this PR?
This PR constructs an equivalent window frame when window frame is
Noneafter sql parsing. And at the rest of the pipeline uses equivalent window frame.Also after equivalent window frame construction default name of the window result column changes. Hence this PR includes changes in the column names of the expected results (I have added aliases to make tests future-proof also). Large bulk of the changes comes from these changes.
Are these changes tested?
Existing tests should work
Are there any user-facing changes?
api change