From c5e6c80f9cb9370db436c94f059afee2e302eec7 Mon Sep 17 00:00:00 2001
From: Johannes Kalmbach <joka921@users.noreply.github.com>
Date: Sun, 5 Jan 2025 22:18:23 +0100
Subject: [PATCH] Don't use Abseil for `Hash(Set|Map)WithMemoryLimit` (#1689)

So far, `HashSetWithMemoryLimit` and `HashMapWithMemoryLimit` were implemented as `absl::flat_hash_set` and `absl::flat_hash_map`, respectively. However, the Abseil data structures are not exception safe, which potentially leads to unexpected or erroneous behavior in Qlever. The two data structures are now implemented using `std::unordered_set`.

These two classes are currently used in the following operations: `GroupBy`, `TransitivePath`, and `Describe`. A quick performance comparison (of the current master and this PR on 20 example queries, and in a small standalone program), shows no significant performance difference.
---
 src/util/HashMap.h   | 12 +++++++++---
 src/util/HashSet.h   | 10 +++++-----
 test/CMakeLists.txt  |  2 +-
 test/HashMapTest.cpp |  2 +-
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/src/util/HashMap.h b/src/util/HashMap.h
index 2a6b94e2e9..39c8a7a4ca 100644
--- a/src/util/HashMap.h
+++ b/src/util/HashMap.h
@@ -6,7 +6,10 @@
 #pragma once
 
 #include <absl/container/flat_hash_map.h>
-#include <util/AllocatorWithLimit.h>
+
+#include <unordered_map>
+
+#include "util/AllocatorWithLimit.h"
 
 namespace ad_utility {
 // Wrapper for HashMaps to be used everywhere throughout code for the semantic
@@ -15,10 +18,13 @@ namespace ad_utility {
 template <typename... Ts>
 using HashMap = absl::flat_hash_map<Ts...>;
 
-// A HashMap with a memory limit.
+// A HashMap with a memory limit. Note: We cannot use `absl::flat_hash_map`
+// here, because it is inherently not exception safe, and the
+// `AllocatorWithLimit` uses exceptions.
 template <class K, class V,
           class HashFct = absl::container_internal::hash_default_hash<K>,
           class EqualElem = absl::container_internal::hash_default_eq<K>,
           class Alloc = ad_utility::AllocatorWithLimit<std::pair<const K, V>>>
-using HashMapWithMemoryLimit = HashMap<K, V, HashFct, EqualElem, Alloc>;
+using HashMapWithMemoryLimit =
+    std::unordered_map<K, V, HashFct, EqualElem, Alloc>;
 }  // namespace ad_utility
diff --git a/src/util/HashSet.h b/src/util/HashSet.h
index 4cf9bd56f5..a851de6fd7 100644
--- a/src/util/HashSet.h
+++ b/src/util/HashSet.h
@@ -6,13 +6,11 @@
 
 #pragma once
 
-#include <string>
+#include <unordered_set>
 
 #include "absl/container/flat_hash_set.h"
 #include "util/AllocatorWithLimit.h"
 
-using std::string;
-
 namespace ad_utility {
 // Wrapper for HashSets (with elements of type T) to be used everywhere
 // throughout code for the semantic search. This wrapper interface is not
@@ -25,11 +23,13 @@ template <class T,
 using HashSet = absl::flat_hash_set<T, HashFct, EqualElem, Alloc>;
 
 // A hash set (with elements of type T) with a memory Limit.
+// Note: We cannot use `absl::flat_hash_set`
+// here, because it is inherently not exception safe, and the
+// `AllocatorWithLimit` uses exceptions.
 template <class T,
           class HashFct = absl::container_internal::hash_default_hash<T>,
           class EqualElem = absl::container_internal::hash_default_eq<T>,
           class Alloc = ad_utility::AllocatorWithLimit<T>>
-using HashSetWithMemoryLimit =
-    absl::flat_hash_set<T, HashFct, EqualElem, Alloc>;
+using HashSetWithMemoryLimit = std::unordered_set<T, HashFct, EqualElem, Alloc>;
 
 }  // namespace ad_utility
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 46a83d5b7a..bd375f4826 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -160,7 +160,7 @@ addLinkAndDiscoverTest(TextLimitOperationTest engine)
 
 addLinkAndDiscoverTestSerial(QueryPlannerTest engine)
 
-addLinkAndDiscoverTest(HashMapTest)
+addLinkAndDiscoverTestNoLibs(HashMapTest)
 
 addLinkAndDiscoverTest(HashSetTest)
 
diff --git a/test/HashMapTest.cpp b/test/HashMapTest.cpp
index 7ce7934f9c..cb1d670fe6 100644
--- a/test/HashMapTest.cpp
+++ b/test/HashMapTest.cpp
@@ -8,7 +8,7 @@
 #include <utility>
 #include <vector>
 
-#include "../src/util/HashMap.h"
+#include "util/HashMap.h"
 
 // Note: Since the HashMap class is a wrapper for a well tested hash map
 // implementation the following tests only check the API for functionality and