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

Remove EliminateAggDistinct optimizer rule, implement distinct aggs in Python #1006

Merged
merged 7 commits into from Jan 26, 2023

Conversation

charlesbluca
Copy link
Collaborator

This PR attempts to remove the EliminateAggDistinct optimizer rule and instead handle distinct aggregations directly from the Python aggregate plugin using drop_duplicates.

This ideally should unblock the groupby related failures cropping up in #998 and resolve the existing groupby agg failures that we had seen in test_compatibility.py.

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2023

Codecov Report

Merging #1006 (1542a28) into main (08cff1a) will increase coverage by 0.13%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
+ Coverage   80.47%   80.60%   +0.13%     
==========================================
  Files          75       75              
  Lines        4231     4234       +3     
  Branches      765      766       +1     
==========================================
+ Hits         3405     3413       +8     
+ Misses        662      653       -9     
- Partials      164      168       +4     
Impacted Files Coverage Δ
dask_sql/physical/rel/logical/aggregate.py 91.25% <100.00%> (+0.14%) ⬆️
dask_sql/_version.py 35.31% <0.00%> (+1.41%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

dask_sql/physical/rel/logical/aggregate.py Show resolved Hide resolved
dask_sql/physical/rel/logical/aggregate.py Outdated Show resolved Hide resolved
tests/unit/test_queries.py Show resolved Hide resolved
Copy link
Collaborator

@ayushdg ayushdg left a comment

Choose a reason for hiding this comment

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

Changes lgtm!

@ayushdg ayushdg marked this pull request as ready for review January 26, 2023 12:03
@jdye64 jdye64 merged commit 5a3ee4c into dask-contrib:main Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants