add `order by` construct in window function and logical plans by jimexist · Pull Request #463 · apache/datafusion

jimexist · 2021-06-01T06:31:30Z

Which issue does this PR close?

add sort_by construct in window function and logical plans.

Related and partly implements #360

Rationale for this change

Implementing order by constructs and its logical planning by following a similar approach to PostgreSQL.

Coming up: adding window frame unit and type defs, and allow window functions to operate differently depending on RANGE or ROW window frame spec.

What changes are included in this PR?

update to ballista proto
add logical plan for window functions with order by
add unit test

Are there any user-facing changes?

codecov-commenter · 2021-06-02T04:41:14Z

Codecov Report

Merging #463 (febc427) into master (c3fc0c7) will decrease coverage by 0.00%.
The diff coverage is 74.20%.

@@            Coverage Diff             @@
##           master     #463      +/-   ##
==========================================
- Coverage   75.84%   75.83%   -0.01%     
==========================================
  Files         153      153              
  Lines       25876    26078     +202     
==========================================
+ Hits        19626    19777     +151     
- Misses       6250     6301      +51

Impacted Files	Coverage Δ
...sta/rust/core/src/serde/logical_plan/from_proto.rs	`35.96% <0.00%> (-0.26%)`	⬇️
...ta/rust/core/src/serde/physical_plan/from_proto.rs	`38.79% <0.00%> (-1.00%)`	⬇️
datafusion/src/optimizer/utils.rs	`47.51% <0.00%> (-2.49%)`	⬇️
datafusion/src/physical_plan/planner.rs	`80.32% <0.00%> (-0.14%)`	⬇️
...lista/rust/core/src/serde/logical_plan/to_proto.rs	`62.48% <16.66%> (+0.07%)`	⬆️
datafusion/src/logical_plan/plan.rs	`81.06% <75.00%> (+0.43%)`	⬆️
datafusion/src/optimizer/projection_push_down.rs	`98.46% <87.50%> (+<0.01%)`	⬆️
datafusion/src/sql/utils.rs	`64.92% <91.26%> (+17.02%)`	⬆️
datafusion/src/logical_plan/expr.rs	`84.60% <91.66%> (+0.07%)`	⬆️
datafusion/src/sql/planner.rs	`84.37% <97.87%> (+0.26%)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3fc0c7...febc427. Read the comment docs.

jimexist · 2021-06-02T07:40:16Z

ballista/rust/core/proto/ballista.proto

 message WindowNode {
  LogicalPlanNode input = 1;
  repeated LogicalExprNode window_expr = 2;
-  repeated LogicalExprNode partition_by_expr = 3;


turns out these are no longer useful

alamb

I reviewed the code and tests carefully. Nice work @jimexist -- this is looking great.

alamb · 2021-06-02T20:44:22Z

ballista/rust/core/proto/ballista.proto

    // udaf = 3
  }
  LogicalExprNode expr = 4;
+  // repeated LogicalExprNode partition_by = 5;


we can probably just delete the old fields in the protobuf files -- I suspect no one is using them in a way that requires backwards compatibility

these commented out ones are not old, they are reminders of future fields.

alamb · 2021-06-02T20:48:11Z

datafusion/src/optimizer/utils.rs

-            fun: fun.clone(),
-            args: expressions.to_vec(),
-        }),
+        Expr::WindowFunction { fun, .. } => {


this pattern is kind of ugly -- at some point I would love to rewrite all this in terms of the expression visitors. Some day (TM) lol

alamb · 2021-06-02T20:49:27Z

datafusion/src/physical_plan/planner.rs

                        self.create_physical_expr(e, physical_input_schema, ctx_state)
                    })
                    .collect::<Result<Vec<_>>>()?;
+                // if !order_by.is_empty() {


Why is this commented out? It seems a better idea to generate an error than to silently error out to me

i would bring them back when it comes to implementing exec plan for sort, but maybe later

datafusion/src/sql/planner.rs

alamb · 2021-06-02T20:53:09Z

datafusion/src/sql/planner.rs

-                        && window.order_by.is_empty()
-                        && window.window_frame.is_none()
-                    {
+                    if window.partition_by.is_empty() && window.window_frame.is_none() {


I don't understand the check for partition_by.is_empty() and window_frame.is_none() -- wouldn't we want to aways process the window clause?

they need to be empty otherwise it goes to the unsupported error clause which is needed to guard unintended usage.

alamb · 2021-06-02T20:55:48Z

datafusion/src/sql/planner.rs

+        quick_test(sql, expected);
+    }
+
+    /// psql result


Thank you for including the postgres results

alamb · 2021-06-02T20:58:20Z

datafusion/src/sql/planner.rs

+    ///                     ->  Seq Scan on orders  (cost=0.00..20.00 rows=1000 width=8)
+    /// ```
+    ///
+    /// FIXME: for now we are not detecting prefix of sorting keys in order to save one sort exec phase


