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

Simplify windows schema calculation #8955

Closed
wants to merge 1 commit into from
Closed

Simplify windows schema calculation #8955

wants to merge 1 commit into from

Conversation

comphead
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

As @viirya noted in #8920 (comment) the window schema can be simplified

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@comphead comphead requested a review from viirya January 22, 2024 23:42
@github-actions github-actions bot added the core Core DataFusion crate label Jan 22, 2024
@comphead comphead mentioned this pull request Jan 22, 2024
Merged
@mustafasrepo
Copy link
Contributor

mustafasrepo commented Jan 23, 2024 •
edited
Loading

I think we should continue with the PR8945.
The reason is that:

  • In this implementation while creating window expr, we use window schema fields to get data type. However, conceptually it should only use its input fields (argument name also suggests that).
  • secondly in this implementation create_window_expr_with_name function has a conversion from DfSchema to Schema. This depends on their field names to be same, however this is not always the same

@comphead
Copy link
Contributor Author

Closed in favor of #8945

@comphead comphead closed this Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viirya viirya Awaiting requested review from viirya

@mustafasrepo mustafasrepo Awaiting requested review from mustafasrepo

Assignees
No one assigned
Labels
core Core DataFusion crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@comphead @mustafasrepo