GH-32190: [C++][Compute] Implement cumulative prod, max and min functions by js8544 · Pull Request #36020 · apache/arrow

js8544 · 2023-06-10T08:02:02Z

Rationale for this change

Implement cumulative prod, max and min compute functions

What changes are included in this PR?

Add implementations, docs and tests for the three functions.
Refactor CumulativeSumOptions to CumulativeOptions for reusability.
Fix a bug where GenericFromScalar(GenericToScalar(std::nullopt)) != std::nullopt.
Remove an unnecessary Cast with the default start value.
Add tests to check behavior with NaN.

I'll explain some of the changes in comments.

Are these changes tested?

Yes, in vector_accumulative_ops_test.cc and test_compute.py

Are there any user-facing changes?

No. The data members of CumulativeSumOptions are changed, but the member functions behave as before. And std::optional also can be constructed directly from T. So users should not feel any difference.

Closes: [C++] Implement cumulative product, max, and min compute functions #32190

github-actions · 2023-06-10T08:02:28Z

⚠️ GitHub issue #32190 has been automatically assigned in GitHub to PR creator.

cpp/src/arrow/compute/api_vector.h

cpp/src/arrow/compute/function_internal.h

cpp/src/arrow/compute/kernels/base_arithmetic_internal.h

bkietz

Thanks for doing this!

This looks good, a few minor comments:

cpp/src/arrow/compute/function_internal.h

cpp/src/arrow/compute/kernels/base_arithmetic_internal.h

cpp/src/arrow/compute/kernels/vector_cumulative_ops.cc

python/pyarrow/_compute.pyx

bkietz · 2023-06-20T18:18:22Z

python/pyarrow/compute.py

@jorisvandenbossche Does this need an alias?

Suggested change

CumulativeOptions,

CumulativeOptions,

CumulativeOptions as CumulativeSumOptions,

For backwards compatibility, that's probably useful, yes (also on the C++ side this was done).

(although we should maybe actually add it as its own class and deprecate it, so we can remove the alias at some point. But this can also be done as a follow-up later)

Follow up issue here #36240

cpp/src/arrow/compute/kernels/vector_cumulative_ops.cc

…ute functions

Co-authored-by: Benjamin Kietzman <[email protected]>

cpp/src/arrow/compute/kernels/vector_cumulative_ops.cc

Co-authored-by: Benjamin Kietzman <[email protected]>

cpp/src/arrow/compute/kernels/base_arithmetic_internal.h

bkietz · 2023-06-21T19:33:46Z

cpp/src/arrow/compute/kernels/base_arithmetic_internal.h

+  template <typename Value>
+  static constexpr Value value{std::numeric_limits<Value>::min()};


These cumulative ops aren't currently instantiated for decimal types anyway: https://github.com/apache/arrow/blob/44edc27/cpp/src/arrow/compute/kernels/codegen_internal.h#L1070-L1100

... if you feel like filing a follow up issue for them, it should be fairly straightforward to specify these as specializations:

Suggested change

template <typename Value>

static constexpr Value value{std::numeric_limits<Value>::min()};

template <typename Value>

static constexpr Value value{std::numeric_limits<Value>::min()};

template <>

static constexpr Decimal128 value<Decimal128> = Decimal128::GetMinSentinel();

Multiplication will require more effort since we can't construct 1 without knowing the scale.

I'll remove them for this pr. Will send another pr for decimals.

python/pyarrow/_compute.pyx

bkietz

LGTM, thanks!

bkietz · 2023-06-22T12:18:15Z

CI failure is a backpressure test flake-out in AsOfJoin, unrelated.

Co-authored-by: Benjamin Kietzman <[email protected]>

icexelloss · 2023-06-22T16:28:29Z

Thanks @bkietz - created #36248 to track the Asof Join backpresure test issue

conbench-apache-arrow · 2023-06-25T21:24:36Z

