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

Improve AccumulatorArgs by removing the usgaes of input_types #11761

Closed
wants to merge 9 commits into from

Conversation

xinlifoobar
Copy link
Contributor

Which issue does this PR close?

Part of #11725

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions core Core DataFusion crate substrait labels Aug 1, 2024
@xinlifoobar xinlifoobar changed the title Improve AccumulatorArgs by Removing the usgaes of schema and input_types Improve AccumulatorArgs by removing the usgaes of schema and input_types Aug 1, 2024
@github-actions github-actions bot removed the substrait label Aug 1, 2024
Copy link
Member

@lewiszlw lewiszlw Aug 2, 2024

Choose a reason for hiding this comment

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

This looks weird that moving a physical expr into logical expr crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, today the physical-expr-common references the expr, the dependency results in weird behavior. It might be better to isolate logic-expr physical-expr and move the code that references both, e.g., udf, udaf etc. another project.

CC @jayzhan211 @alamb

Copy link
Contributor

Choose a reason for hiding this comment

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

@xinlifoobar We should not move physical-expr in expr, what is the issue you met that needs the move?

Copy link
Contributor

Choose a reason for hiding this comment

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

We might to the refactor #11359 before removing input_types

Copy link
Contributor

@alamb alamb Aug 5, 2024

Choose a reason for hiding this comment

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

THank you all for this -- yes getting the physical-expr and Expr types untangled will be a great step. I agree with @jayzhan211 that #11359 might be good to look at first

@xinlifoobar xinlifoobar changed the title Improve AccumulatorArgs by removing the usgaes of schema and input_types Improve AccumulatorArgs by removing the usgaes of input_types Aug 5, 2024
@jayzhan211
Copy link
Contributor

jayzhan211 commented Aug 8, 2024

@xinlifoobar I plan to resolve the issue together in #11845 , so we can step forward to the final state at once

@alamb
Copy link
Contributor

alamb commented Aug 8, 2024

Superceded by #11845 I think, so marking it as draft as it isn't waiting for review

@alamb alamb marked this pull request as draft August 8, 2024 14:24
@alamb alamb closed this in #11845 Aug 9, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants