ARROW-6657: [Rust] [DataFusion] Add Count Aggregate Expression#5513
ARROW-6657: [Rust] [DataFusion] Add Count Aggregate Expression#5513sinistersnare wants to merge 2 commits intoapache:masterfrom
Conversation
|
Hi @sinistersnare thanks for this! Just let us know when you think it is ready for review (or if you have any questions)? |
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
This looks great! Please add SQL tests to context.rs based on the ones for SUM:
https://github.com/apache/arrow/blob/master/rust/datafusion/src/execution/context.rs#L616-L642
|
Those are exactly the tests I was looking for. Thanks, I will push an update tonight! |
|
@sinistersnare I see you merged master into your branch .. that can lead to issues because we don't use a merging model on this repo. See https://andygrove.io/apache_arrow_git_tips/ for more info. |
sinistersnare
force-pushed
the
ARROW-6657
branch
from
b93c61a to
7820840
Compare
|
Took a bit longer than expected (moving currently), but I added some SQL tests! Aside from my worry from above, I think I am ready for this. |
sinistersnare
force-pushed
the
ARROW-6657
branch
from
7820840 to
2e85443
Compare
|
Fixed the style errors too, @andygrove @paddyhoran this should be good-to-go! |
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
LGTM! Thanks @sinistersnare
There was a problem hiding this comment.
I just spotted an issue with this, and I have the same issue with the SumExpr implementation ... we are evaluating the expression against the whole batch multiple times (once for every row in the batch). This is a design flaw in the accumulator trait I guess. I'll give this some thought today.
There was a problem hiding this comment.
I think it would be best if we merge this in without this optimization/fix, so you can simply fix both instances at the same time?
There was a problem hiding this comment.
Please take a look at the proposed fix in #5542 and let me know what you think. I'd prefer to get this reviewed and merged first, then you can rebase this PR and implement the changes.
|
@sinistersnare Please rebase against the latest master and I can approve and merge |
sinistersnare
force-pushed
the
ARROW-6657
branch
from
2e85443 to
895f2ca
Compare
|
@andygrove updated! |
sinistersnare
force-pushed
the
ARROW-6657
branch
from
895f2ca to
6c2d823
Compare
sinistersnare
force-pushed
the
ARROW-6657
branch
from
6c2d823 to
12d0c2c
Compare
Hi, I added this code, and the tests pass. I still need to actually test it using a real example, so I would say its not completely ready for merge yet. Closes #5513 from sinistersnare/ARROW-6657 and squashes the following commits: 64d0c00 <Andy Grove> formatting 12d0c2c <Davis Silverman> Add Count Aggregate Expression Lead-authored-by: Davis Silverman <sinistersnare@gmail.com> Co-authored-by: Andy Grove <andygrove73@gmail.com> Signed-off-by: Andy Grove <andygrove73@gmail.com>
3 participants
Hi, I added this code, and the tests pass. I still need to actually test it using a real example, so I would say its not completely ready for merge yet.