Skip to content

Commit 00c53d3

Browse files
JervenBollemanJervenBolleman
committed
GH-5310 Correctly evaluate constant grouping constants for empty result
sets.
1 parent ddb9167 commit 00c53d3

3 files changed

Lines changed: 205 additions & 107 deletions

File tree

‎core/queryalgebra/evaluation/src/main/java/org/eclipse/rdf4j/query/algebra/evaluation/iterator/GroupIterator.java‎

Lines changed: 34 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -319,8 +319,12 @@ private List<Entry> emptySolutionSpecialCase(List<AggregatePredicateCollectorSup
319319
} else {
320320
List<AggregateCollector> collectors = makeCollectors(aggregates);
321321
List<Predicate<?>> predicates = new ArrayList<>(aggregates.size());
322-
for (int i = 0; i < aggregates.size(); i++) {
323-
predicates.add(ALWAYS_TRUE_BINDING_SET);
322+
for (var ag : aggregates) {
323+
if (ag.agg instanceof WildCardCountAggregate) {
324+
predicates.add(ALWAYS_TRUE_BINDING_SET);
325+
} else {
326+
predicates.add(ALWAYS_TRUE_VALUE);
327+
}
324328
}
325329
final Entry entry = new Entry(null, collectors, predicates);
326330
entry.addSolution(EmptyBindingSet.getInstance(), aggregates);
@@ -407,40 +411,33 @@ private void operate(BindingSet bs, Predicate<?> predicate, Object t) {
407411
return new AggregatePredicateCollectorSupplier<>(wildCardCountAggregate, potentialDistinctTest,
408412
() -> new CountCollector(vf), ge.getName());
409413
} else {
410-
QueryStepEvaluator f = new QueryStepEvaluator(
411-
strategy.precompile(((Count) operator).getArg(), context));
414+
QueryStepEvaluator f = precompileArg(operator);
412415
CountAggregate agg = new CountAggregate(f);
413-
Supplier<Predicate<Value>> predicate = operator.isDistinct() ? DistinctValues::new
414-
: ALWAYS_TRUE_VALUE_SUPPLIER;
416+
Supplier<Predicate<Value>> predicate = createDistinctValueTest(operator);
415417
return new AggregatePredicateCollectorSupplier<>(agg, predicate, () -> new CountCollector(vf),
416418
ge.getName());
417419
}
418420
} else if (operator instanceof Min) {
419421
MinAggregate agg = new MinAggregate(precompileArg(operator), shouldValueComparisonBeStrict());
420-
Supplier<Predicate<Value>> predicate = operator.isDistinct() ? DistinctValues::new
421-
: ALWAYS_TRUE_VALUE_SUPPLIER;
422+
Supplier<Predicate<Value>> predicate = createDistinctValueTest(operator);
422423
return new AggregatePredicateCollectorSupplier<>(agg, predicate, ValueCollector::new, ge.getName());
423424
} else if (operator instanceof Max) {
424425
MaxAggregate agg = new MaxAggregate(precompileArg(operator), shouldValueComparisonBeStrict());
425-
Supplier<Predicate<Value>> predicate = operator.isDistinct() ? DistinctValues::new
426-
: ALWAYS_TRUE_VALUE_SUPPLIER;
426+
Supplier<Predicate<Value>> predicate = createDistinctValueTest(operator);
427427
return new AggregatePredicateCollectorSupplier<>(agg, predicate, ValueCollector::new, ge.getName());
428428
} else if (operator instanceof Sum) {
429429

430430
SumAggregate agg = new SumAggregate(precompileArg(operator));
431-
Supplier<Predicate<Value>> predicate = operator.isDistinct() ? DistinctValues::new
432-
: ALWAYS_TRUE_VALUE_SUPPLIER;
431+
Supplier<Predicate<Value>> predicate = createDistinctValueTest(operator);
433432
return new AggregatePredicateCollectorSupplier<>(agg, predicate, () -> new IntegerCollector(vf),
434433
ge.getName());
435434
} else if (operator instanceof Avg) {
436435
AvgAggregate agg = new AvgAggregate(precompileArg(operator));
437-
Supplier<Predicate<Value>> predicate = operator.isDistinct() ? DistinctValues::new
438-
: ALWAYS_TRUE_VALUE_SUPPLIER;
436+
Supplier<Predicate<Value>> predicate = createDistinctValueTest(operator);
439437
return new AggregatePredicateCollectorSupplier<>(agg, predicate, () -> new AvgCollector(vf), ge.getName());
440438
} else if (operator instanceof Sample) {
441439
SampleAggregate agg = new SampleAggregate(precompileArg(operator));
442-
Supplier<Predicate<Value>> predicate = operator.isDistinct() ? DistinctValues::new
443-
: ALWAYS_TRUE_VALUE_SUPPLIER;
440+
Supplier<Predicate<Value>> predicate = createDistinctValueTest(operator);
444441
return new AggregatePredicateCollectorSupplier<>(agg, predicate, SampleCollector::new, ge.getName());
445442
} else if (operator instanceof GroupConcat) {
446443
ValueExpr separatorExpr = ((GroupConcat) operator).getSeparator();
@@ -451,19 +448,17 @@ private void operate(BindingSet bs, Predicate<?> predicate, Object t) {
451448
} else {
452449
agg = new ConcatAggregate(precompileArg(operator));
453450
}
454-
Supplier<Predicate<Value>> predicate = operator.isDistinct() ? DistinctValues::new
455-
: ALWAYS_TRUE_VALUE_SUPPLIER;
451+
Supplier<Predicate<Value>> predicate = createDistinctValueTest(operator);
456452
return new AggregatePredicateCollectorSupplier<>(agg, predicate, () -> new StringBuilderCollector(vf),
457453
ge.getName());
458454
} else if (operator instanceof AggregateFunctionCall) {
459455
var aggOperator = (AggregateFunctionCall) operator;
460-
Supplier<Predicate<Value>> predicate = operator.isDistinct() ? DistinctValues::new
461-
: ALWAYS_TRUE_VALUE_SUPPLIER;
456+
Supplier<Predicate<Value>> predicate = createDistinctValueTest(operator);
462457
var factory = CustomAggregateFunctionRegistry.getInstance().get(aggOperator.getIRI());
463458

464459
var function = factory.orElseThrow(
465460
() -> new QueryEvaluationException("Unknown aggregate function '" + aggOperator.getIRI() + "'"))
466-
.buildFunction(new QueryStepEvaluator(strategy.precompile(aggOperator.getArg(), context)));
461+
.buildFunction(precompileArg(aggOperator));
467462
return new AggregatePredicateCollectorSupplier<>(function, predicate, () -> factory.get().getCollector(),
468463
ge.getName());
469464

@@ -472,8 +467,21 @@ private void operate(BindingSet bs, Predicate<?> predicate, Object t) {
472467
return null;
473468
}
474469

470+
/**
471+
* Create a predicate that tests if the value is distinct (returning true if the value was not seen yet), or always
472+
* returns true if the operator is not distinct.
473+
*
474+
* @param operator
475+
* @return a supplier that returns a predicate that tests if the value is distinct, or always returns true if the
476+
* operator is not distinct.
477+
*/
478+
private Supplier<Predicate<Value>> createDistinctValueTest(AggregateOperator operator) {
479+
return operator.isDistinct() ? DistinctValues::new : ALWAYS_TRUE_VALUE_SUPPLIER;
480+
}
481+
475482
private QueryStepEvaluator precompileArg(AggregateOperator operator) {
476-
return new QueryStepEvaluator(strategy.precompile(((UnaryValueOperator) operator).getArg(), context));
483+
ValueExpr ve = ((UnaryValueOperator) operator).getArg();
484+
return new QueryStepEvaluator(strategy.precompile(ve, context));
477485
}
478486

479487
private boolean shouldValueComparisonBeStrict() {
@@ -491,7 +499,7 @@ public CountCollector(ValueFactory vf) {
491499

492500
@Override
493501
public Value getFinalValue() {
494-
return SimpleValueFactory.getInstance().createLiteral(Long.toString(value), CoreDatatype.XSD.INTEGER);
502+
return vf.createLiteral(Long.toString(value), CoreDatatype.XSD.INTEGER);
495503
}
496504
}
497505

@@ -561,7 +569,7 @@ public Value getFinalValue() throws ValueExprEvaluationException {
561569
}
562570

563571
if (count == 0) {
564-
return SimpleValueFactory.getInstance().createLiteral("0", CoreDatatype.XSD.INTEGER);
572+
return vf.createLiteral("0", CoreDatatype.XSD.INTEGER);
565573
}
566574

567575
Literal sizeLit = SimpleValueFactory.getInstance().createLiteral(count);
@@ -791,9 +799,9 @@ public StringBuilderCollector(ValueFactory vf) {
791799
@Override
792800
public Value getFinalValue() throws ValueExprEvaluationException {
793801
if (concatenated == null || concatenated.length() == 0) {
794-
return SimpleValueFactory.getInstance().createLiteral("");
802+
return vf.createLiteral("");
795803
}
796-
return SimpleValueFactory.getInstance().createLiteral(concatenated.toString());
804+
return vf.createLiteral(concatenated.toString());
797805
}
798806
}
799807

‎core/queryalgebra/evaluation/src/test/java/org/eclipse/rdf4j/query/algebra/evaluation/impl/ConstantOptimizerTest.java‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,18 +11,21 @@
1111
package org.eclipse.rdf4j.query.algebra.evaluation.impl;
1212

1313
import static org.assertj.core.api.Assertions.assertThat;
14+
import static org.junit.jupiter.api.Assertions.assertEquals;
1415
import static org.junit.jupiter.api.Assertions.assertNotNull;
1516
import static org.junit.jupiter.api.Assertions.assertTrue;
1617
import static org.mockito.Mockito.mock;
1718

1819
import org.eclipse.rdf4j.common.exception.RDF4JException;
1920
import org.eclipse.rdf4j.common.iteration.CloseableIteration;
21+
import org.eclipse.rdf4j.model.Literal;
2022
import org.eclipse.rdf4j.model.impl.BooleanLiteral;
2123
import org.eclipse.rdf4j.model.impl.SimpleValueFactory;
2224
import org.eclipse.rdf4j.query.BindingSet;
2325
import org.eclipse.rdf4j.query.QueryLanguage;
2426
import org.eclipse.rdf4j.query.algebra.And;
2527
import org.eclipse.rdf4j.query.algebra.FunctionCall;
28+
import org.eclipse.rdf4j.query.algebra.GroupElem;
2629
import org.eclipse.rdf4j.query.algebra.QueryRoot;
2730
import org.eclipse.rdf4j.query.algebra.TupleExpr;
2831
import org.eclipse.rdf4j.query.algebra.evaluation.EvaluationStrategy;
@@ -112,12 +115,49 @@ public void testFunctionOptimization() throws RDF4JException {
112115

113116
}
114117

118+
@Test
119+
public void testAggregateOptimization() throws RDF4JException {
120+
String query = "prefix ex: <ex:>" + "select (max(1) AS ?a) \n " + "where {\n" + "?x a ?z \n"
121+
+ "}";
122+
123+
ParsedQuery pq = QueryParserUtil.parseQuery(QueryLanguage.SPARQL, query, null);
124+
EvaluationStrategy strategy = new DefaultEvaluationStrategy(new EmptyTripleSource(), null);
125+
TupleExpr original = pq.getTupleExpr();
126+
127+
final AlgebraFinder finder = new AlgebraFinder();
128+
original.visit(finder);
129+
assertTrue(finder.groupElemFound);
130+
131+
// reset for re-use on optimized query
132+
finder.reset();
133+
134+
QueryBindingSet constants = new QueryBindingSet();
135+
constants.addBinding("x", SimpleValueFactory.getInstance().createLiteral("foo"));
136+
constants.addBinding("z", SimpleValueFactory.getInstance().createLiteral("bar"));
137+
138+
TupleExpr optimized = optimize(pq.getTupleExpr().clone(), constants, strategy);
139+
140+
optimized.visit(finder);
141+
assertThat(finder.functionCallFound).isFalse();
142+
143+
CloseableIteration<BindingSet> result = strategy.precompile(optimized)
144+
.evaluate(
145+
new EmptyBindingSet());
146+
assertNotNull(result);
147+
assertTrue(result.hasNext());
148+
BindingSet bindings = result.next();
149+
assertTrue(bindings.hasBinding("a"));
150+
assertEquals(1, ((Literal) bindings.getBinding("a").getValue()).intValue());
151+
}
152+
115153
private class AlgebraFinder extends AbstractQueryModelVisitor<RuntimeException> {
116154

117155
public boolean logicalAndfound = false;
118156

119157
public boolean functionCallFound = false;
120158

159+
public boolean groupElemFound = false;
160+
121161
@Override
122162
public void meet(And and) {
123163
logicalAndfound = true;
@@ -134,6 +174,12 @@ public void meet(FunctionCall arg) {
134174
public void reset() {
135175
logicalAndfound = false;
136176
functionCallFound = false;
177+
groupElemFound = false;
178+
}
179+
180+
public void meet(GroupElem ge) {
181+
groupElemFound = true;
182+
super.meet(ge);
137183
}
138184
}
139185

0 commit comments

Comments
 (0)