Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add window frame constructs - alternative #506

Merged
merged 1 commit into from
Jun 7, 2021

Conversation

jimexist
Copy link
Member

@jimexist jimexist commented Jun 5, 2021

Which issue does this PR close?

Related #361
Based on #463
Closes #492

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@codecov-commenter
Copy link

Codecov Report

Merging #506 (5e7f0db) into master (a9d04ca) will decrease coverage by 0.01%.
The diff coverage is 71.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #506      +/-   ##
==========================================
- Coverage   76.07%   76.06%   -0.02%     
==========================================
  Files         155      156       +1     
  Lines       26544    26720     +176     
==========================================
+ Hits        20194    20325     +131     
- Misses       6350     6395      +45     
Impacted Files Coverage Δ
...sta/rust/core/src/serde/logical_plan/from_proto.rs 35.41% <0.00%> (-0.51%) ⬇️
...lista/rust/core/src/serde/logical_plan/to_proto.rs 61.64% <0.00%> (-0.77%) ⬇️
...ta/rust/core/src/serde/physical_plan/from_proto.rs 38.51% <0.00%> (-0.14%) ⬇️
datafusion/src/optimizer/utils.rs 48.05% <0.00%> (-0.18%) ⬇️
datafusion/src/physical_plan/mod.rs 78.70% <ø> (ø)
datafusion/src/physical_plan/planner.rs 80.19% <ø> (ø)
datafusion/src/sql/utils.rs 66.54% <50.00%> (-0.25%) ⬇️
datafusion/src/logical_plan/expr.rs 84.56% <81.81%> (-0.05%) ⬇️
datafusion/src/physical_plan/window_frames.rs 86.72% <86.72%> (ø)
datafusion/src/sql/planner.rs 84.37% <95.00%> (+0.12%) ⬆️
... and 2 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 a9d04ca...5e7f0db. Read the comment docs.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jimexist -- this looks good -- I will try and review it carefully tomorrow

@@ -0,0 +1,337 @@
// Licensed to the Apache Software Foundation (ASF) under one
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mod is coupled with sqlparser AST and doesn't seem to be physical specific. Maybe it's better to move this mod into logical plane instead and reimport in physical plane?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it would make more sense for this module to be in logical_plan (I think it would also be fine to do as a follow on PR)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do a follow up pull request when I start working on implementing window frames (after order by and partition by)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #517

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which is fixed in #518

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me - thank you @jimexist

I agree with @houqp that window_function might make more sense to logically live in the logical_plan module, but I am also ok with it being in this module too

datafusion/src/logical_plan/expr.rs Show resolved Hide resolved
@@ -0,0 +1,337 @@
// Licensed to the Apache Software Foundation (ASF) under one
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it would make more sense for this module to be in logical_plan (I think it would also be fine to do as a follow on PR)

impl TryFrom<ast::WindowFrame> for WindowFrame {
type Error = DataFusionError;

fn try_from(value: ast::WindowFrame) -> Result<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I really like this structure

/// There are five ways to describe starting and ending frame boundaries:
///
/// 1. UNBOUNDED PRECEDING
/// 2. <expr> PRECEDING
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@alamb alamb merged commit 767eeb0 into apache:master Jun 7, 2021
@jimexist jimexist deleted the add-window-frame-2 branch June 7, 2021 12:33
@houqp houqp added datafusion Changes in the datafusion crate enhancement New feature or request labels Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datafusion Changes in the datafusion crate enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants