Remove unnecessary first aggregation from groupby agg code
#951
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ayushdg
reviewed
Dec 2, 2022
| # Fix the column names and the order of them, as this was messed with during the aggregations | ||
| df_agg.columns = df_agg.columns.get_level_values(-1) | ||
| # SQL does not care about the index, but if group columns were specified we'll want to keep those | ||
| df_agg = df_agg.reset_index(drop=(not group_columns)) |
There was a problem hiding this comment.
In cases where we have multiple groupby columns but are selecting only 1 does it make sense to handle that logic of dropping the rest here or do we rely on the limit_to projection mechanisms downstream to achieve this?
There was a problem hiding this comment.
Yeah right now we're relying on limit_to to exclude the unnecessary columns, not sure if it's worth it to manually drop these columns after the reset_index
Codecov Report
@@ Coverage Diff @@
## main #951 +/- ##
==========================================
+ Coverage 75.08% 77.51% +2.42%
==========================================
Files 75 75
Lines 4231 4220 -11
Branches 771 767 -4
==========================================
+ Hits 3177 3271 +94
+ Misses 886 775 -111
- Partials 168 174 +6
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
charlesbluca
changed the title
first aggregation from groupby agg codefirst aggregation from groupby agg code
charlesbluca
commented
Dec 5, 2022
|
I tested this on a sample dataset: df = timeseries(freq="10ms")
c.create_table("df",df, persist=True)
%timeit c.sql("SELECT id, SUM(x) from df GROUP BY id").compute()
%timeit c.sql("SELECT id, SUM(x) from df GROUP BY id, name").compute()Main vs This PR |
ayushdg
approved these changes
Dec 13, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now, if group columns are specified for a groupby aggregation (e.g.
select name, sum(x) from df group by name), we include afirstaggregation to grab the relevant row values for each grouping. This shouldn't be necessary, as Dask groupby aggregations already provide this info in the resulting index.This PR removes this
firstaggregation and retains the lost information by instead dropping the groupby agg index withreset_index(drop=False)when group columns are specified.