Implement window functions with `order_by` clause by jimexist · Pull Request #520 · apache/datafusion

jimexist · 2021-06-07T06:48:17Z

Which issue does this PR close?

Closes #360

for now this pull request relies on arrow 4.3.0 to merge

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter · 2021-06-10T16:54:34Z

Codecov Report

Merging #520 (2650680) into master (d382854) will decrease coverage by 0.09%.
The diff coverage is 75.15%.

@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
- Coverage   76.09%   75.99%   -0.10%     
==========================================
  Files         156      156              
  Lines       27047    27036      -11     
==========================================
- Hits        20581    20547      -34     
- Misses       6466     6489      +23

Impacted Files	Coverage Δ
datafusion/src/physical_plan/window_functions.rs	`86.42% <ø> (+0.71%)`	⬆️
datafusion/src/sql/planner.rs	`84.85% <ø> (ø)`
datafusion/src/physical_plan/planner.rs	`79.35% <33.33%> (+1.81%)`	⬆️
datafusion/src/physical_plan/mod.rs	`76.72% <55.55%> (-2.37%)`	⬇️
datafusion/src/physical_plan/windows.rs	`82.57% <70.90%> (-3.90%)`	⬇️
...afusion/src/physical_plan/expressions/nth_value.rs	`78.78% <74.28%> (-11.69%)`	⬇️
datafusion/src/execution/context.rs	`92.04% <100.00%> (+0.03%)`	⬆️
...fusion/src/physical_plan/expressions/row_number.rs	`94.28% <100.00%> (+13.03%)`	⬆️
datafusion/src/physical_plan/hash_aggregate.rs	`86.54% <100.00%> (ø)`
datafusion/src/scalar.rs	`56.19% <100.00%> (ø)`
... and 8 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 d382854...2650680. Read the comment docs.

ballista/rust/core/Cargo.toml

alamb

This looks very nice @jimexist -- I went over the code and saw only goodness :)

All that this PR needs to be mergeable in my opinion is to reset the Cargo arrow* references (now that arrow 4.3.0 has been released)

ballista/rust/core/Cargo.toml

alamb · 2021-06-14T15:43:30Z

datafusion/src/execution/context.rs

+            4,
+        )
+        .await?;
+        // result in one batch, although e.g. having 2 batches do not change


alamb · 2021-06-14T15:46:31Z

datafusion/src/physical_plan/expressions/nth_value.rs

