A first round of working unit tests.

Signed-off-by: Johannes Kalmbach <[email protected]>
ad-freiburg · Nov 13, 2024 · bd2840f · bd2840f
1 parent 777d349
commit bd2840f
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 22 deletions.
diff --git a/src/engine/QueryPlanner.cpp b/src/engine/QueryPlanner.cpp
@@ -209,6 +209,7 @@ QueryExecutionTree QueryPlanner::createExecutionTree(ParsedQuery& pq,
     for (const auto& warning : warnings_) {
       rootOperation.addWarning(warning);
     }
+    warnings_.clear();
     return result;
   } catch (ad_utility::CancellationException& e) {
     e.setOperation("Query planning");

diff --git a/src/parser/ParsedQuery.cpp b/src/parser/ParsedQuery.cpp
@@ -447,28 +447,19 @@ void ParsedQuery::addOrderByClause(OrderClause orderClause, bool isGroupBy,
   auto processVariableOrderKey = [this, isGroupBy, &noteForImplicitGroupBy,
                                   &variablesFromAliases,
                                   additionalError](VariableOrderKey orderKey) {
-    checkVariableIsVisible(orderKey.variable_, "ORDER BY", variablesFromAliases,
-                           additionalError);
-
-    // Check whether grouping is done. The variable being ordered by
-    // must then be either grouped or the result of an alias in the select
-    // clause.
-    if (isGroupBy &&
-        !ad_utility::contains(_groupByVariables, orderKey.variable_) &&
-        (!variablesFromAliases.contains(orderKey.variable_))) {
-      /*
-      throw InvalidSparqlQueryException(
-          "Variable " + orderKey.variable_.name() +
+    if (!isGroupBy) {
+      checkVariableIsVisible(orderKey.variable_, "ORDER BY",
+                             variablesFromAliases, additionalError);
+    } else if (!ad_utility::contains(_groupByVariables, orderKey.variable_) &&
+               (!variablesFromAliases.contains(orderKey.variable_))) {
+      // Check whether grouping is done. The variable being ordered by
+      // must then be either grouped or the result of an alias in the select
+      // clause.
+      addWarning(absl::StrCat(
+          "Variable " + orderKey.variable_.name(),
           " was used in an ORDER BY clause, but is neither grouped nor "
-          "created as an alias in the SELECT clause." +
-          noteForImplicitGroupBy);
-          */
-      LOG(WARN)
-          << "Variable " + orderKey.variable_.name() +
-                 " was used in an ORDER BY clause, but is neither grouped nor "
-                 "created as an alias in the SELECT clause." +
-                 noteForImplicitGroupBy
-          << std::endl;
+          "created as an alias in the SELECT clause.",
+          noteForImplicitGroupBy));
     }
     _orderBy.push_back(std::move(orderKey));
   };

diff --git a/test/SparqlAntlrParserTest.cpp b/test/SparqlAntlrParserTest.cpp
@@ -1185,7 +1185,6 @@ TEST(SparqlParser, SelectQuery) {
 }
 
 TEST(SparqlParser, ConstructQuery) {
-  auto contains = [](const std::string& s) { return ::testing::HasSubstr(s); };
   auto expectConstructQuery =
       ExpectCompleteParse<&Parser::constructQuery>{defaultPrefixMap};
   auto expectConstructQueryFails = ExpectParseFails<&Parser::constructQuery>{};
@@ -1367,6 +1366,19 @@ TEST(SparqlParser, Query) {
 
   // DESCRIBE queries are not yet supported.
   expectQueryFails("DESCRIBE *");
+
+  // Test the various places where warnings are added in a query
+  expectQuery("SELECT ?x {} GROUP BY ?x ORDER BY ?y",
+              m::WarningsOfParsedQuery({"?x was used by GROUP BY",
+                                        "?y was used in an ORDER BY clause"}));
+  expectQuery("SELECT * { BIND (?a as ?b) }",
+              m::WarningsOfParsedQuery(
+                  {"?a was used in the expression of a BIND clause"}));
+  expectQuery("SELECT * { FILTER (?a < 42) }",
+              m::WarningsOfParsedQuery(
+                  {"?a was used in the expression of a FILTER clause"}));
+  expectQuery("SELECT * { } ORDER BY ?s",
+              m::WarningsOfParsedQuery({"?s was used by ORDER BY"}));
 }
 
 // Some helper matchers for the `builtInCall` test below.

diff --git a/test/SparqlAntlrParserTestHelpers.h b/test/SparqlAntlrParserTestHelpers.h
@@ -479,6 +479,14 @@ inline auto GroupByVariables =
                   testing::UnorderedElementsAreArray(vars));
 };
 
+inline auto WarningsOfParsedQuery =
+    [](const vector<std::string>& warnings) -> Matcher<const ParsedQuery&> {
+  auto matchers = ad_utility::transform(
+      warnings, [](const std::string& s) { return ::testing::HasSubstr(s); });
+  return AD_PROPERTY(ParsedQuery, warnings,
+                     testing::UnorderedElementsAreArray(matchers));
+};
+
 inline auto Values = [](const std::vector<::Variable>& vars,
                         const std::vector<vector<TripleComponent>>& values)
     -> Matcher<const p::Values&> {