Skip to content

Remove redundant Aggregate when DISTINCT & GROUP BY are in the same query - #11781

Merged
ozankabak merged 31 commits into
apache:mainfrom
synnada-ai:feature/eliminate-aggregate
Aug 4, 2024
Merged

Remove redundant Aggregate when DISTINCT & GROUP BY are in the same query#11781
ozankabak merged 31 commits into
apache:mainfrom
synnada-ai:feature/eliminate-aggregate

Conversation

@mertak-synnada

Copy link
Copy Markdown

Which issue does this PR close?

Closes #.

Rationale for this change

Inefficient planning is produced when a redundant DISTINCT & GROUP BY clause is used together.

Eg.

EXPLAIN SELECT c3 FROM aggregate_test_100 GROUP BY c3 LIMIT 5;
logical_plan
Limit: skip=0, fetch=5
--Aggregate: groupBy=[[aggregate_test_100.c3]], aggr=[[]]
----Aggregate: groupBy=[[aggregate_test_100.c3]], aggr=[[]]
------TableScan: aggregate_test_100 projection=[c3]
physical_plan
GlobalLimitExec: skip=0, fetch=5
--AggregateExec: mode=Final, gby=[c3@0 as c3], aggr=[], lim=[5]
----CoalescePartitionsExec
------AggregateExec: mode=Partial, gby=[c3@0 as c3], aggr=[], lim=[5]
--------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
----------AggregateExec: mode=Final, gby=[c3@0 as c3], aggr=[], lim=[5]
------------CoalescePartitionsExec
--------------AggregateExec: mode=Partial, gby=[c3@0 as c3], aggr=[], lim=[5]
----------------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
------------------CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100.csv]]}, projection=[c3], has_header=true

Consecutive empty aggregates with same group by are unnecessary.

What changes are included in this PR?

It's fixed that if a distinct is the same as a following group by statement, it should not be turned into an Aggregate plan.

Are these changes tested?

Yes

Are there any user-facing changes?

No

metesynnadaand others added 29 commits February 22, 2024 09:56
Fix deploying DataFusion site error
…eature/eliminate-aggregate
# Conflicts:
#	.github/workflows/docs.yaml
#	datafusion/optimizer/src/lib.rs
#	datafusion/optimizer/src/optimizer.rs
#	datafusion/optimizer/src/single_distinct_to_groupby.rs
add additional tests for not removing cases
Co-authored-by: Mehmet Ozan Kabak <ozankabak@gmail.com>
Co-authored-by: Mehmet Ozan Kabak <ozankabak@gmail.com>
@github-actionsgithub-actionsBot added the optimizer Optimizer rules label Aug 2, 2024
@github-actionsgithub-actionsBot added the sqllogictest SQL Logic Tests (.slt) label Aug 2, 2024

@ozankabakozankabak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this and it LGTM. Easy win!

@alambalamb changed the title Feature/eliminate aggregateRemove redundant Aggregate when DISTINCT & GROUP BYAug 2, 2024
@alambalamb changed the title Remove redundant Aggregate when DISTINCT & GROUP BYRemove redundant Aggregate when DISTINCT & GROUP BY are in the same queryAug 2, 2024

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

logical_plan
01)Limit: skip=0, fetch=5
02)--Aggregate: groupBy=[[aggregate_test_100.c3]], aggr=[[]]
03)----Aggregate: groupBy=[[aggregate_test_100.c3]], aggr=[[]]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These certainly look like bettr plans to me.

However, I wonder if these changes now mean the query doesn't fulfill the original intent of the tests

## Push limit into distinct group-by aggregation tests#

Therefore I wonder if we should add another test like

EXPLAIN SELECT DISTINCT c3, min(c1) FROM aggregate_test_100 group by c3 limit 5;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW in my opinion it would be fine to add the other tests as a new PR too

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds good, let's do it that way

@ozankabak
ozankabak merged commit c8e5996 into apache:mainAug 4, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizerOptimizer rulessqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@mertak-synnada@alamb@ozankabak@metesynnada@mustafasrepo