Conbench analyzed the 6 benchmark runs on commit e3eb5898.

There were 6 benchmark results indicating a performance regression:

Commit Run on ursa-thinkcentre-m75q at 2023-06-25 03:03:51Z
- params=1048576/1, source=cpp-micro, suite=arrow-acero-aggregate-benchmark
- params=/1048576/0, source=cpp-micro, suite=arrow-acero-aggregate-benchmark
and 4 more (see the report linked below)

The full Conbench report has more details.

js8544 requested review from AlenkaF and westonpace as code owners June 10, 2023 08:02

github-actions bot added Component: C++ Component: Documentation Component: Python awaiting review Awaiting review labels Jun 10, 2023

js8544 commented Jun 10, 2023

View reviewed changes

cpp/src/arrow/compute/api_vector.h Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 10, 2023

js8544 commented Jun 10, 2023

View reviewed changes

cpp/src/arrow/compute/function_internal.h Outdated Show resolved Hide resolved

js8544 commented Jun 10, 2023

View reviewed changes

cpp/src/arrow/compute/kernels/base_arithmetic_internal.h Outdated Show resolved Hide resolved

js8544 commented Jun 10, 2023

View reviewed changes

cpp/src/arrow/compute/kernels/base_arithmetic_internal.h Outdated Show resolved Hide resolved

bkietz requested changes Jun 20, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jun 20, 2023

js8544 added 4 commits June 21, 2023 05:46

apacheGH-32190: [C++] Implement cumulative product, max, and min comp…

91e474f

…ute functions

fix python lint

4bd7ca2

fix

439b5fc

remove identity for decimal; fix doc

58c0952

js8544 force-pushed the jinshang/cumulative_functions branch from f4de821 to 4bd7ca2 Compare June 21, 2023 16:17

Update cpp/src/arrow/compute/kernels/vector_cumulative_ops.cc

ce630a5

Co-authored-by: Benjamin Kietzman <[email protected]>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 21, 2023

js8544 commented Jun 21, 2023

View reviewed changes

cpp/src/arrow/compute/kernels/vector_cumulative_ops.cc Outdated Show resolved Hide resolved

js8544 and others added 2 commits June 22, 2023 00:23

Apply suggestions from code review

c61d6cf

Co-authored-by: Benjamin Kietzman <[email protected]>

change identity to a trait

bfc5396

js8544 requested a review from bkietz June 21, 2023 16:35

bkietz requested changes Jun 21, 2023

View reviewed changes

github-actions bot removed the awaiting change review Awaiting change review label Jun 21, 2023

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 21, 2023

js8544 requested a review from bkietz June 22, 2023 00:13

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 22, 2023

bkietz mentioned this pull request Jun 22, 2023

[Python] Refactor CumulativeSumOptions to a separate class for independent deprecation #36240

Closed

bkietz approved these changes Jun 22, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Jun 22, 2023

js8544 and others added 2 commits June 22, 2023 20:32

Update python/pyarrow/compute.py

8611992

Co-authored-by: Benjamin Kietzman <[email protected]>

revert unnecessary change

cee20ec

bkietz merged commit e3eb589 into apache:main Jun 22, 2023

icexelloss mentioned this pull request Jun 22, 2023

[C++] Fix and re-enable Asof Join Backpresure test flakiness #36248

Open

	CumulativeOptions,
	CumulativeOptions,
	CumulativeOptions as CumulativeSumOptions,

		template <typename Value>
		static constexpr Value value{std::numeric_limits<Value>::min()};

Conversation

js8544 commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Jun 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkietz Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkietz Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkietz Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

js8544 Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

bkietz commented Jun 22, 2023

Uh oh!

icexelloss commented Jun 22, 2023

Uh oh!

conbench-apache-arrow bot commented Jun 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

js8544 commented Jun 10, 2023 •

edited

Loading

jorisvandenbossche Jun 22, 2023 •

edited

Loading