GH-45269: [C++][Compute] Add "pivot_wider" and "hash_pivot_wider" functions #45562

pitrou · 2025-02-18T12:49:57Z

Rationale for this change

Add "pivot wider" functionality such as in Pandas, through two dedicated functions:

a "pivot_wider" scalar aggregate function returning a Struct scalar
a "hash_pivot_wider" grouped aggregate function returning a Struct array

Both functions take two arguments (the column of pivot keys and the column of pivot values) and require passing a PivotWiderOptions structure with the expected pivot keys, so as to determine the output Struct type.

Are these changes tested?

Yes, by dedicated unit tests.

Are there any user-facing changes?

No, just new APIs.

GitHub Issue: [C++][Compute] Add a pivot function #45269

pitrou · 2025-02-18T12:53:05Z

@github-actions crossbow submit -g cpp

pitrou · 2025-02-18T13:06:18Z

cpp/src/arrow/acero/scalar_aggregate_node.cc

+  // Move away the states and recreate them eagerly, to make sure that any error
+  // below does not leave us with empty states.
+  auto states = std::move(states_);
+  states_.resize(kernels_.size());
+  if (!is_last) {
+    RETURN_NOT_OK(ResetKernelStates());
+  }


Without this change, any error in MergeAll below could lead to a crash.

Not sure I understand this - shouldn't error in MergeAll be bubbled up and stop execution? (Why does it lead to crash?)

I suppose it does not actually stop execution. @zanmato1984 might know more about this.

I don't know... why a new scalar agg function makes an existing acero node having to change the internal logic?

Is there any test failing w/o this part of change? Maybe I can take a look and then tell if this change is right.

Any test where the merge step returns an error could be affected. I guess other hash-aggregate functions never fail in their merge step, so this was not exercised before.

I tried, no luck, sorry. And from the code, the case PivotDuplicateValues is using group by thus shouldn't be executing this code path. I can't speak for the validity of this change w/o truly understanding what the problem is.

I do have a suspect though: this might be race related. And if, a big if, this is the case, I'm afraid your change doesn't fix the race. However there are still many things unclear so please don't take this seriously until I have new findings.

Do you still hit the problem if you revert this part of the change? Or shall we try reverting it (maybe temporarily) to see if CI is good?

And from the code, the case PivotDuplicateValues is using group by thus shouldn't be executing this code path.

Update: I was wrong about this part - the test is indeed doing the scalar agg.

(But unfortunately this doesn't change rest of the my comment.)

Hmm, I can't seem to reproduce locally right now, though I did have to change this to have the tests pass at the time. I'm gonna revert temporarily so that we can check CI.

I do have a suspect though: this might be race related. And if, a big if, this is the case, I'm afraid your change doesn't fix the race. However there are still many things unclear so please don't take this seriously until I have new findings.

Update: Turns out my suspect of race won't happen actually.

So I'm at my wits end by now.

Okay, even with Valgrind I can't reproduce anymore.

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

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

icexelloss · 2025-02-25T15:26:23Z

Should we also add python binding for the option and call via pyarrow.compute?

pitrou · 2025-02-25T17:04:21Z

Should we also add python binding for the option and call via pyarrow.compute?

We can indeed, for the aggregate function that is.

…r" functions

zanmato1984 · 2025-02-26T10:14:34Z

@github-actions crossbow submit -g cpp

pitrou · 2025-02-26T15:06:56Z

@github-actions crossbow submit -g cpp

github-actions · 2025-02-26T15:09:36Z

Revision: d97a9a7

Submitted crossbow builds: ursacomputing/crossbow @ actions-e51f9aecb5

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-meson
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

pitrou · 2025-02-26T16:29:48Z

I've addressed your review comments and also added Python bindings. @icexelloss @zanmato1984

zanmato1984

Still looking at the implantation. Just a few questions about the function prototype so far.

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

cpp/src/arrow/compute/api_aggregate.h

zanmato1984

Still looking at the detailed code. Some comments so far.

zanmato1984 · 2025-02-28T08:54:36Z

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

+  Result<PivotWiderKeyIndex> KeyNotFound(std::string_view key_name) {
+    if (unexpected_key_behavior_ == PivotWiderOptions::kIgnore) {
+      return kNullPivotKey;
+    }


Suggested change

}

}

DCHECK_EQ(unexpected_key_behavior_, PivotWiderOptions::kRaise);

zanmato1984 · 2025-02-28T09:13:04Z

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

+     "Each pivot key decides in which output field the corresponding pivot value\n"
+     "is emitted. If a pivot key doesn't appear, null is emitted.\n"
+     "If a pivot key appears twice, KeyError is raised.\n"
+     "Behavior of unexpected pivot keys is controlled by PivotWiderOptions."),


We may need to add some description about "duplicate values".

zanmato1984 · 2025-02-28T09:14:20Z

cpp/src/arrow/compute/api_aggregate.h

+///   2    |   null   |    13
+///   3    |   14     |    null
+/// ```
+class ARROW_EXPORT PivotWiderOptions : public FunctionOptions {


Any reason why this is named PivotWider? Will PivotWiden or just Pivot be more appropriate?

Well, because there is already a node named "pivot_longer".

arrow/cpp/src/arrow/acero/pivot_longer_node.cc

Line 4 in 0fbf982

// regarding copyright ownership. The ASF licenses this file

Besides, I find "pivot_wider" / "pivot_longer" more explanatory than "pivot" / "melt" or other alternatives.

zanmato1984 · 2025-02-28T16:33:46Z

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

+    //
+    // We are going to compute:
+    // - take_indices[key = 0] = [null, 0, null, null]
+    // - take_indices[key = 1] = [3, 2, null, 2]


Suggested change

// - take_indices[key = 1] = [3, 2, null, 2]

// - take_indices[key = 1] = [3, 2, null, 1]

zanmato1984 · 2025-02-28T16:38:20Z

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

+      // Populate the take_indices for each output column
+      for (int64_t i = 0; i < values.length; ++i) {
+        const PivotWiderKeyIndex key = keys[i];
+        const uint32_t group = groups[i];


Can be moved into the following if?

zanmato1984 · 2025-02-28T16:51:05Z

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

+     "All output struct fields have the same type as `pivot_values`.\n"
+     "Each pivot key decides in which output field the corresponding pivot value\n"
+     "is emitted. If a pivot key doesn't appear in a given group, null is emitted.\n"
+     "If a pivot key appears twice in a given group, KeyError is raised.\n"


Ditto, add description about "duplicate value".

github-actions bot added Component: C++ Component: Documentation awaiting review Awaiting review labels Feb 18, 2025

pitrou marked this pull request as ready for review February 18, 2025 12:52

pitrou requested a review from westonpace as a code owner February 18, 2025 12:52

pitrou requested review from zanmato1984 and removed request for westonpace February 18, 2025 12:52

This comment was marked as outdated.

Sign in to view

pitrou commented Feb 18, 2025

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Feb 18, 2025

pitrou mentioned this pull request Feb 18, 2025

[C++][Compute] Split up hash_aggregate.cc #45563

Open

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 25, 2025

icexelloss reviewed Feb 25, 2025

View reviewed changes

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

icexelloss reviewed Feb 25, 2025

View reviewed changes

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

apacheGH-45269: [C++][Compute] Add "pivot_wider" and "hash_pivot_wide…

a9e4700

…r" functions

pitrou force-pushed the pivot_kernel branch from 53cff55 to a9e4700 Compare February 26, 2025 09:37

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 26, 2025

Temporarily revert aggregate node changes

d584190

This comment was marked as outdated.

Sign in to view

Address review comments

d97a9a7

github-actions bot added the Component: Python label Feb 26, 2025

Add Python bindings

e5ebef4

pitrou force-pushed the pivot_kernel branch from ca31478 to e5ebef4 Compare February 26, 2025 16:42

zanmato1984 reviewed Feb 28, 2025

View reviewed changes

cpp/src/arrow/compute/kernels/hash_aggregate.cc Show resolved Hide resolved

cpp/src/arrow/compute/api_aggregate.h Show resolved Hide resolved

cpp/src/arrow/compute/api_aggregate.h Show resolved Hide resolved

zanmato1984 requested changes Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-45269: [C++][Compute] Add "pivot_wider" and "hash_pivot_wider" functions #45562

GH-45269: [C++][Compute] Add "pivot_wider" and "hash_pivot_wider" functions #45562

pitrou commented Feb 18, 2025 •

edited by github-actions bot

Loading

pitrou commented Feb 18, 2025

This comment was marked as outdated.

pitrou Feb 18, 2025

icexelloss Feb 25, 2025

pitrou Feb 25, 2025

zanmato1984 Feb 26, 2025

pitrou Feb 26, 2025

zanmato1984 Feb 26, 2025

zanmato1984 Feb 26, 2025

pitrou Feb 26, 2025

zanmato1984 Feb 26, 2025

pitrou Feb 26, 2025

icexelloss commented Feb 25, 2025

pitrou commented Feb 25, 2025

zanmato1984 commented Feb 26, 2025

This comment was marked as outdated.

pitrou commented Feb 26, 2025

github-actions bot commented Feb 26, 2025

pitrou commented Feb 26, 2025

zanmato1984 left a comment

zanmato1984 left a comment

zanmato1984 Feb 28, 2025

zanmato1984 Feb 28, 2025

zanmato1984 Feb 28, 2025

pitrou Mar 1, 2025

zanmato1984 Feb 28, 2025

zanmato1984 Feb 28, 2025

zanmato1984 Feb 28, 2025

	}
	}
	DCHECK_EQ(unexpected_key_behavior_, PivotWiderOptions::kRaise);

	// - take_indices[key = 1] = [3, 2, null, 2]
	// - take_indices[key = 1] = [3, 2, null, 1]

GH-45269: [C++][Compute] Add "pivot_wider" and "hash_pivot_wider" functions #45562

Are you sure you want to change the base?

GH-45269: [C++][Compute] Add "pivot_wider" and "hash_pivot_wider" functions #45562

Conversation

pitrou commented Feb 18, 2025 • edited by github-actions bot Loading

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

pitrou commented Feb 18, 2025

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icexelloss commented Feb 25, 2025

pitrou commented Feb 25, 2025

zanmato1984 commented Feb 26, 2025

This comment was marked as outdated.

pitrou commented Feb 26, 2025

github-actions bot commented Feb 26, 2025

pitrou commented Feb 26, 2025

zanmato1984 left a comment

Choose a reason for hiding this comment

zanmato1984 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Feb 18, 2025 •

edited by github-actions bot

Loading