Skip to content

ARROW-9055: [C++] Add sum/mean/minmax kernels for Boolean type #7478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
kszucs wants to merge 11 commits into from
Closed

ARROW-9055: [C++] Add sum/mean/minmax kernels for Boolean type #7478

kszucs wants to merge 11 commits into from

Conversation

@kszucs
Copy link
Member

@kszucs kszucs commented Jun 18, 2020

No description provided.

kszucs
kszucs commented Jun 18, 2020
View reviewed changes
@github-actions
Copy link

github-actions bot commented Jun 18, 2020

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

pitrou
pitrou reviewed Jun 18, 2020
View reviewed changes
cpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Copy link
Member

@pitrou pitrou Jun 18, 2020

Choose a reason for hiding this comment

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

Why isn't it 0 here?

pitrou
pitrou reviewed Jun 18, 2020
View reviewed changes
pitrou
pitrou reviewed Jun 18, 2020
View reviewed changes
pitrou
pitrou reviewed Jun 18, 2020
View reviewed changes
pitrou
pitrou reviewed Jun 18, 2020
View reviewed changes
wesm
wesm requested changes Jun 18, 2020
View reviewed changes
@nealrichardson
Copy link
Member

nealrichardson commented Jun 18, 2020

I pushed some changes to the R bindings to take advantage of this. I'll note that MinMax also doesn't support boolean and probably should.

@kszucs kszucs changed the title ARROW-9055: [C++] Add sum/mean kernels for Boolean type ARROW-9055: [C++] Add sum/mean/minmax kernels for Boolean type Jun 19, 2020
kszucs
kszucs commented Jun 19, 2020
View reviewed changes
cpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Copy link
Member Author

@kszucs kszucs Jun 19, 2020

Choose a reason for hiding this comment

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

BooleanArray has no raw_values() method.

kszucs
kszucs commented Jun 19, 2020
View reviewed changes
kszucs
kszucs commented Jun 19, 2020
View reviewed changes
cpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Copy link
Member Author

@kszucs kszucs Jun 19, 2020

Choose a reason for hiding this comment

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

So these were returning the types' extreme values.

kszucs
kszucs commented Jun 19, 2020
View reviewed changes
nealrichardson
nealrichardson approved these changes Jun 19, 2020
View reviewed changes
Copy link
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

From the R side, this LGTM

@kszucs kszucs requested a review from wesm June 22, 2020 09:38
@kszucs
Copy link
Member Author

kszucs commented Jun 24, 2020

ping @wesm

@wesm
Copy link
Member

wesm commented Jun 24, 2020

Looking

wesm
wesm reviewed Jun 24, 2020
View reviewed changes
Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

The min/max implementation seems more complicated / less efficient than it needs to be, and I think the unit test setup could be simplified, but otherwise this looks OK

@kszucs
Copy link
Member Author

kszucs commented Jun 26, 2020

@ursabot build

wesm
wesm approved these changes Jun 30, 2020
View reviewed changes
Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1. We will have to resolve the sum([]) -> null/0 by introducing a "minimum valid values" option.

  • minimum # valid values == 0 -> 0
  • minimum # valid values == 1 -> null

@wesm wesm closed this in db719ff Jul 1, 2020
@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Jul 1, 2020 •
edited
Loading

We will have to resolve the sum([]) -> null/0 by introducing a "minimum valid values" option.

Do we already have a JIRA to track this?

EDIT -> it is mentioned in ARROW-9054

@asfimport asfimport mentioned this pull request Jul 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou pitrou left review comments

@wesm wesm wesm approved these changes

@nealrichardson nealrichardson nealrichardson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kszucs @nealrichardson @wesm @jorisvandenbossche @pitrou