Skip to content

Commit 0ae7fc6

Browse files
hmottestadhmottestad
authored
GH-4992 fix for parent (#5496)
Add regression tests for aggregate parent references
2 parents ce71ddc + 439b12b commit 0ae7fc6

2 files changed

Lines changed: 202 additions & 4 deletions

File tree

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

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -449,7 +449,7 @@ private TupleExpr processHavingClause(ASTHavingClause havingNode, TupleExpr tupl
449449

450450
// create an extension linking the operator to the variable
451451
// name.
452-
ExtensionElem pe = new ExtensionElem(operator, alias);
452+
ExtensionElem pe = new ExtensionElem(operator.clone(), alias);
453453
extension.addElement(pe);
454454

455455
// add the aggregate operator to the group.
@@ -494,7 +494,7 @@ private TupleExpr processOrderClause(ASTOrderClause orderNode, TupleExpr tupleEx
494494
// name.
495495
String alias = var.getName();
496496

497-
ExtensionElem pe = new ExtensionElem(operator, alias);
497+
ExtensionElem pe = new ExtensionElem(operator.clone(), alias);
498498
extension.addElement(pe);
499499

500500
// add the aggregate operator to the group.
@@ -576,7 +576,7 @@ public TupleExpr visit(ASTSelect node, Object data) throws VisitorException {
576576
Extension anonymousExtension = new Extension();
577577
Var anonVar = createAnonVar();
578578
expr.replaceChildNode(operator, anonVar);
579-
anonymousExtension.addElement(new ExtensionElem(operator, anonVar.getName()));
579+
anonymousExtension.addElement(new ExtensionElem(operator.clone(), anonVar.getName()));
580580

581581
anonymousExtension.setArg(result);
582582
result = anonymousExtension;
@@ -593,7 +593,7 @@ public TupleExpr visit(ASTSelect node, Object data) throws VisitorException {
593593
// SELECT expressions need to be captured as an extension, so that original and alias are
594594
// available for the ORDER BY clause (which gets applied _before_ projection). See GH-4066
595595
// and https://www.w3.org/TR/sparql11-query/#sparqlSolMod .
596-
ExtensionElem extElem = new ExtensionElem(valueExpr, alias);
596+
ExtensionElem extElem = new ExtensionElem(cloneIfAggregate(valueExpr), alias);
597597
extension.addElement(extElem);
598598
elem.setSourceExpression(extElem);
599599
} else if (child instanceof ASTVar) {
@@ -663,6 +663,13 @@ public TupleExpr visit(ASTSelect node, Object data) throws VisitorException {
663663
return result;
664664
}
665665

666+
private ValueExpr cloneIfAggregate(ValueExpr valueExpr) {
667+
if (valueExpr instanceof AggregateOperator) {
668+
return ((AggregateOperator) valueExpr).clone();
669+
}
670+
return valueExpr;
671+
}
672+
666673
private static boolean isIllegalCombinedWithGroupByExpression(ValueExpr expr, List<ProjectionElem> elements,
667674
Set<String> groupNames) {
668675
if (expr instanceof ValueConstant) {

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

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,27 @@
1717
import static org.junit.jupiter.api.Assertions.fail;
1818

1919
import java.util.ArrayList;
20+
import java.util.IdentityHashMap;
2021
import java.util.List;
22+
import java.util.Map;
2123

2224
import org.eclipse.rdf4j.model.impl.SimpleValueFactory;
25+
import org.eclipse.rdf4j.query.algebra.AggregateOperator;
26+
import org.eclipse.rdf4j.query.algebra.Count;
2327
import org.eclipse.rdf4j.query.algebra.Extension;
28+
import org.eclipse.rdf4j.query.algebra.ExtensionElem;
29+
import org.eclipse.rdf4j.query.algebra.Filter;
30+
import org.eclipse.rdf4j.query.algebra.Group;
31+
import org.eclipse.rdf4j.query.algebra.GroupElem;
2432
import org.eclipse.rdf4j.query.algebra.Order;
2533
import org.eclipse.rdf4j.query.algebra.Projection;
2634
import org.eclipse.rdf4j.query.algebra.Service;
2735
import org.eclipse.rdf4j.query.algebra.SingletonSet;
2836
import org.eclipse.rdf4j.query.algebra.Slice;
2937
import org.eclipse.rdf4j.query.algebra.TupleExpr;
38+
import org.eclipse.rdf4j.query.algebra.ValueExpr;
3039
import org.eclipse.rdf4j.query.algebra.Var;
40+
import org.eclipse.rdf4j.query.algebra.helpers.AbstractQueryModelVisitor;
3141
import org.eclipse.rdf4j.query.parser.sparql.ast.ASTQueryContainer;
3242
import org.eclipse.rdf4j.query.parser.sparql.ast.ASTServiceGraphPattern;
3343
import org.eclipse.rdf4j.query.parser.sparql.ast.ASTUpdateSequence;
@@ -74,6 +84,113 @@ public void testSimpleAliasHandling() {
7484
}
7585
}
7686

87+
@Test
88+
public void testAggregateProjectionParentReferences() throws Exception {
89+
String query = "SELECT (COUNT(?s) AS ?count) WHERE { ?s ?p ?o }";
90+
91+
ASTQueryContainer qc = SyntaxTreeBuilder.parseQuery(query);
92+
TupleExpr tupleExpr = builder.visit(qc, null);
93+
94+
assertThat(tupleExpr).isInstanceOf(Projection.class);
95+
Projection projection = (Projection) tupleExpr;
96+
97+
assertThat(projection.getArg()).isInstanceOf(Extension.class);
98+
Extension extension = (Extension) projection.getArg();
99+
100+
assertThat(extension.getArg()).isInstanceOf(Group.class);
101+
Group group = (Group) extension.getArg();
102+
103+
assertThat(group.getGroupElements()).hasSize(1);
104+
GroupElem groupElem = group.getGroupElements().get(0);
105+
AggregateOperator operator = groupElem.getOperator();
106+
107+
assertThat(operator).isInstanceOf(Count.class);
108+
assertThat(operator.getParentNode()).isSameAs(groupElem);
109+
110+
assertThat(extension.getElements()).hasSize(1);
111+
ExtensionElem extensionElem = extension.getElements().get(0);
112+
ValueExpr extExpr = extensionElem.getExpr();
113+
114+
assertThat(extExpr).isInstanceOf(Count.class);
115+
assertThat(extExpr.getParentNode()).isSameAs(extensionElem);
116+
assertThat(extExpr).isNotSameAs(operator);
117+
}
118+
119+
@Test
120+
public void testAggregateOrderByParentReferences() throws Exception {
121+
String query = "SELECT (COUNT(?s) AS ?count) WHERE { ?s ?p ?o } ORDER BY (COUNT(?s))";
122+
123+
ASTQueryContainer qc = SyntaxTreeBuilder.parseQuery(query);
124+
TupleExpr tupleExpr = builder.visit(qc, null);
125+
126+
AggregateOperatorContext context = collectAggregateOperators(tupleExpr);
127+
128+
assertThat(context.groupOperators).isNotEmpty();
129+
assertThat(context.extensionOperators).hasSizeGreaterThanOrEqualTo(2);
130+
131+
for (AggregateOperator operator : context.groupOperators) {
132+
assertThat(operator.getParentNode()).isInstanceOf(GroupElem.class);
133+
}
134+
135+
for (AggregateOperator operator : context.extensionOperators) {
136+
assertThat(operator.getParentNode()).isInstanceOf(ExtensionElem.class);
137+
assertThat(context.containsSameInstanceInGroup(operator)).isFalse();
138+
}
139+
}
140+
141+
@Test
142+
public void testAggregateHavingParentReferences() throws Exception {
143+
String query = "SELECT (COUNT(?s) AS ?count) WHERE { ?s ?p ?o } HAVING (COUNT(?s) > 1)";
144+
145+
ASTQueryContainer qc = SyntaxTreeBuilder.parseQuery(query);
146+
TupleExpr tupleExpr = builder.visit(qc, null);
147+
148+
AggregateOperatorContext context = collectAggregateOperators(tupleExpr);
149+
150+
assertThat(context.groupOperators).isNotEmpty();
151+
assertThat(context.extensionOperators).hasSizeGreaterThanOrEqualTo(2);
152+
153+
for (AggregateOperator operator : context.groupOperators) {
154+
assertThat(operator.getParentNode()).isInstanceOf(GroupElem.class);
155+
}
156+
157+
for (AggregateOperator operator : context.extensionOperators) {
158+
assertThat(operator.getParentNode()).isInstanceOf(ExtensionElem.class);
159+
assertThat(context.containsSameInstanceInGroup(operator)).isFalse();
160+
}
161+
162+
Filter filter = findNode(tupleExpr, Filter.class);
163+
assertThat(filter).isNotNull();
164+
}
165+
166+
@Test
167+
public void testAggregateGroupConditionParentReferences() throws Exception {
168+
String query = "SELECT (COUNT(?s) AS ?count) WHERE { ?s ?p ?o } GROUP BY (COUNT(?s) AS ?groupCount)";
169+
170+
ASTQueryContainer qc = SyntaxTreeBuilder.parseQuery(query);
171+
TupleExpr tupleExpr = builder.visit(qc, null);
172+
173+
AggregateOperatorContext context = collectAggregateOperators(tupleExpr);
174+
assertThat(context.groupOperators).isNotEmpty();
175+
assertThat(context.extensionOperators).isNotEmpty();
176+
177+
for (AggregateOperator operator : context.groupOperators) {
178+
assertThat(operator.getParentNode()).isInstanceOf(GroupElem.class);
179+
}
180+
181+
for (AggregateOperator operator : context.extensionOperators) {
182+
assertThat(operator.getParentNode()).isInstanceOf(ExtensionElem.class);
183+
assertThat(context.containsSameInstanceInGroup(operator)).isFalse();
184+
}
185+
186+
ExtensionElem groupAliasExtension = findExtensionElem(tupleExpr, "groupCount");
187+
assertThat(groupAliasExtension).isNotNull();
188+
assertThat(groupAliasExtension.getExpr()).isInstanceOf(AggregateOperator.class);
189+
AggregateOperator groupAliasOperator = (AggregateOperator) groupAliasExtension.getExpr();
190+
assertThat(groupAliasOperator.getParentNode()).isSameAs(groupAliasExtension);
191+
assertThat(context.containsSameInstanceInGroup(groupAliasOperator)).isFalse();
192+
}
193+
77194
@Test
78195
public void testBindVarReuseHandling() {
79196
String query = "SELECT * WHERE { ?s ?p ?o. BIND(<foo:bar> as ?o) }";
@@ -315,4 +432,78 @@ public List<String> getGraphPatterns() {
315432
return graphPatterns;
316433
}
317434
}
435+
436+
private AggregateOperatorContext collectAggregateOperators(TupleExpr tupleExpr) {
437+
AggregateOperatorContext context = new AggregateOperatorContext();
438+
tupleExpr.visit(new AbstractQueryModelVisitor<RuntimeException>() {
439+
@Override
440+
public void meet(GroupElem node) {
441+
AggregateOperator operator = node.getOperator();
442+
context.groupOperators.add(operator);
443+
context.groupIdentities.put(operator, Boolean.TRUE);
444+
super.meet(node);
445+
}
446+
447+
@Override
448+
public void meet(ExtensionElem node) {
449+
ValueExpr expr = node.getExpr();
450+
if (expr instanceof AggregateOperator) {
451+
context.extensionOperators.add((AggregateOperator) expr);
452+
}
453+
super.meet(node);
454+
}
455+
});
456+
return context;
457+
}
458+
459+
private <T> T findNode(TupleExpr tupleExpr, Class<T> type) {
460+
class Finder extends AbstractQueryModelVisitor<RuntimeException> {
461+
private T result;
462+
463+
@Override
464+
protected void meetNode(org.eclipse.rdf4j.query.algebra.QueryModelNode node) {
465+
if (result != null) {
466+
return;
467+
}
468+
if (type.isInstance(node)) {
469+
result = type.cast(node);
470+
} else {
471+
super.meetNode(node);
472+
}
473+
}
474+
}
475+
476+
Finder finder = new Finder();
477+
tupleExpr.visit(finder);
478+
return finder.result;
479+
}
480+
481+
private ExtensionElem findExtensionElem(TupleExpr tupleExpr, String name) {
482+
class Finder extends AbstractQueryModelVisitor<RuntimeException> {
483+
private ExtensionElem result;
484+
485+
@Override
486+
public void meet(ExtensionElem node) {
487+
if (result == null && name.equals(node.getName())) {
488+
result = node;
489+
return;
490+
}
491+
super.meet(node);
492+
}
493+
}
494+
495+
Finder finder = new Finder();
496+
tupleExpr.visit(finder);
497+
return finder.result;
498+
}
499+
500+
private static final class AggregateOperatorContext {
501+
private final List<AggregateOperator> groupOperators = new ArrayList<>();
502+
private final Map<AggregateOperator, Boolean> groupIdentities = new IdentityHashMap<>();
503+
private final List<AggregateOperator> extensionOperators = new ArrayList<>();
504+
505+
boolean containsSameInstanceInGroup(AggregateOperator operator) {
506+
return groupIdentities.containsKey(operator);
507+
}
508+
}
318509
}

0 commit comments

Comments
 (0)