Collapse sort into window expr and do sort within logical phase #571
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #571 +/- ##
==========================================
+ Coverage 76.02% 76.10% +0.07%
==========================================
Files 156 156
Lines 27063 27145 +82
==========================================
+ Hits 20575 20658 +83
+ Misses 6488 6487 -1
Continue to review full report at Codecov.
|
jimexist
force-pushed
the
window-aggr-collapse-sort
branch
3 times, most recently
from
906b62d to
f4341d4
Compare
jimexist
changed the title
|
performance comparison: consider that the test data are generated per run, i would ignore 5%-ish variations. |
|
with 1million records and 8k batch size |
jimexist
force-pushed
the
window-aggr-collapse-sort
branch
2 times, most recently
from
eef960a to
2f83560
Compare
|
this will make way for #569 |
|
I am sorry I am behind on reviews in DataFusion -- I plan to work through the backlog tomorrow |
jimexist
force-pushed
the
window-aggr-collapse-sort
branch
2 times, most recently
from
7845ac4 to
e48fdd3
Compare
houqp
reviewed
Jun 23, 2021
jimexist
force-pushed
the
window-aggr-collapse-sort
branch
2 times, most recently
from
ce6a559 to
7553b89
Compare
alamb
approved these changes
Jun 23, 2021
|
|
||
| // at this moment we are guaranteed by the logical planner | ||
| // to have all the window_expr to have equal sort key |
There was a problem hiding this comment.
Is the design that the planner will group all WindowExpr that have the same window (ORDER BY, PARTITION BY) into the same LogicalPlan:Window node? That sounds like a good plan to me 👍
It might (eventually) be worth validating that invariant here and asserting (or returning an Error` if the sort keys are not the same for multiple window exprs (mostly to catch potential bugs faster in the future)
There was a problem hiding this comment.
i can add the assertion later. yes the invariant is real.
| \n WindowAggr: windowExpr=[[MIN(#orders.qty)]]\ | ||
| \n Sort: #orders.order_id DESC NULLS FIRST\ | ||
| \n TableScan: orders projection=None"; | ||
| \n WindowAggr: windowExpr=[[MAX(#orders.qty) ORDER BY [#orders.order_id ASC NULLS FIRST]]]\ |
There was a problem hiding this comment.
Yeah this is cool to see two different WindowAggr nodes with different ORDER BY clauses. 👍
There was a problem hiding this comment.
rust uses timsort, and it shall be quick (to reverse an already sorted vec).
| @@ -1452,11 +1452,18 @@ impl fmt::Debug for Expr { | |||
| } | |||
| Expr::WindowFunction { | |||
| fun, | |||
| ref args, | |||
| args, | |||
| partition_by, | |||
There was a problem hiding this comment.
It seems like the partition_by and order_by more logically belong on the LogicalPlan::Window node if they can be shared across Expr::WindowFunction
There was a problem hiding this comment.
that's a separate construct not yet available in the sql parser.
see https://www.postgresql.org/docs/current/tutorial-window.html
When a query involves multiple window functions, it is possible to write out each one with a separate OVER clause, but this is duplicative and error-prone if the same windowing behavior is wanted for several functions. Instead, each windowing behavior can be named in a WINDOW clause and then referenced in OVER. For example:
SELECT sum(salary) OVER w, avg(salary) OVER w
FROM empsalary
WINDOW w AS (PARTITION BY depname ORDER BY salary DESC);There was a problem hiding this comment.
It seems like the partition_by and order_by more logically belong on the LogicalPlan::Window node if they can be shared across Expr::WindowFunction
for logically planned window function node it's not necessarily the case, because sort order is defined by the concatenations of partition_by and then order_by, but consider:
select max(c1) over (partition by c2 order by c3), max(c1) over (order by c2, c3) from test;both window functions will have the same sort key of [c2 asc nulls first, c3 asc nulls first] but they mean different things, and they might be logically planned together (meaning same pre-sorting happens) but not the same evaluation will happen
There was a problem hiding this comment.
I see -- this makes sense. Thank you for the clarification
Perhaps it is worth a comment (in a follow on PR) on the LogicalPlan window to this effect
|
Looks like a good step forward to me |
jimexist
force-pushed
the
window-aggr-collapse-sort
branch
from
7553b89 to
5b44dce
Compare
alamb
approved these changes
Jun 23, 2021
| @@ -1452,11 +1452,18 @@ impl fmt::Debug for Expr { | |||
| } | |||
| Expr::WindowFunction { | |||
| fun, | |||
| ref args, | |||
| args, | |||
| partition_by, | |||
There was a problem hiding this comment.
I see -- this makes sense. Thank you for the clarification
Perhaps it is worth a comment (in a follow on PR) on the LogicalPlan window to this effect
|
@jimexist there appears to be a build failure now: unless there are any other comments I'll plan to merge this tomorrow when the build issue gets fixed |
jimexist
force-pushed
the
window-aggr-collapse-sort
branch
from
5b44dce to
20c19ad
Compare
jimexist
commented
Jun 24, 2021
|
|
||
| let sort_keys = get_sort_keys(&window_expr[0]); | ||
| if window_expr.len() > 1 { | ||
| debug_assert!( |
alamb
approved these changes
Jun 24, 2021
|
|
||
| let sort_keys = get_sort_keys(&window_expr[0]); | ||
| if window_expr.len() > 1 { | ||
| debug_assert!( |
|
Thanks again @jimexist |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Based on #564 so review that first
Closes #573
Closes #526
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?