pitrou commented on code in PR #45515:
URL: https://github.com/apache/arrow/pull/45515#discussion_r1957839485
##########
cpp/src/arrow/acero/swiss_join.cc:
##########
@@ -696,17 +697,18 @@ void SwissTableMerge::MergePartition(SwissTable* target,
const SwissTable* sourc
}
}
-inline bool SwissTableMerge::InsertNewGroup(SwissTable* target, uint64_t
group_id,
- uint32_t hash, int64_t
max_block_id) {
+inline bool SwissTableMerge::InsertNewGroup(SwissTable* target, uint32_t
group_id,
+ uint32_t hash, uint32_t
max_block_id) {
// Load the first block to visit for this hash
//
- int64_t block_id = hash >> (SwissTable::bits_hash_ - target->log_blocks());
- int64_t block_id_mask = ((1LL << target->log_blocks()) - 1);
+ uint32_t block_id = SwissTable::block_id_from_hash(hash,
target->log_blocks());
+ uint32_t block_id_mask = (1 << target->log_blocks()) - 1;
Review Comment:
Would a signed `int` shift be UB if `target->log_blocks()` is 31? Or does
that not happen anyway?
##########
cpp/src/arrow/compute/key_map_internal.cc:
##########
@@ -297,8 +289,9 @@ uint64_t SwissTable::num_groups_for_resize() const {
}
}
-uint64_t SwissTable::wrap_global_slot_id(uint64_t global_slot_id) const {
- uint64_t global_slot_id_mask = (1 << (log_blocks_ + 3)) - 1;
+uint32_t SwissTable::wrap_global_slot_id(uint32_t global_slot_id) const {
+ uint32_t global_slot_id_mask =
+ static_cast<uint32_t>((1ULL << (log_blocks_ + kLogSlotsPerBlock)) -
1ULL);
Review Comment:
Interestingly we're still using a 64-bit unsigned shift here (see previous
comment).
--
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]
