apply feedback

src/engine/SpatialJoin.cpp

@@ -48,15 +48,24 @@ SpatialJoin::SpatialJoin(
 std::shared_ptr<SpatialJoin> SpatialJoin::addChild(
     std::shared_ptr<QueryExecutionTree> child,
     const Variable& varOfChild) const {
+  std::shared_ptr<SpatialJoin> sj;
   if (varOfChild == config_.left_) {
-    return std::make_shared<SpatialJoin>(getExecutionContext(), config_,
-                                         std::move(child), childRight_);
+    sj = std::make_shared<SpatialJoin>(getExecutionContext(), config_,
+                                       std::move(child), childRight_);
   } else if (varOfChild == config_.right_) {
-    return std::make_shared<SpatialJoin>(getExecutionContext(), config_,
-                                         childLeft_, std::move(child));
+    sj = std::make_shared<SpatialJoin>(getExecutionContext(), config_,
+                                       childLeft_, std::move(child));
   } else {
     AD_THROW("variable does not match");
   }
+
+  // The new spatial join after adding a child needs to inherit the warnings of
+  // its predecessor.
+  for (const auto& warning : getWarnings()) {
+    sj->addWarning(warning);
+  }
+
+  return sj;
 }
 
 // ____________________________________________________________________________
@@ -127,7 +136,8 @@ string SpatialJoin::getCacheKeyImpl() const {
 
     // Selected payload variables
     os << "payload:";
-    auto rightVarColFiltered = getVarColMapPayloadVars();
+    auto rightVarColFiltered =
+        copySortedByColumnIndex(getVarColMapPayloadVars());
     for (const auto& [var, info] : rightVarColFiltered) {
       os << " " << info.columnIndex_;
     }
@@ -315,25 +325,29 @@ VariableToColumnMap SpatialJoin::getVarColMapPayloadVars() const {
                     "column map can be computed.");
 
   auto payloadVariablesVisitor = [this]<typename T>(const T& value) {
-    auto varColMap = childRight_->getVariableColumns();
+    const auto& varColMap = childRight_->getVariableColumns();
     VariableToColumnMap newVarColMap;
 
     if constexpr (std::is_same_v<T, PayloadAllVariables>) {
       newVarColMap = VariableToColumnMap{varColMap};
     } else {
       static_assert(std::is_same_v<T, std::vector<Variable>>);
       for (const auto& var : value) {
-        AD_CONTRACT_CHECK(varColMap.contains(var), [&]() {
-          return absl::StrCat("Variable '", var.name(),
-                              "' selected as payload to spatial join but not "
-                              "present in right child.");
-        });
-        newVarColMap[var] = varColMap[var];
+        if (varColMap.contains(var)) {
+          newVarColMap.insert_or_assign(var, varColMap.at(var));
+        } else {
+          // TODO<ullingerc> Find solution here: const - not-const
+          // addWarningOrThrow(
+          AD_THROW(absl::StrCat("Variable '", var.name(),
+                                "' selected as payload to spatial join but not "
+                                "present in right child."));
+        }
       }
       AD_CONTRACT_CHECK(
           varColMap.contains(config_.right_),
           "Right variable is not defined in right child of spatial join.");
-      newVarColMap[config_.right_] = varColMap[config_.right_];
+      newVarColMap.insert_or_assign(config_.right_,
+                                    varColMap.at(config_.right_));
     }
     return newVarColMap;
   };
@@ -349,8 +363,9 @@ PreparedSpatialJoinParams SpatialJoin::prepareJoin() const {
     return std::pair{idTablePtr, std::move(resTable)};
   };
 
-  auto getJoinCol = [](VariableToColumnMap varColMap,
+  auto getJoinCol = [](std::shared_ptr<QueryExecutionTree> child,
                        const Variable& childVariable) {
+    VariableToColumnMap varColMap = child->getVariableColumns();
     return varColMap[childVariable].columnIndex_;
   };
 
@@ -359,10 +374,8 @@ PreparedSpatialJoinParams SpatialJoin::prepareJoin() const {
   auto [idTableRight, resultRight] = getIdTable(childRight_);
 
   // Input table columns for the join
-  ColumnIndex leftJoinCol =
-      getJoinCol(childLeft_->getVariableColumns(), config_.left_);
-  VariableToColumnMap rightVarColMap = childRight_->getVariableColumns();
-  ColumnIndex rightJoinCol = getJoinCol(rightVarColMap, config_.right_);
+  ColumnIndex leftJoinCol = getJoinCol(childLeft_, config_.left_);
+  ColumnIndex rightJoinCol = getJoinCol(childRight_, config_.right_);
 
   // Payload cols and join col
   auto varsAndColInfo = copySortedByColumnIndex(getVarColMapPayloadVars());

src/parser/MagicServiceQuery.cpp

@@ -68,8 +68,8 @@ std::string_view MagicServiceQuery::extractParameterName(
   }
 
   // Get IRI without brackets
-  auto iri =
-      ad_utility::triple_component::Iri::fromIriref(magicIRI).getContent();
+  auto iriObj = ad_utility::triple_component::Iri::fromIriref(magicIRI);
+  auto iri = iriObj.getContent();
 
   // Remove prefix if applicable: this allows users to define the parameter
   // either as magicServicePrefix:parameterName or <parameterName>.

src/parser/SpatialQuery.cpp

@@ -146,6 +146,8 @@ SpatialQuery::SpatialQuery(const SparqlTriple& triple) {
   // Helper to convert a ctre match to an integer
   auto matchToInt = [](std::string_view match) -> std::optional<size_t> {
     if (match.size() == 0) {
+      // We need to accept empty matches because the maximum distance argument
+      // to a <nearest-neighbors:...> predicate is optional.
       return std::nullopt;
     }
     size_t res = 0;

src/parser/sparqlParser/SparqlQleverVisitor.cpp

@@ -863,10 +863,8 @@ GraphPatternOperation Visitor::visitSpatialQuery(
     // its result here to detect errors early and report them to the user with
     // highlighting. It's only a small struct so not much is wasted.
     spatialQuery.toSpatialJoinConfiguration();
-  } catch (const parsedQuery::MagicServiceException& magicExc) {
-    reportError(ctx, magicExc.what());
-  } catch (const parsedQuery::SpatialSearchException& magicExc) {
-    reportError(ctx, magicExc.what());
+  } catch (const std::exception& ex) {
+    reportError(ctx, ex.what());
   }
 
   return spatialQuery;

test/QueryPlannerTest.cpp

@@ -2195,23 +2195,42 @@ TEST(QueryPlanner, SpatialJoinLegacyPredicateSupport) {
                                          ::testing::_),
                                ::testing::ContainsRegex("unknown triple"));
 
-  // Nearest neighbors special predicate
+  // Test that the nearest neighbors special predicate is still accepted but
+  // produces a warning
   h::expect(
       "SELECT ?x ?y WHERE {"
       "?x <p> ?y."
       "?a <p> ?b."
       "?y <nearest-neighbors:2:500> ?b }",
-      h::SpatialJoin(500, 2, V{"?y"}, V{"?b"}, std::nullopt,
-                     PayloadAllVariables{}, S2, scan("?x", "<p>", "?y"),
-                     scan("?a", "<p>", "?b")));
+      h::QetWithWarnings(
+          {"special predicate <nearest-neighbors:...> is deprecated"},
+          h::SpatialJoin(500, 2, V{"?y"}, V{"?b"}, std::nullopt,
+                         PayloadAllVariables{}, S2, scan("?x", "<p>", "?y"),
+                         scan("?a", "<p>", "?b"))));
   h::expect(
       "SELECT ?x ?y WHERE {"
       "?x <p> ?y."
       "?a <p> ?b."
       "?y <nearest-neighbors:20> ?b }",
-      h::SpatialJoin(-1, 20, V{"?y"}, V{"?b"}, std::nullopt,
-                     PayloadAllVariables{}, S2, scan("?x", "<p>", "?y"),
-                     scan("?a", "<p>", "?b")));
+      h::QetWithWarnings(
+          {"special predicate <nearest-neighbors:...> is deprecated"},
+          h::SpatialJoin(-1, 20, V{"?y"}, V{"?b"}, std::nullopt,
+                         PayloadAllVariables{}, S2, scan("?x", "<p>", "?y"),
+                         scan("?a", "<p>", "?b"))));
+
+  AD_EXPECT_THROW_WITH_MESSAGE(h::expect("SELECT ?x ?y WHERE {"
+                                         "?x <p> ?y."
+                                         "?a <p> ?b."
+                                         "?y <nearest-neighbors:1:-200> ?b }",
+                                         ::testing::_),
+                               ::testing::ContainsRegex("unknown triple"));
+
+  AD_EXPECT_THROW_WITH_MESSAGE(h::expect("SELECT ?x ?y WHERE {"
+                                         "?x <p> ?y."
+                                         "?a <p> ?b."
+                                         "?y <nearest-neighbors:0:-1> ?b }",
+                                         ::testing::_),
+                               ::testing::ContainsRegex("unknown triple"));
 
   EXPECT_ANY_THROW(
       h::expect("SELECT ?x ?y WHERE {"

test/engine/SpatialJoinTest.cpp

@@ -5,7 +5,6 @@
 
 #include <absl/strings/str_cat.h>
 #include <gmock/gmock.h>
-#include <gtest/gtest.h>
 #include <s2/s2earth.h>
 #include <s2/s2point.h>
 
@@ -15,6 +14,8 @@
 #include <regex>
 #include <sstream>
 
+#include "../printers/VariablePrinters.h"
+#include "../printers/VariableToColumnMapPrinters.h"
 #include "../util/GTestHelpers.h"
 #include "../util/IndexTestHelpers.h"
 #include "./../../src/global/ValueId.h"
@@ -28,8 +29,6 @@
 #include "engine/SpatialJoin.h"
 #include "engine/VariableToColumnMap.h"
 #include "global/Constants.h"
-#include "gmock/gmock.h"
-#include "gtest/gtest.h"
 #include "parser/data/Variable.h"
 
 namespace {  // anonymous namespace to avoid linker problems
@@ -364,19 +363,6 @@ class SpatialJoinVarColParamTest
     }
   }
 
-  // Helper for debugging. Currently unused in the tests, but test error
-  // messages from gtest are not helpful, so you may use this function to
-  // gain better insight in a variable to column map.
-  std::string stringifyVariableToColumnMap(VariableToColumnMap vc) {
-    std::string result = "";
-    auto vec = copySortedByColumnIndex(vc);
-    for (auto [var, info] : vec) {
-      result = absl::StrCat(result, "(", var.name(), " => ", info.columnIndex_,
-                            " ; ", info.mightContainUndef_, ")");
-    }
-    return result;
-  }
-
   // Function to test the variable to column map that is generated by spatial
   // join with the left and right children as given by the parameters. It will
   // try all possible, valid payloadVariables settings and check the variable
@@ -391,14 +377,13 @@ class SpatialJoinVarColParamTest
 
     auto computeAndCompareVarToColMaps =
         [&](bool addDist, PayloadVariables pv,
-            VariableToColumnMap expectedVarToColVec) {
+            VariableToColumnMap expectedVarToColMap) {
           auto qec = buildTestQEC();
           auto spJoin2 = makeSpatialJoin(qec, parameters, addDist, pv);
           auto spatialJoin = static_cast<SpatialJoin*>(spJoin2.get());
           auto vc = spatialJoin->computeVariableToColumnMap();
-          VarToColVec vcVec = copySortedByColumnIndex(vc);
-          VarToColVec expVcVec = copySortedByColumnIndex(expectedVarToColVec);
-          ASSERT_THAT(vcVec, ::testing::UnorderedElementsAreArray(expVcVec));
+          ASSERT_THAT(
+              vc, ::testing::UnorderedElementsAreArray(expectedVarToColMap));
         };
 
     // Expected variables
@@ -500,6 +485,7 @@ class SpatialJoinVarColParamTest
             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),