alamb commented on code in PR #25218:
URL: https://github.com/apache/datafusion/pull/25218#discussion_r3996141942
##########
datafusion/physical-expr-common/src/binary_map.rs:
##########
@@ -511,12 +511,21 @@ where
else {
// Check if the value is already present in the set
let entry = self.map.find_mut(hash, |header| {
- // compare value if hashes match
- if header.hash != hash {
+ // Compare the value only when the hashes match and the
+ // existing entry is itself a long value. The length check
Review Comment:
I think the comemnts
"The length check
// is what makes reading the buffer below sound: a short
// entry keeps its bytes inlined in `offset_or_inline`
// rather than an offset, so on a hash collision between
a
// short and a long value, `header.range()` would be
built
// from those bytes and point outside the buffer.
"
are somewhat redundant and describe implementation elsewhere and could
probably be removed
The point is that if the hashes match, we also need to check the length
before trying to compare the bytes
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
