dask-sql icon indicating copy to clipboard operation
dask-sql copied to clipboard

Add basic ROLLUP support for groupbys

Open charlesbluca opened this issue 3 years ago • 2 comments

Adds in a crude implementation of SQL groupbys with rollup; this is achieved by iterated through the various combinations of column that need to be grouped with _do_aggregations, and then concating all the resulting frames together.

charlesbluca avatar Dec 08 '22 20:12 charlesbluca

Codecov Report

Merging #963 (74b3b62) into main (747d8ab) will increase coverage by 0.16%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #963      +/-   ##
==========================================
+ Coverage   77.67%   77.84%   +0.16%     
==========================================
  Files          75       75              
  Lines        4215     4224       +9     
  Branches      765      768       +3     
==========================================
+ Hits         3274     3288      +14     
+ Misses        775      766       -9     
- Partials      166      170       +4     
Impacted Files Coverage Δ
dask_sql/physical/rel/logical/aggregate.py 89.78% <100.00%> (+0.34%) :arrow_up:
dask_sql/_version.py 35.31% <0.00%> (+1.41%) :arrow_up:
dask_sql/physical/rel/logical/subquery_alias.py 76.92% <0.00%> (+6.92%) :arrow_up:

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

codecov-commenter avatar Dec 08 '22 20:12 codecov-commenter

Looks like the failures here are because in the bump from datafusion 14 -> 22, we are no longer checking for rollup/cube as reserved function names?

https://github.com/apache/arrow-datafusion/blob/6d00bd990ce5644181ad1549a6c70c8406219070/datafusion/sql/src/planner.rs#L2148-L2155

Though it seems like there are still working tests using rollup so this might just be an indication that we need to make some modifications to our parsing logic around aggregate functions

charlesbluca avatar Jun 06 '23 20:06 charlesbluca