apply feedback

ad-freiburg · Nov 30, 2024 · 456dc02 · 456dc02
1 parent c845136
commit 456dc02
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 32 deletions.
diff --git a/src/engine/Operation.cpp b/src/engine/Operation.cpp
@@ -51,7 +51,7 @@ vector<string> Operation::collectWarnings() const {
 }
 
 // _____________________________________________________________________________
-void Operation::addWarningOrThrow(std::string warning) {
+void Operation::addWarningOrThrow(std::string warning) const {
   if (RuntimeParameters().get<"throw-on-unbound-variables">()) {
     throw InvalidSparqlQueryException(std::move(warning));
   } else {

diff --git a/src/engine/Operation.h b/src/engine/Operation.h
@@ -64,14 +64,16 @@ class Operation {
 
   // Add a warning to the `Operation`. The warning will be returned by
   // `collectWarnings()` above.
-  void addWarning(std::string warning) {
+  void addWarning(std::string warning) const {
+    // TODO<ullingerc>
+    // warnings_.wlock()->push_back(std::move(warning));
     warnings_.push_back(std::move(warning));
   }
 
   // If unbound variables that are used in a query are supposed to throw because
   // the corresponding `RuntimeParameter` is set, then throw. Else add a
   // warning.
-  void addWarningOrThrow(std::string warning);
+  void addWarningOrThrow(std::string warning) const;
 
   /**
    * @return A list of columns on which the result of this operation is sorted.
@@ -254,6 +256,7 @@ class Operation {
   [[nodiscard]] virtual vector<ColumnIndex> resultSortedOn() const = 0;
 
   /// interface to the generated warnings of this operation
+  // TODO<ullingerc>
   std::vector<std::string>& getWarnings() { return warnings_; }
   [[nodiscard]] const std::vector<std::string>& getWarnings() const {
     return warnings_;
@@ -384,8 +387,11 @@ class Operation {
   RuntimeInformationWholeQuery _runtimeInfoWholeQuery;
 
   // Collect all the warnings that were created during the creation or
-  // execution of this operation.
-  std::vector<std::string> warnings_;
+  // execution of this operation. This attribute is declared mutable in order to
+  // allow const-functions in subclasses of Operation to add warnings.
+  // TODO<ullingerc> use a
+  // ad_utility::Synchronized<std::vector<std::string>, std::shared_mutex>?
+  mutable std::vector<std::string> warnings_;
 
   // The limit from a SPARQL `LIMIT` clause.
 

diff --git a/test/engine/SpatialJoinTest.cpp b/test/engine/SpatialJoinTest.cpp
@@ -29,6 +29,7 @@
 #include "engine/SpatialJoin.h"
 #include "engine/VariableToColumnMap.h"
 #include "global/Constants.h"
+#include "gmock/gmock.h"
 #include "parser/data/Variable.h"
 
 namespace {  // anonymous namespace to avoid linker problems
@@ -377,13 +378,19 @@ class SpatialJoinVarColParamTest
 
     auto computeAndCompareVarToColMaps =
         [&](bool addDist, PayloadVariables pv,
-            VariableToColumnMap expectedVarToColMap) {
+            VariableToColumnMap expectedVarToColMap,
+            std::optional<std::string> matchWarning = std::nullopt) {
           auto qec = buildTestQEC();
           auto spJoin2 = makeSpatialJoin(qec, parameters, addDist, pv);
           auto spatialJoin = static_cast<SpatialJoin*>(spJoin2.get());
           auto vc = spatialJoin->computeVariableToColumnMap();
           ASSERT_THAT(
               vc, ::testing::UnorderedElementsAreArray(expectedVarToColMap));
+          if (matchWarning) {
+            ASSERT_THAT(spatialJoin->collectWarnings(),
+                        ::testing::Contains(
+                            ::testing::HasSubstr(matchWarning.value())));
+          }
         };
 
     // Expected variables
@@ -466,43 +473,36 @@ class SpatialJoinVarColParamTest
             computeAndCompareVarToColMaps(addDist, payloadVars, exp);
             payloadVars.pop_back();
 
-            // Test throw if left join variable contained
+            // Test warnings for unbound variables
             payloadVars.push_back(Variable{"?point1"});
-            AD_EXPECT_THROW_WITH_MESSAGE(
-                computeAndCompareVarToColMaps(addDist, payloadVars, exp),
-                ::testing::HasSubstr(
-                    "Variable '?point1' selected as payload to "
-                    "spatial join but not present in right child"));
+            computeAndCompareVarToColMaps(
+                addDist, payloadVars, exp,
+                "Variable '?point1' selected as payload to "
+                "spatial join but not present in right child");
             payloadVars.pop_back();
 
-            // Test throw if left other variable contained
             payloadVars.push_back(Variable{"?obj1"});
-            AD_EXPECT_THROW_WITH_MESSAGE(
-                computeAndCompareVarToColMaps(addDist, payloadVars, exp),
-                ::testing::HasSubstr(
-                    "Variable '?obj1' selected as payload to "
-                    "spatial join but not present in right child"));
+            computeAndCompareVarToColMaps(
+                addDist, payloadVars, exp,
+                "Variable '?obj1' selected as payload to "
+                "spatial join but not present in right child");
             payloadVars.pop_back();
 
-            // Test throw if unbound contained
-            // TODO<ullingerc> Switch to warning
             payloadVars.push_back(Variable{"?isThereSomebodyHere"});
-            AD_EXPECT_THROW_WITH_MESSAGE(
-                computeAndCompareVarToColMaps(addDist, payloadVars, exp),
-                ::testing::HasSubstr(
-                    "Variable '?isThereSomebodyHere' selected as payload to "
-                    "spatial join but not present in right child"));
+            computeAndCompareVarToColMaps(
+                addDist, payloadVars, exp,
+                "Variable '?isThereSomebodyHere' selected as payload to "
+                "spatial join but not present in right child");
             payloadVars.pop_back();
 
-            // Test throw if distance variable contained
+            // Test if distance variable contained
             if (addDist) {
               payloadVars.push_back(distVar);
-              AD_EXPECT_THROW_WITH_MESSAGE(
-                  computeAndCompareVarToColMaps(addDist, payloadVars, exp),
-                  ::testing::HasSubstr(
-                      "Variable '?distOfTheTwoObjectsAddedInternally' selected "
-                      "as payload to spatial join but not present in right "
-                      "child"));
+              computeAndCompareVarToColMaps(
+                  addDist, payloadVars, exp,
+                  "Variable '?distOfTheTwoObjectsAddedInternally' selected "
+                  "as payload to spatial join but not present in right "
+                  "child");
               payloadVars.pop_back();
             }
           }