Allow the grouped variables to be duplicated and in parentheses.

Signed-off-by: Johannes Kalmbach <[email protected]>
ad-freiburg · Dec 16, 2024 · 05f67f3 · 05f67f3
1 parent a97905e
commit 05f67f3
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 6 deletions.
diff --git a/src/parser/ParsedQuery.cpp b/src/parser/ParsedQuery.cpp
@@ -356,17 +356,27 @@ void ParsedQuery::checkUsedVariablesAreVisible(
 
 // ____________________________________________________________________________
 void ParsedQuery::addGroupByClause(std::vector<GroupKey> groupKeys) {
-  // Process groupClause
-  auto processVariable = [this](const Variable& groupKey) {
+  // Deduplicate the group by variables to support e.g. `GROUP BY ?x ?x ?x`.
+  // Note: The `GroupBy` class expects the grouped variables to be unique.
+  ad_utility::HashSet<Variable> deduplicatedGroupByVars;
+  auto processVariable = [this,
+                          &deduplicatedGroupByVars](const Variable& groupKey) {
     checkVariableIsVisible(groupKey, "GROUP BY");
-
-    _groupByVariables.push_back(groupKey);
+    if (deduplicatedGroupByVars.insert(groupKey).second) {
+      _groupByVariables.push_back(groupKey);
+    }
   };
 
   ad_utility::HashSet<Variable> variablesDefinedInGroupBy;
   auto processExpression =
-      [this, &variablesDefinedInGroupBy](
-          sparqlExpression::SparqlExpressionPimpl groupKey) {
+      [this, &variablesDefinedInGroupBy,
+       &processVariable](sparqlExpression::SparqlExpressionPimpl groupKey) {
+        // Handle the case of redundant braces around a variable, e.g. `GROUP BY
+        // (?x)`, which is parsed as an expression by the parser.
+        if (auto var = groupKey.getVariableOrNullopt(); var.has_value()) {
+          processVariable(var.value());
+          return;
+        }
         checkUsedVariablesAreVisible(groupKey, "GROUP BY",
                                      variablesDefinedInGroupBy,
                                      " or previously in the same GROUP BY");

diff --git a/test/QueryPlannerTest.cpp b/test/QueryPlannerTest.cpp
@@ -2904,3 +2904,12 @@ TEST(QueryPlanner, Describe) {
                                     "?y", "<p>", "<o>", {},
                                     ad_utility::HashSet<std::string>{"<g>"})));
 }
+
+// ____________________________________________________________________________
+TEST(QueryPlanner, GroupByRedundanteParensAndVariables) {
+  auto matcher = h::GroupBy({Variable{"?x"}}, {},
+                            h::IndexScanFromStrings("?x", "?y", "?z"));
+  h::expect("SELECT ?x { ?x ?y ?z} GROUP BY (?x)", matcher);
+  h::expect("SELECT ?x { ?x ?y ?z} GROUP BY ?x ?x", matcher);
+  h::expect("SELECT ?x { ?x ?y ?z} GROUP BY ?x ?x (?x)", matcher);
+}