GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA

[felipecrv]

Rationale for this change

take concatenates chunks when it's applied to a chunked values array, but when the indices arrays is also chunked it concatenates values more than once -- one Concatenate call with values.chunks() for every chunk in indices. This PR doesn't remove the concatenation, but ensures it's done only once instead of indices.size() times.

What changes are included in this PR?

• Adding return type to the TakeXX names (-> TakeXXY) to makes code easier to understand
• Adding benchmarks to TakeCCC — copied from ARROW-9773: [C++] Implement Take kernel for ChunkedArray #13857
• Remove the concatenation from the loop body (!)

Are these changes tested?

By existing tests.

Are there any user-facing changes?

A faster compute kernel.

• GitHub Issue: [C++] TakeCC is doing indices.num_chunks() Concatenate() calls when it could be doing only one #40207

[github-actions]

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

[felipecrv]

Items per second improved a lot. Higher is better.

EDIT: these numbers come from the initial version of the benchmarks (10 chunks per array and a 10x bigger indices array relative to values).

[felipecrv]

Making the only difference between the benchmarks, the fact that the array is chunked (in a 100 chunks) makes the differences more evident.

[js8544]

+1. Thanks!

[felipecrv]

@js8544 thank you for the review. I pushed a revert for the last commit because it isn't necessary for this PR and might conflict with a PR someone else is working on.

If not one objects, I will probably merge this right before I'm ready to send more Take-related PRs.

[pitrou]

Why not add at least "ChunkedChunked" variations for each of these types as well? (FSB, FSL, String). I'm assuming the performance characteristics might be different?

[pitrou]

("ChunkedFlat" is less interesting arguably)

[pitrou]

@felipecrv Would you like to rebase if not already done?

[felipecrv]

@felipecrv Would you like to rebase if not already done?

Soon. I'm currently working on top of this branch locally 😅

[conbench-apache-arrow]

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

There were 2 benchmark results with an error:

• Commit Run on ursa-i9-9960x at 2024-02-28 12:08:00Z
  • tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-12, scale_factor=10
  • tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-05, scale_factor=10

There were 13 benchmark results indicating a performance regression:

• Commit Run on ursa-i9-9960x at 2024-02-28 12:08:00Z
  • tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-20, scale_factor=1
  • tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-01, scale_factor=1
• and 11 more (see the report linked below)

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.