FWIW I think the FIXME is fine to do later -- let's get the functionality (with tests) working first and then we can optimize afterwards.

lots of room for optimization. there are several considerations when it comes to:

how to compute order by and partition by and re-order them to optimize

how to compute window aggregations given a possibility of either a ever-growing window or a shifting window (that can shrink and expand, depending on # of rows or the absolute values)

for 1, e.g. with max(a) over (partition by b order by c) you can either:

hash partition by b, merge, and then sort by c

sort by (b, c) so it's easier to implement but you lose parallelism here

for 2, for ever growing window, accumulative scan can be used, but for shrinking or shifting window, vec dequeue can be used, but also there's segment tree...

alamb · 2021-06-02T21:00:01Z

datafusion/src/sql/planner.rs

        );
    }

+    /// psql result


I don't think it really matters, but the max, min and avg window functions don't actually depend on order (and so in theory all of the sorts here could be optimized away.

they do. with order by they compute accumulative sum/avg/max/min, not a full partition one.

alamb · 2021-06-02T21:04:14Z

datafusion/src/sql/utils.rs

+                fun: WindowFunction::AggregateFunction(AggregateFunction::Max),
+                args: vec![col("name")],
+                order_by: vec![
+                    Expr::Sort {


FYI you can use the sort method here for less verbosity if you want: https://docs.rs/datafusion/4.0.0/datafusion/logical_plan/enum.Expr.html#method.sort

So something like order_by: vec![col("age").sort(true, true)]

let sort_expr = col("foo").sort(true, true); // SORT ASC NULLS_FIRST

thanks for the reminder - i plan to optimize this in subsequent PRs - as there would be more to comp

Dandandan · 2021-06-03T16:12:50Z

datafusion/src/sql/planner.rs

+        \n    WindowAggr: windowExpr=[[MAX(#qty)]] partitionBy=[]\
+        \n      Sort: #qty ASC NULLS FIRST, #order_id ASC NULLS FIRST\
+        \n        WindowAggr: windowExpr=[[MIN(#qty)]] partitionBy=[]\
+        \n          Sort: #order_id ASC NULLS FIRST, #qty ASC NULLS FIRST\


Might be good to remove the defaults (i.e. nulls first)

alamb · 2021-06-03T21:18:50Z

I am merging this one in so that @jimexist can continue with a minimum number of PRs open. Thanks again for keeping the 🚋 rolling!

jimexist force-pushed the add-sort-in-window branch 4 times, most recently from faa28c6 to c4f2779 Compare June 2, 2021 03:04

jimexist force-pushed the add-sort-in-window branch 2 times, most recently from 1684b78 to 177506a Compare June 2, 2021 07:13

jimexist marked this pull request as ready for review June 2, 2021 07:14

jimexist mentioned this pull request Jun 2, 2021

Support window functions with order by #360

Closed

2 tasks

jimexist changed the title ~~Add sort in window functions~~ add sort_by construct in window function and logical plans Jun 2, 2021

jimexist force-pushed the add-sort-in-window branch from 5aa2b4e to cc51a62 Compare June 2, 2021 07:38

jimexist commented Jun 2, 2021

View reviewed changes

jimexist force-pushed the add-sort-in-window branch from cc51a62 to 3261152 Compare June 2, 2021 08:10

jimexist changed the title ~~add sort_by construct in window function and logical plans~~ add order by construct in window function and logical plans Jun 2, 2021

alamb approved these changes Jun 2, 2021

View reviewed changes

fix window expression with alias

76e7e6d

jimexist force-pushed the add-sort-in-window branch from febc427 to 76e7e6d Compare June 3, 2021 05:50

jimexist mentioned this pull request Jun 3, 2021

Add window frame constructs #492

Closed

Dandandan approved these changes Jun 3, 2021

View reviewed changes

Dandandan reviewed Jun 3, 2021

View reviewed changes

alamb merged commit e82d053 into apache:master Jun 3, 2021

jimexist deleted the add-sort-in-window branch June 4, 2021 00:04

jimexist mentioned this pull request Jun 5, 2021

Add window frame constructs - alternative #506

Merged

houqp added ballista enhancement New feature or request labels Jul 30, 2021

Conversation

jimexist commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Jun 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jimexist commented Jun 1, 2021 •

edited

Loading

codecov-commenter commented Jun 2, 2021 •

edited

Loading