Skip to content

ARROW-9054: [C++] Add ScalarAggregateOptions - #9758

Closed
rok wants to merge 23 commits into
apache:masterfrom
rok:ARROW-9054
Closed

ARROW-9054: [C++] Add ScalarAggregateOptions#9758
rok wants to merge 23 commits into
apache:masterfrom
rok:ARROW-9054

Conversation

@rok

@rokrok commented Mar 20, 2021

Copy link
Copy Markdown
Member

ARROW-9054.

This adds ScalarAggregateOptions to control null behavior of mean and sum kernels.

@github-actions

github-actionsBot commented Mar 20, 2021

Copy link
Copy Markdown

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

@rok

rok commented Mar 21, 2021

Copy link
Copy Markdown
MemberAuthor

As per comments in Jiras this should introduce the following:

  • sum - don't promote type (e.g. int32 to int64) if possible
  • sum / mean - should have min_count and return NA if the count of non-nulls is lower than min_count.
  • sum / mean - should short circuit when it becomes impossible for non-null count to be below min_count.
  • mean - should have skipna and keepna options.

@rok
rokforce-pushed the ARROW-9054 branch 2 times, most recently from d210140 to 97c78d6CompareMarch 25, 2021 16:17
@rok
rokforce-pushed the ARROW-9054 branch 2 times, most recently from c71b1f8 to 6e89d74CompareApril 12, 2021 16:02
@nealrichardson

nealrichardson commented Apr 12, 2021

Copy link
Copy Markdown
Member

Should MinMax be updated to use ScalarAggregateOptions?

You should also be able to remove https://github.com/apache/arrow/blob/master/r/R/compute.R#L113-L117 and search for any other references to that jira number in the R code/tests.

@rok

rok commented Apr 13, 2021

Copy link
Copy Markdown
MemberAuthor

Should MinMax be updated to use ScalarAggregateOptions?

I'll look into it. Count would be a candidate as well.

You should also be able to remove https://github.com/apache/arrow/blob/master/r/R/compute.R#L113-L117 and search for any other references to that jira number in the R code/tests.

Will do.

@rok

rok commented Apr 15, 2021

Copy link
Copy Markdown
MemberAuthor

This is slowly coming together. Remaining todo:

  • min/max must cover R edge cases
  • sum - don't promote type (e.g. int32 to int64) if possible
  • sum / mean - should short circuit when it becomes impossible for non-null count to be below min_count.

@jorisvandenbossche

jorisvandenbossche commented Apr 15, 2021

Copy link
Copy Markdown
Member

sum - don't promote type (e.g. int32 to int64) if possible

Is this desired behaviour? I think in general the resulting data type should be known based on the input types and operation (and not depend on the values)

jorisvandenbossche
jorisvandenbossche reviewed Apr 15, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/api_aggregate.h Outdated
Comment threadcpp/src/arrow/compute/api_aggregate.h Outdated
Comment threadcpp/src/arrow/compute/api_aggregate.h Outdated
Comment threadcpp/src/arrow/compute/kernels/aggregate_test.cc Outdated

@jorisvandenbosschejorisvandenbosscheApr 15, 2021

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.

Just to make sure I am reading this one correctly, is it basically testing mean([], min_count=0) == 0 ?

I am not sure that should give 0 as result? I would have expected either NaN or Null.
Basically mean of empty is dividing 0/0, which is NaN.

@rokrokApr 15, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Agreed. I think I've put it there to have it documented while I was changing other things.

@jorisvandenbosschejorisvandenbosscheApr 19, 2021

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.

It seems you changed it to return null now? Should it rather return NaN? (I assume it could be consistent with a manual mean from sum/count with min_count=0)

@jorisvandenbossche

jorisvandenbossche commented Apr 15, 2021

Copy link
Copy Markdown
Member

Looking forward to this! Added a few comments on naming and exact expected semantics.

@rok
rokforce-pushed the ARROW-9054 branch 2 times, most recently from 85e8bf0 to 1478720CompareApril 16, 2021 02:07
@rok
rok marked this pull request as ready for review April 16, 2021 02:20
@rok

rok commented Apr 16, 2021

Copy link
Copy Markdown
MemberAuthor

This requires some more work on C Glib but is ready for review otherwise.

@rok

rok commented Apr 16, 2021

Copy link
Copy Markdown
MemberAuthor

@jorisvandenbossche & @nealrichardson could you take a look at this proposal?
I'm not very confident about the c_glib and Ruby changes, who could review that?

@rok

rok commented Apr 16, 2021•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Should we introduce ScalarAggregateOptions in All and Any as well?

@rok
rokforce-pushed the ARROW-9054 branch 5 times, most recently from 40f5e49 to ce22f1aCompareApril 17, 2021 18:43
rokand others added 23 commits May 20, 2021 23:07
@rok
Adding ScalarAggregateOptions.
4edad4a
@rok
Minor changes.
9e90fb0
@rok
Replacing MinMaxOptions with ScalarAggregateOptions.
78d8fa0
@rok
Replace CountOptions with ScalarAggregateOptions.
b031a3b
@rok
Updating R bindings.
81a4aef
@rok
Fix to sum/mean behaviour.
3ae521e
@rok
Switching CountOptions to ScalarAggregateOptions.
71afe91
@rok
Changes to Python and c_glib.
d78b9cd
@rok
Further R and Python work.
ebdcbad
@rok
Reverting c_glib changes.
e9aefdb
@rok
Changes to c_glib.
b998691
@rok
Fixing casting warnings.
33958cf
@kou@rok
[GLib] Adjust for CountOptions -> ScalarAggregateOptions change
f61c732
@rok
R change.
72e84ab
@rok@jorisvandenbossche
Update cpp/src/arrow/compute/kernels/aggregate_test.cc
a5aef8e
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@rok
Changing ScalarAggregateOptions.min_count default to 0.
16e3af6
@rok
Implementing review feedback.
c47a58f
@rok
Fixing doxygen issue.
e6b217f
@rok
Implementing review feedback.
1e703ff
@rok
Changing mean behaviour.
3d1fa60
@rok
Removing min_count from minmax.
4583049
@kou@rok
Improve GLib part
2672108
* Fix typos
* Improve styles
* Add more tests
@nealrichardson@rok
💅 in R
3708060
Co-authored-by: Rok Mihevc <rok@mihevc.org>
@rok

rok commented May 20, 2021

Copy link
Copy Markdown
MemberAuthor

Some checks timed out. Rebased to restart the tasks.

@kou

kou commented May 21, 2021

Copy link
Copy Markdown
Member

The "R / AMD64 Windows RTools 35" failure was occurred on master too: https://github.com/apache/arrow/runs/2634288057

It's unrelated to this. I'll merge this.

@koukou closed this in 0a82432May 21, 2021
@rok

rok commented May 21, 2021

Copy link
Copy Markdown
MemberAuthor

Thanks for the help everyone!

@asfimportasfimport mentioned this pull request May 21, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@koukoukou left review comments
@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@nealrichardsonnealrichardsonnealrichardson approved these changes
@cyb70289cyb70289cyb70289 approved these changes

Assignees

No one assigned

Labels

Component: C++Component: GLibComponent: PythonComponent: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rok@nealrichardson@jorisvandenbossche@kou@cyb70289