GH-3782 clean up handling of GROUP BY projection element verification

core/queryparser/sparql/src/main/java/org/eclipse/rdf4j/query/parser/sparql/TupleExprBuilder.java

@@ -17,7 +17,6 @@
 import java.util.Set;
 import java.util.UUID;
 import java.util.concurrent.atomic.AtomicLong;
-import java.util.concurrent.atomic.LongAdder;
 
 import org.eclipse.rdf4j.common.annotation.InternalUseOnly;
 import org.eclipse.rdf4j.model.IRI;
@@ -633,34 +632,38 @@ public TupleExpr visit(ASTSelect node, Object data) throws VisitorException {
  }
 
  result = new Projection(result, projElemList);
-
  if (group != null) {
  for (ProjectionElem elem : projElemList.getElements()) {
  if (!elem.hasAggregateOperatorInExpression()) {
- Set<String> groupNames = group.getBindingNames();
-
+ // non-aggregate projection elem is only allowed to be a constant or a simple expression (see
+ // https://www.w3.org/TR/sparql11-query/#aggregateRestrictions)
  ExtensionElem extElem = elem.getSourceExpression();
  if (extElem != null) {
  ValueExpr expr = extElem.getExpr();
+ if (!(expr instanceof ValueConstant)) {
+ throw new VisitorException(
+ "non-aggregate expression '" + expr
+ + "' not allowed in projection when using GROUP BY.");
+ }
 
-  VarCollector collector = new VarCollector();
- expr.visit(collector);
+ } else {
+ Set<String> groupNames = group.getBindingNames();
 
- for (Var var : collector.getCollectedVars()) {
- if (!groupNames.contains(var.getName())) {
+ if (!elem.getSourceName().equals(elem.getTargetName())) {
+ // projection element is a SELECT expression using a simple var (e.g. (?a AS ?b)).
+ // Source var must be present in GROUP BY.
+ if (!groupNames.contains(elem.getSourceName())) {
  throw new VisitorException(
- "variable '" + var.getName() + "' in projection not present in GROUP BY.");
-
+ "variable '" + elem.getSourceName()
+ + "' in projection not present in GROUP BY.");
+ }
+ } else {
+ // projection element is simple var. Must be present in GROUP BY.
+ if (!groupNames.contains(elem.getTargetName())) {
+ throw new VisitorException(
+ "variable '" + elem.getTargetName()
+ + "' in projection not present in GROUP BY.");
  }
- }
- } else {
- if (!groupNames.contains(elem.getTargetName())) {
- throw new VisitorException(
- "variable '" + elem.getTargetName() + "' in projection not present in GROUP BY.");
- } else if (!groupNames.contains(elem.getSourceName())) {
- throw new VisitorException(
- "variable '" + elem.getSourceName() + "' in projection not present in GROUP BY.");
-
  }
  }
  }

core/queryparser/sparql/src/test/java/org/eclipse/rdf4j/query/parser/sparql/SPARQLParserTest.java

@@ -8,13 +8,13 @@
 package org.eclipse.rdf4j.query.parser.sparql;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
 
 import java.util.List;
 
@@ -41,9 +41,9 @@
 import org.eclipse.rdf4j.query.parser.ParsedQuery;
 import org.eclipse.rdf4j.query.parser.ParsedTupleQuery;
 import org.eclipse.rdf4j.query.parser.ParsedUpdate;
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Test;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 
 /**
  * @author jeen
@@ -55,15 +55,15 @@ public class SPARQLParserTest {
  /**
  * @throws java.lang.Exception
  */
- @Before
+ @BeforeEach
  public void setUp() throws Exception {
  parser = new SPARQLParser();
  }
 
  /**
  * @throws java.lang.Exception
  */
- @After
+ @AfterEach
  public void tearDown() throws Exception {
  parser = null;
  }
@@ -78,8 +78,7 @@ public void testSourceStringAssignment() throws Exception {
 
  ParsedQuery q = parser.parseQuery(simpleSparqlQuery, null);
 
- assertNotNull(q);
- assertEquals(simpleSparqlQuery, q.getSourceString());
+ assertThat(q.getSourceString()).isEqualTo(simpleSparqlQuery);
  }
 
  @Test
@@ -90,7 +89,7 @@ public void testInsertDataLineNumberReporting() throws Exception {
  ParsedUpdate u = parser.parseUpdate(insertDataString, null);
  fail("should have resulted in parse exception");
  } catch (MalformedQueryException e) {
- assertTrue(e.getMessage().contains("line 2,"));
+ assertThat(e.getMessage()).contains("line 2,");
  }
 
  }
@@ -103,7 +102,7 @@ public void testDeleteDataLineNumberReporting() throws Exception {
  ParsedUpdate u = parser.parseUpdate(deleteDataString, null);
  fail("should have resulted in parse exception");
  } catch (MalformedQueryException e) {
- assertTrue(e.getMessage().contains("line 2,"));
+ assertThat(e.getMessage()).contains("line 2,");
  }
  }
 
@@ -171,7 +170,7 @@ public void testParseWildcardSubselectInUpdate() throws Exception {
  List<ProjectionElem> elements = projectionElemList.getElements();
  assertNotNull(elements);
 
- assertEquals("projection should contain all three variables", 3, elements.size());
+ assertThat(elements).hasSize(3);
  }
 
  @Test
@@ -381,4 +380,85 @@ public void testWildCardPathComplexSubjectHandling() {
 
  assertThat(subClassOfObjectVar).isEqualTo(commentObjectVar);
  }
+
+ @Test
+ public void testGroupByProjectionHandling_NoAggregate() {
+ String query = "SELECT DISTINCT ?s (?o AS ?o1) \n"
+ + "WHERE {\n"
+ + " ?s ?p ?o \n"
+ + "} GROUP BY ?s ?o";
+
+ // should parse without error
+ parser.parseQuery(query, null);
+ }
+
+ @Test
+ public void testGroupByProjectionHandling_Aggregate_NonSimpleExpr() {
+ String query = "SELECT (COUNT(?s) as ?count) (?o + ?s AS ?o1) \n"
+ + "WHERE {\n"
+ + " ?s ?p ?o \n"
+ + "} GROUP BY ?o";
+
+ assertThatExceptionOfType(MalformedQueryException.class).isThrownBy(() -> parser.parseQuery(query, null))
+ .withMessageStartingWith("non-aggregate expression 'MathExpr (+)");
+
+ }
+
+ @Test
+ public void testGroupByProjectionHandling_Aggregate_Alias() {
+ String query = "SELECT (COUNT(?s) as ?count) (?o AS ?o1) \n"
+ + "WHERE {\n"
+ + " ?s ?p ?o \n"
+ + "} GROUP BY ?o";
+
+ // should parse without error
+ parser.parseQuery(query, null);
+ }
+
+ @Test
+ public void testGroupByProjectionHandling_Aggregate_Alias2() {
+ String query = "SELECT (COUNT(?s) as ?count) (?o AS ?o1) \n"
+ + "WHERE {\n"
+ + " ?s ?p ?o \n"
+ + "} GROUP BY ?p";
+
+ assertThatExceptionOfType(MalformedQueryException.class).isThrownBy(() -> parser.parseQuery(query, null))
+ .withMessageStartingWith("variable 'o' in projection not present in GROUP BY.");
+ }
+
+ @Test
+ public void testGroupByProjectionHandling_Aggregate_SimpleExpr() {
+ String query = "SELECT (COUNT(?s) as ?count) ?o \n"
+ + "WHERE {\n"
+ + " ?s ?p ?o \n"
+ + "} GROUP BY ?p";
+
+ assertThatExceptionOfType(MalformedQueryException.class).isThrownBy(() -> parser.parseQuery(query, null))
+ .withMessageStartingWith("variable 'o' in projection not present in GROUP BY.");
+
+ }
+
+ @Test
+ public void testGroupByProjectionHandling_Aggregate_SimpleExpr2() {
+ String query = "SELECT (COUNT(?s) as ?count) ?o \n"
+ + "WHERE {\n"
+ + " ?s ?p ?o \n"
+ + "} GROUP BY ?o";
+
+ // should parse without error
+ parser.parseQuery(query, null);
+
+ }
+
+ @Test
+ public void testGroupByProjectionHandling_Aggregate_Constant() {
+ String query = "SELECT (COUNT(?s) as ?count) (<foo:constant> as ?constant) \n"
+ + "WHERE {\n"
+ + " ?s ?p ?o \n"
+ + "} GROUP BY ?o";
+
+ // should parse without error
+ parser.parseQuery(query, null);
+
+ }
 }