Skip to content

Conversation

@qazxcdswe123
Copy link
Contributor

See #14586 (comment) for benchmark and context

Which issue does this PR close?

Rationale for this change

This pass is no longer useful according to benchmark and may even slow down planning speed.

What changes are included in this PR?

  • removed single_distinct_to_groupby optimizer pass

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added the optimizer Optimizer rules label Mar 8, 2025
// Filters can't be pushed down past Limits, we should do PushDownFilter after PushDownLimit
Arc::new(PushDownLimit::new()),
Arc::new(PushDownFilter::new()),
Arc::new(SingleDistinctToGroupBy::new()),
Copy link
Member

Choose a reason for hiding this comment

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

Queries like avg(distinct a) rely on this rule, and without it, they cannot be executed anymore.

The following query is available on the main branch, but not on this PR branch.

DataFusion CLI v46.0.0
> select avg(distinct a) from values(1) t(a);
Execution error: avg(DISTINCT) aggregations are not available

Copy link
Contributor

@alamb alamb Mar 11, 2025

Choose a reason for hiding this comment

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

The alternative would be to directly implement distinct support in the avg accumulator

The lack of avg support means we can't run multiple distinct avg aggregates

> select avg(distinct a), avg(distinct b) from values(1, 2) t(a, b);
Execution error: avg(DISTINCT) aggregations are not available

This is tracked by

Copy link
Contributor

Choose a reason for hiding this comment

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

I also updated the ticket #2408 to explain the issue and a bit more about what is needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants