Skip to content

ARROW-14035: [C++][Python][R] Implement count distinct kernel - #11257

Closed
aucahuasi wants to merge 3 commits into
apache:masterfrom
aucahuasi:ARROW-14035-implement-count_distinct-kernel
Closed

ARROW-14035: [C++][Python][R] Implement count distinct kernel#11257
aucahuasi wants to merge 3 commits into
apache:masterfrom
aucahuasi:ARROW-14035-implement-count_distinct-kernel

Conversation

@aucahuasi

@aucahuasiaucahuasi commented Sep 28, 2021

Copy link
Copy Markdown
Contributor

@github-actions

github-actionsBot commented Sep 28, 2021

Copy link
Copy Markdown

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@aucahuasiaucahuasi changed the title [C++] ARROW-14035: Implement count distinct kernelARROW-14035: [C++] Implement count distinct kernelSep 28, 2021
@github-actions

github-actionsBot commented Sep 28, 2021

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14035

lidavidm
lidavidm reviewed Sep 28, 2021
View reviewed changes

@lidavidmlidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I realize this is very much a draft, but I left some notes.

Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
edponce
edponce reviewed Sep 28, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
edponce
edponce reviewed Sep 28, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
@ianmcook

ianmcook commented Sep 28, 2021

Copy link
Copy Markdown
Member

@aucahuasi may I push a commit to this PR to implement the R binding? (ARROW-14036)

@aucahuasi

aucahuasi commented Sep 28, 2021

Copy link
Copy Markdown
ContributorAuthor

@ianmcook sure!

@aucahuasi

aucahuasi commented Sep 28, 2021

Copy link
Copy Markdown
ContributorAuthor

Follow up https://issues.apache.org/jira/browse/ARROW-14158

@aucahuasiaucahuasi changed the title ARROW-14035: [C++] Implement count distinct kernelARROW-14035: [C++][Python] Implement count distinct kernelSep 29, 2021
@aucahuasi
aucahuasiforce-pushed the ARROW-14035-implement-count_distinct-kernel branch from 7765ce6 to 8d61f6aCompareSeptember 29, 2021 00:18
@aucahuasi

aucahuasi commented Sep 29, 2021

Copy link
Copy Markdown
ContributorAuthor

@lidavidm@edponce Once again, thank you for the feedback!
I sent new changes and I think this PR is ready for review.
btw I also added a test in python with timestamps.

@aucahuasi
aucahuasi marked this pull request as ready for review September 29, 2021 00:20
@aucahuasi
aucahuasiforce-pushed the ARROW-14035-implement-count_distinct-kernel branch from 8d61f6a to 5570299CompareSeptember 29, 2021 03:09
edponce
edponce reviewed Sep 29, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
edponce
edponce suggested changes Sep 29, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Comment threadpython/pyarrow/tests/test_compute.py Outdated
pitrou
pitrou requested changes Sep 29, 2021
View reviewed changes

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for doing this. I'm adding some comments to Eduardo's.

Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
lidavidm
lidavidm reviewed Sep 29, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
@aucahuasiaucahuasi changed the title ARROW-14035: [C++][Python] Implement count distinct kernelARROW-14035: [C++][Python][R] Implement count distinct kernelSep 29, 2021
@aucahuasi
aucahuasiforce-pushed the ARROW-14035-implement-count_distinct-kernel branch from 5570299 to 47571c7CompareSeptember 30, 2021 18:14
ianmcook
ianmcook reviewed Oct 1, 2021
View reviewed changes
Comment threadr/tests/testthat/test-dplyr-summarize.R Outdated
ianmcook
ianmcook reviewed Oct 1, 2021
View reviewed changes
Comment threadr/tests/testthat/test-dplyr-summarize.R Outdated
@aucahuasi
aucahuasiforce-pushed the ARROW-14035-implement-count_distinct-kernel branch from 47571c7 to e6c7d1dCompareOctober 1, 2021 15:19
@aucahuasi

aucahuasi commented Oct 1, 2021

Copy link
Copy Markdown
ContributorAuthor

Let me know about any additional feedback please!
Thanks!
cc @pitrou@ianmcook

nealrichardson
nealrichardson reviewed Oct 1, 2021
View reviewed changes
Comment threadr/tests/testthat/test-dplyr-summarize.R Outdated
edponce
edponce suggested changes Oct 2, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
@aucahuasi
aucahuasiforce-pushed the ARROW-14035-implement-count_distinct-kernel branch 3 times, most recently from efe7974 to 5711dfdCompareOctober 2, 2021 21:02
ianmcook
ianmcook reviewed Oct 3, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst Outdated
@aucahuasi
aucahuasiforce-pushed the ARROW-14035-implement-count_distinct-kernel branch from cc10c5f to a1b6dbbCompareOctober 3, 2021 20:58
pitrou
pitrou requested changes Oct 4, 2021
View reviewed changes

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the update @aucahuasi . It looks like there's a couple issues remaining, see below.

Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadr/src/compute.cpp Outdated
@aucahuasi
aucahuasiforce-pushed the ARROW-14035-implement-count_distinct-kernel branch 3 times, most recently from e0c42bb to 5c47cdaCompareOctober 4, 2021 17:23
@aucahuasi
aucahuasi requested a review from pitrouOctober 4, 2021 17:36
aucahuasiand others added 3 commits October 5, 2021 14:43
@aucahuasi@pitrou
ARROW-14035: Implement count distinct kernel
e53c0b8
finish basic implementation & add tests and docs
add count_distinct_doc
add R binding for count_distinct kernel
support more types & add more tests
minor change
reduce code
use type matchers to reduce even more code
minor change
Update docs/source/cpp/compute.rst Co-authored-by: Ian Cook <ianmcook@gmail.com>
minor changes
add tests for scalar types and fix an issue for scalars
@pitrou
Simplify namespaces and function registration
9761c38
@pitrou
Some nits
5de1a5c
@pitrou
pitrouforce-pushed the ARROW-14035-implement-count_distinct-kernel branch from 5c47cda to 5de1a5cCompareOctober 5, 2021 13:10
ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
@aucahuasi@pitrou@ViniciusSouzaRoque
ARROW-14035: [C++][Python][R] Implement count distinct kernel
9e262c4
https://issues.apache.org/jira/browse/ARROW-14035Closesapache#11257 from aucahuasi/ARROW-14035-implement-count_distinct-kernel
Lead-authored-by: Percy Camilo Triveño Aucahuasi <percy.camilo.ta@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
@asfimportasfimport mentioned this pull request Oct 5, 2021
Closed
@asfimportasfimport mentioned this pull request Sep 28, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ianmcookianmcookianmcook left review comments
@lidavidmlidavidmlidavidm left review comments
@nealrichardsonnealrichardsonnealrichardson left review comments
@thisisnicthisisnicthisisnic left review comments
@pitroupitroupitrou approved these changes
+1 more reviewer
@edponceedponceedponce requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Component: C++Component: PythonComponent: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@aucahuasi@ianmcook@edponce@lidavidm@pitrou@nealrichardson@thisisnic