Skip to content

Minor: add Window::try_new_with_schema constructor #10850

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
Merged
alamb merged 1 commit into from
Jun 10, 2024
Merged

Minor: add Window::try_new_with_schema constructor #10850

alamb merged 1 commit into from
Jun 10, 2024

Conversation

@sadboy
Copy link
Contributor

@sadboy sadboy commented Jun 10, 2024

Which issue does this PR close?

Closes N/A

Rationale for this change

This change makes the Window node API more uniform with other LogicalPlan nodes, which all expose both try_new and try_new_with_schema constructors.

What changes are included in this PR?

New method Window::try_new_with_schema

Are these changes tested?

Covered by existing tests.

Are there any user-facing changes?

No.

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Jun 10, 2024
@sadboy sadboy marked this pull request as ready for review June 10, 2024 03:53
alamb
alamb approved these changes Jun 10, 2024
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 good to me -- thank you @sadboy 🙏

I have a documentation suggestion, but I think that could be done as a follow on PR

datafusion/expr/src/logical_plan/plan.rs
)
}

pub fn try_new_with_schema(
Copy link
Contributor

@alamb alamb Jun 10, 2024

Choose a reason for hiding this comment

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

Perhaps we can add some documentation strings that explain how this is different than Window::try_new (specifically that the schema check is much faster)?

alamb
alamb reviewed Jun 10, 2024
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.

Also I wonder if we could update at least one callsite to use try_new_with_schema so we get some test coverage 🤔

@sadboy
Copy link
Contributor Author

sadboy commented Jun 10, 2024

if we could update at least one callsite to use try_new_with_schema so we get some test coverage

Window::try_new actually calls try_new_with_schema to do the work, that's why I didn't bother adding new tests. Do you think that's sufficient coverage?

@alamb
Copy link
Contributor

alamb commented Jun 10, 2024

Window::try_new actually calls try_new_with_schema to do the work, that's why I didn't bother adding new tests. Do you think that's sufficient coverage?

I think it is reasonable enough 🤔 . It might be good as a follow on PR to add another external callsite so that we don't accidentally mess up this API by accident)

@alamb alamb merged commit 0bd84e1 into apache:main Jun 10, 2024
@alamb
Copy link
Contributor

alamb commented Jun 10, 2024

Thanks again @sadboy -- We can add additional documentation as a follow on PR

findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
@sadboy @findepi
Add Window::try_new_from_schema (apache#10850)
1cd89ee
@Jefffrey Jefffrey mentioned this pull request Oct 4, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

Assignees

No one assigned

Labels

logical-expr Logical plan and expressions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sadboy @alamb