GH-37055: [C++] Optimize hash kernels for Dictionary ChunkedArrays

[js8544]

Rationale for this change

When merging dictionaries across chunks, the hash kernels unnecessarily unify the existing dictionary, dragging down the performance.

What changes are included in this PR?

Reuse the dictionary unifier across chunks.

Are these changes tested?

Yes, with a new benchmark for dictionary chunked arrays.

Are there any user-facing changes?

No.

• Closes: [C++][Python] value_counts extremely slow for chunked DictionaryArray #37055

[github-actions]

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

[js8544]

Benchmark result: https://gist.github.com/js8544/d64f289313c814a2df6a87a945a0b382#file-pr38394-txt
Improvement is significant when # of unique is large, i.e. when dictionary size is large.

Also significant improvement in user's case in python: #37055 (comment)

[js8544]

It was mentioned in #9683 (comment) that we can compute the result of each chunk and then merge them with hash_sum. However, since hash aggregate functions are moved to acero. It's less ideal to have compute kernels depend on acero because it's a level higher in the dependency tree.

This PR saves calls of the dictionary unifier, we can also further optimize this by optimizing the unifying process. This will be done once we have a faster hashtable: #38372.

[js8544]

@felipecrv Do you mind having a look at this?

[felipecrv]

@felipecrv Do you mind having a look at this?

Soon!

[felipecrv]

Do we even need dictionary_ to be a member variable now? Wouldn't it suffice to perform a single DictionaryUnifier::GetResult call at the end?

[felipecrv]

Documentation for DictionaryUnifier::GetResult says the unifier can't be re-used after a call to GetResult [1].

My suggestion (that I think will work well):

Rename dictionary_ to first_dictionary_ and change Append(arr) to transition through this state machine on each call:

// --------------------------------------------------------------------------------------------------------------
//  Current State                                     Next State
// --------------------------------------------------------------------------------------------------------------
//  !first_dictionary_ && !dictionary_unifier_   -->  first_dictionary_ = arr_dict_
//                                                    UNCHANGED dictionary_unifier_
// --------------------------------------------------------------------------------------------------------------
//   first_dictionary_ && !dictionary_unifier_   -->  if !first_dictionary_.Equals(arr_dict) then
//                                                       dictionary_unifier_ = unify(first_dictionary_, arr_dict)
//                                                       first_dictionary_ = nullptr
//                                                    else
//                                                       UNCHANGED first_dictionary_, dictionary_unifier_
//                                                    end
// --------------------------------------------------------------------------------------------------------------
                         dictionary_unifier_    -->  dictionary_unifier_ = unify(dictionary_unifier_, arr_dict)

You will then have to re-think how dictionary_value_type and dictionary work below.

[1] https://github.com/apache/arrow/blob/main/cpp/src/arrow/array/array_dict.h#L169

[felipecrv]

@js8544 to clarify, dictionary_unifier_ = unify(dictionary_unifier_, arr_dict) means something like unifier->unify(arr_dict) in the actual code :)

[felipecrv]

I don't see any changes other than the rebase.

[js8544]

I don't see any changes other than the rebase.

Still working on it :）

[js8544]

@felipecrv Updated as you suggested.

[felipecrv]

Thank you!

[felipecrv]

@js8544 I won't merge my own suggestions before you have a chance to reply and apply them yourself. If you're OK with them and CI passes with them applied I will merge.

[felipecrv]

macOS build failing for some unknown reason and this has been rebased very recently, so I'm merging.

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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