From 05f67f3cba50dced3499752b94b6b51541cf549b Mon Sep 17 00:00:00 2001
From: Johannes Kalmbach <johannes.kalmbach@gmail.com>
Date: Mon, 16 Dec 2024 14:41:13 +0100
Subject: [PATCH] Allow the grouped variables to be duplicated and in
 parentheses.

Signed-off-by: Johannes Kalmbach <johannes.kalmbach@gmail.com>
---
 src/parser/ParsedQuery.cpp | 22 ++++++++++++++++------
 test/QueryPlannerTest.cpp  |  9 +++++++++
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/src/parser/ParsedQuery.cpp b/src/parser/ParsedQuery.cpp
index 6bee76da2c..6cb844071b 100644
--- 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
index 92ae4a32c0..90462f3cc3 100644
--- 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);
+}