GH-32381: [C++] Improve error handling for hash table merges#49512
GH-32381: [C++] Improve error handling for hash table merges#49512kris-gaudel wants to merge 3 commits intoapache:mainfrom
Conversation
|
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? or See also: |
kris-gaudel
changed the title
zanmato1984
left a comment
zanmato1984
left a comment
There was a problem hiding this comment.
Thanks — the fix direction looks right to me overall, but I think there are two follow-ups worth addressing inline.
| ASSERT_EQ(table.size(), map.size()); | ||
| } | ||
|
|
||
| TEST(ScalarMemoTable, MergeTablePropagatesInsertError) { |
There was a problem hiding this comment.
Since this PR changes both ScalarMemoTable::MergeTable() and BinaryMemoTable::MergeTable(), could we add a matching regression test for the binary path too? Right now the new coverage only protects the scalar merge case.
| Status status = Status::OK(); | ||
|
|
||
| other_hashtable.VisitEntries([this](const HashTableEntry* other_entry) { | ||
| other_hashtable.VisitEntries([this, &status](const HashTableEntry* other_entry) { |
There was a problem hiding this comment.
Would it make sense to have VisitEntries(...) propagate callback failures, so this can just be RETURN_NOT_OK(other_hashtable.VisitEntries(...))? That would avoid the manual Status status plumbing here. If changing VisitEntries is too broad, a status-aware variant/helper could work too.
2 participants
Rationale for this change
Fixes #32381
What changes are included in this PR?
Proper error handling in
util/hashing.hand unit test for said error handling inutil/hashing_test.ccAre these changes tested?
Yes, via included unit test
Are there any user-facing changes?
Yes, error handling in hashing utilities