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.

Sign up for GitHub

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

Jump to bottom

Add window frame constructs - alternative #506

Merged
merged 1 commit into from
Jun 7, 2021
Merged

Add window frame constructs - alternative #506

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?

@jimexist jimexist mentioned this pull request Jun 5, 2021
Closed
@jimexist jimexist force-pushed the add-window-frame-2 branch from 0e2c3d0 to 947e18f Compare June 5, 2021 06:12
@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.

alamb
alamb reviewed Jun 5, 2021
View reviewed changes
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

houqp
houqp reviewed Jun 5, 2021
View reviewed changes
datafusion/src/physical_plan/window_frames.rs
@@ -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

alamb
alamb approved these changes Jun 6, 2021
View reviewed changes
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
datafusion/src/physical_plan/window_frames.rs
@@ -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)

datafusion/src/physical_plan/window_frames.rs
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

datafusion/src/physical_plan/window_frames.rs
/// 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.

👍

This was referenced Jun 6, 2021
Closed
Merged
@jimexist jimexist force-pushed the add-window-frame-2 branch from 78d49b8 to 31ec1ee Compare June 7, 2021 03:21
@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
@alamb alamb mentioned this pull request Jun 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houqp houqp houqp left review comments

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
datafusion Changes in the datafusion crate enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jimexist @codecov-commenter @alamb @houqp