Skip to content

ARROW-9056: [C++] Support aggregations over scalars - #10725

Closed
lidavidm wants to merge 10 commits into
apache:masterfrom
lidavidm:arrow-9056
Closed

ARROW-9056: [C++] Support aggregations over scalars#10725
lidavidm wants to merge 10 commits into
apache:masterfrom
lidavidm:arrow-9056

Conversation

@lidavidm

@lidavidmlidavidm commented Jul 15, 2021

Copy link
Copy Markdown
Member

This adds various trivial implementations to support scalar aggregations over scalar inputs, for all kernels except index.

@github-actions

github-actionsBot commented Jul 15, 2021

Copy link
Copy Markdown

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

@bkietz
bkietz self-requested a review July 15, 2021 17:53
nealrichardson
nealrichardson requested changes Jul 15, 2021
View reviewed changes

@nealrichardsonnealrichardson 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.

There are a couple of skipped tests for this in the R package, could you remove the skips? (Just search for "ARROW-9056")

bkietz
bkietz requested changes Jul 15, 2021
View reviewed changes

@bkietzbkietz 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!
Two high level comments:

Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
bkietz
bkietz requested changes Jul 15, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic_internal.h Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic_internal.h Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_basic_internal.h Outdated
bkietz
bkietz approved these changes Jul 15, 2021
View reviewed changes

@bkietzbkietz 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.

LGTM

@lidavidm

lidavidm commented Jul 16, 2021

Copy link
Copy Markdown
MemberAuthor

Fixed the MSVC failure + rebased.

@lidavidmlidavidm mentioned this pull request Aug 9, 2021
Closed
@asfimportasfimport mentioned this pull request Jul 16, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nealrichardsonnealrichardsonnealrichardson requested changes
@bkietzbkietzbkietz approved these changes

Assignees

No one assigned

Labels

Component: C++Component: GLibComponent: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lidavidm@bkietz@nealrichardson