Make hash by the value allocated, not by pointer

arangod/Aql/AqlItemBlock.cpp

@@ -34,6 +34,8 @@
 #include "Basics/VelocyPackHelper.h"
 #include "Containers/FlatHashMap.h"
 #include "Containers/HashSet.h"
+#include "Logger/LogMacros.h"
+#include "Logger/Logger.h"
 #include "Transaction/Context.h"
 #include "Transaction/Methods.h"
 
@@ -510,9 +512,24 @@ SharedAqlItemBlockPtr AqlItemBlock::cloneDataAndMoveShadow() {
           if (a.requiresDestruction()) {
             AqlValueGuard guard{a, true};
             auto [it, inserted] = cache.emplace(a.data());
-            res->setValue(row, col, AqlValue(a, (*it)));
             if (inserted) {
-              // otherwise, destroy this; we used a cached value.
+              // First time seeing this pointer - we can reuse it
+              res->setValue(row, col, AqlValue(a, (*it)));
+              // Transfer ownership to res - guard won't destroy it
+              guard.steal();
+            } else {
+              // Value was already cached. This means a previous row/column
+              // already processed a value pointing to this same memory and
+              // called guard.steal(), so the memory is now owned by res
+              // (tracked via _valueCount). We can safely reuse the cached
+              // pointer - setValue will increment the refCount. We MUST call
+              // guard.steal() to prevent a.destroy() from freeing memory that
+              // res already owns.
+              res->setValue(row, col, AqlValue(a, (*it)));
+              // CRITICAL: Call guard.steal() to prevent a.destroy() from
+              // freeing memory that res already owns. Without this, the guard
+              // would free the memory when it goes out of scope, even though
+              // res has other AqlValues pointing to it.
               guard.steal();
             }
           } else {
@@ -805,12 +822,7 @@ void AqlItemBlock::toVelocyPack(size_t from, size_t to,
         currentState = Range;
       } else {
         TRI_ASSERT(pos >= 2);
-        // attempt an insert into the map without checking if the
-        // target element already exists. if it already exists,
-        // then the try_emplace does nothing, but we will get an
-        // iterator to the existing element.
-        // in case we inserted the value, we can simply go on and
-        // increase the global position counter.
+
         auto [it, inserted] = table.try_emplace(a, pos);
 
         if (inserted) {

arangod/Aql/AqlValue.cpp

@@ -29,6 +29,8 @@
 #include "Basics/SupervisedBuffer.h"
 #include "Basics/GlobalResourceMonitor.h"
 #include "Basics/VelocyPackHelper.h"
+#include "Logger/LogMacros.h"
+#include "Logger/Logger.h"
 #include "Transaction/Context.h"
 #include "Transaction/Helpers.h"
 #ifdef USE_V8
@@ -1371,59 +1373,80 @@ namespace std {
 using arangodb::aql::AqlValue;
 
 size_t hash<AqlValue>::operator()(AqlValue const& x) const noexcept {
-  auto t = x.type();
-  switch (t) {
-    case AqlValue::VPACK_INLINE:
-      return static_cast<size_t>(
-          VPackSlice(x._data.inlineSliceMeta.slice).volatileHash());
-    case AqlValue::VPACK_INLINE_INT64:
-    case AqlValue::VPACK_INLINE_UINT64:
-    case AqlValue::VPACK_INLINE_DOUBLE:
-      return static_cast<size_t>(
-          VPackSlice(x._data.longNumberMeta.data.slice.slice).volatileHash());
-      // TODO(MBkkt) these hashes have bad distribution
-    case AqlValue::VPACK_SLICE_POINTER:
-      return std::hash<void const*>()(x._data.slicePointerMeta.pointer);
-    case AqlValue::VPACK_MANAGED_SLICE:
-      return std::hash<void const*>()(x._data.managedSliceMeta.pointer);
-    case AqlValue::VPACK_MANAGED_STRING:
-      return std::hash<void const*>()(x._data.managedStringMeta.pointer);
-    case AqlValue::RANGE:
-      return std::hash<void const*>()(x._data.rangeMeta.range);
+  // Use a non-zero seed to minimize the chance of hash returning 0
+  // 0xdeadbeef is a common "magic number" used as a non-zero seed
+  auto hash64 =
+      x.hash(0xdeadbeef);  // make a normalized hash, for the semantics of the
+                           // value regardless of the storage type
+  size_t h = static_cast<size_t>(hash64);
+  // If hash is 0 (very unlikely with non-zero seed), remap to a large prime
+  // to avoid collision with the marker that uses h == 0. Using a large prime
+  // minimizes collision probability with other hash values.
+  if (h == 0) {
+    // Large prime number, unlikely to collide with other hash values
+    h = 0x9e3779b97f4a7c15ULL;
   }
-  return 0;
+  return h;
 }
 
 bool equal_to<AqlValue>::operator()(AqlValue const& a,
                                     AqlValue const& b) const noexcept {
-  // TODO(MBkkt) can be just compare two uint64_t?
-  auto t = a.type();
-  if (t != b.type()) {
-    return false;
+  using T = AqlValue::AqlValueType;
+  auto ta = a.type();
+  auto tb = b.type();
+
+  if (ta == tb) {
+    switch (ta) {
+      case T::VPACK_INLINE:
+      case T::VPACK_SLICE_POINTER:
+      case T::VPACK_MANAGED_SLICE:
+      case T::VPACK_MANAGED_STRING: {
+        auto sa = a.slice(ta);
+        auto sb = b.slice(tb);
+        TRI_ASSERT(arangodb::velocypack::Options::Defaults.customTypeHandler !=
+                   nullptr)
+            << "VelocyPackHelper must be initialized before AqlValue "
+               "comparison";
+        // handles Custom types and normalized comparison
+        return arangodb::basics::VelocyPackHelper::equal(
+            sa, sb, false, &arangodb::velocypack::Options::Defaults, &sa, &sb);
+      }
+
+      case T::VPACK_INLINE_INT64:
+      case T::VPACK_INLINE_UINT64:
+      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;
+
+      case T::RANGE: {
+        auto const* ra = a._data.rangeMeta.range;
+        auto const* rb = b._data.rangeMeta.range;
+        if (ra == nullptr || rb == nullptr) {
+          return ra == rb;  // both null -> equal, one null -> not equal
+        }
+        return ra->_low == rb->_low && ra->_high == rb->_high;
+      }
+
+      default:
+        // Should not happen
+        TRI_ASSERT(false);
+        return false;
+    }
   }
-  switch (t) {
-    case AqlValue::VPACK_INLINE:
-      return VPackSlice(a._data.inlineSliceMeta.slice)
-          .binaryEquals(VPackSlice(b._data.inlineSliceMeta.slice));
-    case AqlValue::VPACK_INLINE_INT64:
-    case AqlValue::VPACK_INLINE_UINT64:
-    case AqlValue::VPACK_INLINE_DOUBLE:
-      // equal is equal. sign/endianess does not matter
-      return a._data.longNumberMeta.data.intLittleEndian.val ==
-             b._data.longNumberMeta.data.intLittleEndian.val;
-    case AqlValue::VPACK_SLICE_POINTER:
-      return a._data.slicePointerMeta.pointer ==
-             b._data.slicePointerMeta.pointer;
-    case AqlValue::VPACK_MANAGED_SLICE:
-      return a._data.managedSliceMeta.pointer ==
-             b._data.managedSliceMeta.pointer;
-    case AqlValue::VPACK_MANAGED_STRING:
-      return a._data.managedStringMeta.pointer ==
-             b._data.managedStringMeta.pointer;
-    case AqlValue::RANGE:
-      return a._data.rangeMeta.range == b._data.rangeMeta.range;
+
+  if (ta == T::RANGE || tb == T::RANGE) {
+    return false;
   }
-  return false;
+
+  auto sa = a.slice(ta);
+  auto sb = b.slice(tb);
+  // handles Custom types and normalized comparison for different storage types
+  TRI_ASSERT(arangodb::velocypack::Options::Defaults.customTypeHandler !=
+             nullptr)
+      << "VelocyPackHelper must be initialized before AqlValue comparison";
+  return arangodb::basics::VelocyPackHelper::equal(
+      sa, sb, false, &arangodb::velocypack::Options::Defaults, &sa, &sb);
 }
 
 }  // namespace std