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

Move optimizer rule that has aggregate function out of core #10455

Closed
jayzhan211 opened this issue May 11, 2024 · 3 comments
Closed

Move optimizer rule that has aggregate function out of core #10455

jayzhan211 opened this issue May 11, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@jayzhan211
Copy link
Contributor

jayzhan211 commented May 11, 2024

Is your feature request related to a problem or challenge?

Part of #8708
The background is that we are moving builtin aggregate function to functions-aggregate as an UDAF

While moving count to UDAF, I found that we need to call UDAF in the optimize rule single_distinct_to_groupby.
I think we need to move the rule that has aggregate function out of the core because the optimizer is the core of the datafusion that we would like to prevent importing the functions-aggregate crate

Describe the solution you'd like

Optimizer

Move common optimizer things to optimizer-common crate or datafusion-expr crate
Move rules to functions-aggregate

  • single_distinct_to_groupby
  • common_subexpr_eliminate
  • decorrelate
  • replace_distinct_aggregate

Analyzer

Describe alternatives you've considered

No response

Additional context

No response

@jayzhan211 jayzhan211 added the enhancement New feature or request label May 11, 2024
@jayzhan211 jayzhan211 changed the title Introduce user-defined-optimizer for user-defined function Move optimizer rule that has UDF or UDAF out of core May 11, 2024
@jayzhan211 jayzhan211 changed the title Move optimizer rule that has UDF or UDAF out of core Move optimizer rule that has aggregate function out of core May 11, 2024
@alamb
Copy link
Contributor

alamb commented May 12, 2024

What part of the rule was aggregate specific?

I think the single distinct aggregate removal rule is general to all aggreagetes (it isn't aggregate specific) including user defined ones so I am not sure we need to move the rules.

BTW this PR is updating single distinct aggregate rule #10460

@jayzhan211
Copy link
Contributor Author

I thought I need to import functions-aggregate to create new aggregate function expression, but I found out that I could just modify AggregateFunction directly, since the function does not change.

@jayzhan211
Copy link
Contributor Author

let me close the issue first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants