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

Cleanup WindowExpr and AggregateExpr trait #12069

Closed

Conversation

lewiszlw
Copy link
Member

@lewiszlw lewiszlw commented Aug 20, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

Found some methods are never used in WindowExpr and AggregateExpr trait, as they are not public traits, so just remove them.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the physical-expr Physical Expressions label Aug 20, 2024
@berkaysynnada
Copy link
Contributor

We use these methods in our fork. If there is no harm, we would prefer to keep them.

@lewiszlw
Copy link
Member Author

We might eventually delete entire AggregateExpr trait (#11810).

@berkaysynnada
Copy link
Contributor

I agree that AggregateExpr should be deleted, but I prefer to remove it at one shot. You are also removing more here.

@lewiszlw
Copy link
Member Author

I could remove WindowExpr trait change in this pr.

Removing AggregateExpr trait at one shot will result a huge pr, it's hard for reviewing.

Copy link
Contributor

@ozankabak ozankabak left a comment

Choose a reason for hiding this comment

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

This would get the code in a state that breaks some downstream users (including us). Let's proceed one by one -- let's start off by focusing on removing AggregateExpr. We will help with reviewing.

If you want to do this in small PRs and make this the first step, you can start off by moving all_expressions and with_new_expressions APIs to AggregateFunctionExpr. Thanks

@lewiszlw
Copy link
Member Author

Alright, I'll try to remove AggregateExpr trait in one pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants