GH-33484: [C++][Compute] Implement `Grouper::Reset` #41352

zanmato1984 · 2024-04-23T14:44:13Z

Rationale for this change

Recently I've been working on some improvement for Grouper and I found adding Reset function could be beneficial. Then I trace down to #33484 from a TODO in code. Here comes this PR.

What changes are included in this PR?

Add Reset function for all the concrete Grouper implementations, and eliminate the recreation of Grouper in AnyKeysSegmenter.

Also add more RowSegmenter cases covering AnyKeysSegmenter.

Are these changes tested?

Yes. Legacy UTs should cover it well. Also added some new UTs.

Are there any user-facing changes?

None.

GitHub Issue: [C++] Add Grouper::Reset #33484

github-actions · 2024-04-23T14:44:39Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

zanmato1984 · 2024-04-23T17:13:39Z

cc @westonpace

github-actions · 2024-04-23T17:13:54Z

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

ZhangHuiGui · 2024-04-26T02:04:09Z

cpp/src/arrow/compute/row/grouper.cc

+    rows_minibatch_.Clean();
+    map_.cleanup();
+    RETURN_NOT_OK(map_.init(encode_ctx_.hardware_flags, ctx_->memory_pool()));
+    return Status::OK();


Do you think we should enlarge the size of the temp_stack_? Or the stack's internal states like top_ may no enough for the next bunch of operations after grouper reset.

I think all the temp vectors allocated by the temp_stack_ would be released at the end the of the call to each individual grouper member function (IMO that's why they are designed to be "temp vector"). Meaning by the time of Reset, the stack is empty already.

Ah, i see, thanks.
TempVectorStack will hold a resizable buffer allocated in TempVectorStack::Init function until the end of the grouper object.
But the TempVectorHolder will release the used space (already allocated in TempVectorStack ) at the call to each of individual grouper member function as you said.

Can you add a DCHECK that the temp_stack_ is empty, then?

Yeah that's probably reasonable. But considering current temp stack doesn't have a public method for size/empty check, and I have other open PRs for temp stack restructure, I'd add the necessary methods after other PRs are done and get this one rebased.

zanmato1984 · 2024-05-08T04:10:11Z

Hi @pitrou , would you please help to take a look? Thanks.

cpp/src/arrow/compute/row/grouper.h

pitrou · 2024-05-13T13:22:03Z

cpp/src/arrow/compute/row/grouper.cc

      : BaseRowSegmenter(key_types),
-        ctx_(ctx),
-        grouper_(nullptr),
+        grouper_(Grouper::Make(key_types, ctx).ValueOrDie()),


Is there a reasonable possibility that this might fail, for example due to input types?

Without extra context, there is. But the factory method (cleverly) avoids this by a dummy instantiation of Grouper:

arrow/cpp/src/arrow/compute/row/grouper.cc

Line 220 in a715ea0

ARROW_RETURN_NOT_OK(Grouper::Make(key_types, ctx)); // check types

But isn't it pointless to construct the Grouper twice? Can't the constructor simply take the Grouper that was constructed in the factory method?

Ah, I see what you are saying. Will update. Thanks for pointing this out.

pitrou · 2024-05-13T13:25:46Z

cpp/src/arrow/compute/row/grouper.cc

+    map_.clear();
+    offsets_.clear();
+    key_bytes_.clear();
+    num_groups_ = 0;


What about encoders_?

From what I saw in https://github.com/apache/arrow/blob/main/cpp/src/arrow/compute/kernels/row_encoder_internal.h, none of the encoders holds states that would change during the lifespan of the grouper.

pitrou · 2024-05-13T13:27:11Z

cpp/src/arrow/compute/row/grouper.cc

+    rows_minibatch_.Clean();
+    map_.cleanup();
+    RETURN_NOT_OK(map_.init(encode_ctx_.hardware_flags, ctx_->memory_pool()));
+    return Status::OK();


Can you add a DCHECK that the temp_stack_ is empty, then?

cpp/src/arrow/compute/row/grouper.cc

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

cpp/src/arrow/acero/hash_aggregate_test.cc

pitrou · 2024-05-14T14:39:48Z

cpp/src/arrow/acero/hash_aggregate_test.cc

+  // Assert next is the last (empty) segment.
+  ASSERT_OK_AND_ASSIGN(auto segment, segmenter->GetNextSegment(batch, offset));
+  ASSERT_TRUE(segment.offset >= batch.length);
+  ASSERT_TRUE(segment.is_open);


Should something be checked about segment.length here? Or is undetermined?

The last segment.length is supposed to be 0. Updated.

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

pitrou

Thanks @zanmato1984 . I'll declare this good to merge, though I'd love if @westonpace could double-check this.

conbench-apache-arrow · 2024-05-14T20:36:52Z

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit ada965f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change Recently I've been working on some improvement for `Grouper` and I found adding `Reset` function could be beneficial. Then I trace down to apache#33484 from a TODO in code. Here comes this PR. ### What changes are included in this PR? Add `Reset` function for all the concrete `Grouper` implementations, and eliminate the recreation of `Grouper` in `AnyKeysSegmenter`. Also add more `RowSegmenter` cases covering `AnyKeysSegmenter`. ### Are these changes tested? Yes. Legacy UTs should cover it well. Also added some new UTs. ### Are there any user-facing changes? None. * GitHub Issue: apache#33484 Lead-authored-by: Ruoxi Sun <zanmato1984@gmail.com> Co-authored-by: Rossi Sun <zanmato1984@gmail.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr> Signed-off-by: Antoine Pitrou <antoine@python.org>

Implement grouper reset

c232972

github-actions bot added Component: C++ awaiting review Awaiting review labels Apr 23, 2024

zanmato1984 changed the title ~~Implement grouper reset~~ GH-33484: [C++][Compute] Implement Grouper::Reset Apr 23, 2024

zanmato1984 marked this pull request as ready for review April 23, 2024 17:13

zanmato1984 requested a review from westonpace as a code owner April 23, 2024 17:13

ZhangHuiGui reviewed Apr 26, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 26, 2024

pitrou reviewed May 13, 2024

View reviewed changes

zanmato1984 and others added 3 commits May 14, 2024 02:45

Update cpp/src/arrow/compute/row/grouper.h

7c34623

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Change segment ctor to accept grouper instance

12a0410

Add todo for dictionaries resetting necessity

0cbdb27

pitrou reviewed May 14, 2024

View reviewed changes

cpp/src/arrow/acero/hash_aggregate_test.cc Outdated Show resolved Hide resolved

pitrou reviewed May 14, 2024

View reviewed changes

zanmato1984 and others added 2 commits May 14, 2024 23:07

Update cpp/src/arrow/acero/hash_aggregate_test.cc

536dbce

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Enhance segment test checking

184392e

pitrou approved these changes May 14, 2024

View reviewed changes

pitrou merged commit ada965f into apache:main May 14, 2024

pitrou removed the awaiting committer review Awaiting committer review label May 14, 2024

pitrou mentioned this pull request May 14, 2024

[C++] Add Grouper::Reset #33484

Closed

zanmato1984 added a commit to zanmato1984/arrow that referenced this pull request May 14, 2024

Address a remaining comment in apache#41352

4821398

zanmato1984 mentioned this pull request May 14, 2024

GH-41334: [C++][Acero] Use per-node basis temp vector stack to mitigate overflow #41335

Merged

GH-33484: [C++][Compute] Implement Grouper::Reset #41352

GH-33484: [C++][Compute] Implement Grouper::Reset #41352

Uh oh!

Conversation

zanmato1984 commented Apr 23, 2024 • edited by github-actions bot 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 Apr 23, 2024

Uh oh!

zanmato1984 commented Apr 23, 2024

Uh oh!

github-actions bot commented Apr 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanmato1984 commented May 8, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

conbench-apache-arrow bot commented May 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GH-33484: [C++][Compute] Implement `Grouper::Reset` #41352

GH-33484: [C++][Compute] Implement `Grouper::Reset` #41352

zanmato1984 commented Apr 23, 2024 •

edited by github-actions bot

Loading