zanmato1984 commented on code in PR #49512:
URL: https://github.com/apache/arrow/pull/49512#discussion_r2937405800
##########
cpp/src/arrow/util/hashing.h:
##########
@@ -534,13 +534,16 @@ class ScalarMemoTable : public MemoTable {
// Merge entries from `other_table` into `this->hash_table_`.
Status MergeTable(const ScalarMemoTable& other_table) {
const HashTableType& other_hashtable = other_table.hash_table_;
+ Status status = Status::OK();
- other_hashtable.VisitEntries([this](const HashTableEntry* other_entry) {
+ other_hashtable.VisitEntries([this, &status](const HashTableEntry*
other_entry) {
Review 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.
##########
cpp/src/arrow/util/hashing_test.cc:
##########
@@ -376,6 +377,32 @@ TEST(ScalarMemoTable, StressInt64) {
ASSERT_EQ(table.size(), map.size());
}
+TEST(ScalarMemoTable, MergeTablePropagatesInsertError) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
