Make hash by the value allocated, not by pointer #22129
Conversation
mchacki
left a comment
mchacki
left a comment
There was a problem hiding this comment.
I have some clarifying questions
arangod/Aql/AqlValue.cpp
Outdated
| return std::hash<void const*>()(x._data.rangeMeta.range); | ||
| using T = AqlValue::AqlValueType; | ||
| auto aqlValueType = x.type(); | ||
| // as this is non owning, we hash by the pointer |
There was a problem hiding this comment.
What does the owning make for a difference?
Hashing is a quick look of the content is equal.
In my opinion it should not take into account if the value is owned or not.
arangod/Aql/AqlValue.cpp
Outdated
| if (h == 0) { // fallback to avoid collision with the marker that uses h == | ||
| // 0, very unlikely to happen | ||
| h = 1; | ||
| } |
arangod/Aql/AqlValue.cpp
Outdated
| return a._data.slicePointerMeta.pointer == | ||
| b._data.slicePointerMeta.pointer; |
There was a problem hiding this comment.
Why is this pointer comparison?
And not real equality comparison like the default case?
… semantically equal values
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| cache.emplace(a.data()); | ||
| res->setValue(row, col, a); | ||
| // Transfer ownership to res - guard won't destroy it | ||
| guard.steal(); |
There was a problem hiding this comment.
Bug: Shadow row deduplication removed while cache remains populated
The shadow row handling now calls cache.emplace(a.data()) but ignores the return value and always transfers ownership via guard.steal(). The old code used the emplace result to deduplicate: when a pointer was already cached, it would destroy the duplicate value and reuse the cached one. The new code removes this deduplication but still populates the cache, suggesting incomplete refactoring. If multiple shadow row cells share the same underlying data pointer, this could lead to multiple AqlValue objects referencing the same memory, potentially causing memory management issues.
| case T::VPACK_INLINE_DOUBLE: | ||
| // long numbers are stored in the same form, so we can compare raw bits | ||
| return a._data.longNumberMeta.data.intLittleEndian.val == | ||
| b._data.longNumberMeta.data.intLittleEndian.val; |
There was a problem hiding this comment.
Bug: Inline double equality uses raw bits incorrectly
The equal_to<AqlValue> comparison for VPACK_INLINE_INT64, VPACK_INLINE_UINT64, and VPACK_INLINE_DOUBLE uses raw bit comparison (intLittleEndian.val == intLittleEndian.val). This is problematic for doubles because -0.0 and +0.0 have different bit patterns but are semantically equal in IEEE 754 (-0.0 == +0.0 is true). The hash function uses normalizedHash() which likely normalizes these values to the same hash. This could violate the hash/equality contract: two values might hash to the same bucket but compare unequal, causing incorrect behavior in hash maps/sets used for deduplication.
Scope & Purpose
In AqlValue.cpp, there's a hashing function and a comparator. An AqlValue object can have different types of values, inline, slice pointers, managed slices and managed strings (and, in the future, supervised slice). Inline values are non-allocated values that are distributed within the AqlValue's 16 bytes of available space, slice pointers are pointers to values not owned by the AqlValue object, and managed slices and strings are dynamically allocated payloads owned by the AqlValue object. Even if multiple aql values point to the same managed slice, still, it's owned by AqlValue, and the last AqlValue that points to it to be destroy has to free this allocated memory. The hashing and comparator are used in AqlItemBlock for deduplication, when inserting the AqlValue on a block, so it has to compare if two aql values are equal.
Now onto the hashing, before, the hash was made with the pointers, not the data pointed to by the pointers. The data that the pointer stores is the address of the payload it points to, hence, if two aql values that own memory were compared using this approach, they would never be considered equal, and they could be, because, even though having memory allocated in different addresses, the payload that each store could be semantically the same. This would lead to the comparison used in AqlItemBlock not working properly, and wrongfully distinguishing values that should be considered the same in practice.
The new approach is to hash by the value stored in the address in cases where there's dynamic allocation.
When the value is inline, it's safe to maintain the original approach.
When the value is of type slice pointer, it means AqlValue doesn't own it, hence, the pointers should be hashed. If they point to the same address, then they could be considered the same.
Checklist
Related Information
(Please reference tickets / specification / other PRs etc)
Note
Switches AqlValue hash/equality to normalized content-based semantics (incl. ranges) and updates AqlItemBlock cloning/serialization accordingly, adding comprehensive tests.
hash()(incl.RANGE), addkDefaultSeed, and remap zero hash.std::hash<AqlValue>andstd::equal_to<AqlValue>to use VelocyPack normalized comparison across storage types; compare inlines by value;RANGEby bounds.toVelocyPack(...): usesFlatHashMap<AqlValue, size_t>with value-based keys; minor cleanup.cloneDataAndMoveShadow(...): simplify shadow-row handling to set values and transfer ownership; cache by pointer only for cloning optimization.AqlValueHashAlgorithmCorrectnessTest,AqlValueHashEqualTest, andAqlValueHashTestvalidating content-based hashing/equality and block (de)serialization behavior.tests/CMakeLists.txt.Written by Cursor Bugbot for commit d2def76. This will update automatically on new commits. Configure here.