+        if num_rows == 0 {
+            return Ok(new_empty_array(value.data_type()));
+        }
+        let index: usize = match self.kind {


alamb · 2021-06-14T15:47:23Z

datafusion/src/physical_plan/expressions/nth_value.rs

+                value.len()
+            )));
+        }
+        if num_rows == 0 {


Could this function could ever be passed a 0 row input? This check isn't a problem I am just wondering if my mental model is correct.

this will be changed in later pull request

but you are right this would not be passed with 0 length input. this check is just being pedantic.

alamb · 2021-06-14T15:47:53Z

datafusion/src/physical_plan/expressions/nth_value.rs

-    fn test_i32_result(expr: Arc<NthValue>, expected: i32) -> Result<()> {
+    fn test_i32_result(expr: NthValue, expected: Vec<i32>) -> Result<()> {
        let arr: ArrayRef = Arc::new(Int32Array::from(vec![1, -2, 3, -4, 5, -6, 7, 8]));
+        let values = vec![arr];


This test change shows the nice refactoring

alamb · 2021-06-14T15:58:00Z

datafusion/src/physical_plan/mod.rs

-    /// return states with the same description as `state_fields`
-    fn create_accumulator(&self) -> Result<Box<dyn WindowAccumulator>>;
-
    /// expressions that are passed to the WindowAccumulator.


The WindowExpr trait is looking 👌

alamb · 2021-06-14T15:58:54Z

datafusion/src/physical_plan/window_functions.rs

+/// A window expression that is a built-in window function.
+///
+/// Note that unlike aggregation based window functions, built-in window functions normally ignore
+/// window frame spec, with th expression of first_value, last_value, and nth_value.


Suggested change

/// window frame spec, with th expression of first_value, last_value, and nth_value.

/// window frame spec, with the exception of first_value, last_value, and nth_value.

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

datafusion/src/physical_plan/windows.rs

+    /// peer based evaluation based on the fact that batch is pre-sorted given the sort columns
+    /// and then per partition point we'll evaluate the peer group (e.g. SUM or MAX gives the same
+    /// results for peers) and concatenate the results.
+    fn peer_based_evaluate(&self, batch: &RecordBatch) -> Result<ArrayRef> {


I don't understand the naming of peer here (rather than range_based_evaluate for example, to match with WindowFrameUnits::Range)

i will possibly change this naming in implementing #361 but for the moment, range and groups both evaluates with peers but rows evaluates based on rows on each scan.

since this is private function i guess i can leave the naming part for later changes.

Yes I think it is fine for now

alamb · 2021-06-14T16:04:46Z

datafusion/src/physical_plan/windows.rs

+        let len = value_range.end - value_range.start;
+        let values = values
+            .iter()
+            .map(|v| v.slice(value_range.start, len))


alamb · 2021-06-14T16:05:14Z

integration-tests/sqls/simple_window_ordered_aggregation.sql

+-- See the License for the specific language gOVERning permissions and
+-- limitations under the License.
+
+SELECT


Very nice 👍

jimexist · 2021-06-15T00:50:29Z

This looks very nice @jimexist -- I went over the code and saw only goodness :)

All that this PR needs to be mergeable in my opinion is to reset the Cargo arrow* references (now that arrow 4.3.0 has been released)

thank you for taking time to review. the changes to arrow references are now reverted.

jimexist · 2021-06-15T15:48:18Z

@alamb this pull request is ready now

alamb

Thanks @jimexist

alamb · 2021-06-16T10:25:05Z

datafusion/src/physical_plan/windows.rs

+    /// peer based evaluation based on the fact that batch is pre-sorted given the sort columns
+    /// and then per partition point we'll evaluate the peer group (e.g. SUM or MAX gives the same
+    /// results for peers) and concatenate the results.
+    fn peer_based_evaluate(&self, batch: &RecordBatch) -> Result<ArrayRef> {


Yes I think it is fine for now

…ons (apache#520) * Improve documentation about supported operators and expressions * Improve documentation about supported operators and expressions * more notes * Add more supported expressions * rename protobuf Negative to UnaryMinus for consistency * format * remove duplicate ASF header * SMJ not disabled by default * Update docs/source/user-guide/operators.md Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com> * Update docs/source/user-guide/operators.md Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com> * remove RLike --------- Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>

jimexist changed the title ~~Impl window order by~~ WIP Impl window order by Jun 7, 2021

jimexist force-pushed the impl-window-order-by branch 8 times, most recently from 1511b62 to 6cc7f96 Compare June 10, 2021 15:18

jimexist mentioned this pull request Jun 11, 2021

reuse datafusion physical planner in ballista building from protobuf #532

Merged

jimexist force-pushed the impl-window-order-by branch 4 times, most recently from 3d76e38 to 2a038e1 Compare June 13, 2021 09:31

jimexist changed the title ~~WIP Impl window order by~~ Implement window functions with order_by clause Jun 13, 2021

jimexist marked this pull request as ready for review June 13, 2021 10:12

jimexist commented Jun 13, 2021

View reviewed changes

ballista/rust/core/Cargo.toml Outdated Show resolved Hide resolved

jimexist force-pushed the impl-window-order-by branch 3 times, most recently from 81a834e to 7fbe3f0 Compare June 13, 2021 12:33

This was referenced Jun 13, 2021

implement lead and lag built-in window function #429

Merged

implement rank and dense rank window functions #555

Closed

Implement window functions with partition_by clause #558

Merged

alamb reviewed Jun 14, 2021

View reviewed changes

alamb added the datafusion label Jun 14, 2021

jimexist force-pushed the impl-window-order-by branch from 2650680 to ce4e262 Compare June 15, 2021 00:12

add window function implementation with order_by clause

9f6a56b

jimexist force-pushed the impl-window-order-by branch from ce4e262 to 9f6a56b Compare June 15, 2021 00:52

alamb approved these changes Jun 16, 2021

View reviewed changes

alamb merged commit 51e5445 into apache:master Jun 16, 2021

jimexist deleted the impl-window-order-by branch June 16, 2021 14:08

houqp added the enhancement New feature or request label Jul 31, 2021

	/// window frame spec, with th expression of first_value, last_value, and nth_value.
	/// window frame spec, with the exception of first_value, last_value, and nth_value.

Conversation

jimexist commented Jun 7, 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 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

alamb left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

jimexist commented Jun 15, 2021

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jimexist commented Jun 7, 2021 •

edited

Loading

codecov-commenter commented Jun 10, 2021 •

edited

Loading

jimexist commented Jun 15, 2021 •

edited

Loading