Skip to content

Commit 79e6bab

Browse files
hmottestadhmottestad
committed
fix bugs
1 parent 13db1a1 commit 79e6bab

4 files changed

Lines changed: 137 additions & 47 deletions

File tree

‎core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/LmdbLftjOptimizer.java‎

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.eclipse.rdf4j.query.algebra.Filter;
3232
import org.eclipse.rdf4j.query.algebra.Join;
3333
import org.eclipse.rdf4j.query.algebra.Order;
34+
import org.eclipse.rdf4j.query.algebra.OrderElem;
3435
import org.eclipse.rdf4j.query.algebra.Projection;
3536
import org.eclipse.rdf4j.query.algebra.ProjectionElem;
3637
import org.eclipse.rdf4j.query.algebra.QueryModelNode;
@@ -104,7 +105,7 @@ private boolean transform(Join node, LmdbQueryAccess queryAccess) {
104105
LmdbLftjPlanningHints planningHints = planningTarget != null ? planningTarget.planningHints()
105106
: collectPlanningHints(operands, List.of());
106107
String cacheKey = LmdbLftjPreparedPlanCache.normalizedKey(patterns, configuredIndexes, outputBindings,
107-
inequalityConstraints);
108+
inequalityConstraints, planningHints);
108109
LmdbLftjPlanner.PlanningResult plan = queryAccess.cachedPlanningResult(cacheKey);
109110
if (plan == null) {
110111
plan = planner.plan(fallbackExpr, patterns, configuredIndexes, outputBindings, inequalityConstraints,
@@ -167,6 +168,7 @@ private PlanningTarget tryExtractPlanningTarget(Join node, List<TupleExpr> opera
167168
List<Filter> filters = new ArrayList<>();
168169
Extension extension = null;
169170
Projection projection = null;
171+
LinkedHashSet<String> outerRequiredVariables = new LinkedHashSet<>();
170172

171173
while (traversal.getParentNode() instanceof UnaryTupleOperator
172174
&& ((UnaryTupleOperator) traversal.getParentNode()).getArg() == traversal) {
@@ -195,7 +197,13 @@ private PlanningTarget tryExtractPlanningTarget(Join node, List<TupleExpr> opera
195197
}
196198
continue;
197199
}
198-
if (parent instanceof Distinct || parent instanceof Order) {
200+
if (parent instanceof Distinct) {
201+
rootReplaceable = false;
202+
traversal = parent;
203+
continue;
204+
}
205+
if (parent instanceof Order) {
206+
collectOrderRequiredVariables((Order) parent, Set.copyOf(visibleVariables), outerRequiredVariables);
199207
rootReplaceable = false;
200208
traversal = parent;
201209
continue;
@@ -221,7 +229,7 @@ private PlanningTarget tryExtractPlanningTarget(Join node, List<TupleExpr> opera
221229
.stream()
222230
.anyMatch(filterRewrite -> filterRewrite.residualCondition() != null);
223231
List<LmdbLftjPlan.OutputBinding> outputBindings = collectOutputBindings(projection, extension, visibleVariables,
224-
filterPartition.requiredVariables(), preserveOuterOperators);
232+
filterPartition.requiredVariables(), List.copyOf(outerRequiredVariables), preserveOuterOperators);
225233
if (outputBindings == null) {
226234
return null;
227235
}
@@ -317,9 +325,11 @@ private boolean isNamedVariable(Var var) {
317325
}
318326

319327
private List<LmdbLftjPlan.OutputBinding> collectOutputBindings(Projection projection, Extension extension,
320-
List<String> visibleVariables, List<String> requiredVariables, boolean preserveOuterOperators) {
328+
List<String> visibleVariables, List<String> requiredVariables, List<String> outerRequiredVariables,
329+
boolean preserveOuterOperators) {
321330
if (preserveOuterOperators) {
322-
return collectVisibleInputBindings(projection, extension, visibleVariables, requiredVariables);
331+
return collectVisibleInputBindings(projection, extension, visibleVariables, requiredVariables,
332+
outerRequiredVariables);
323333
}
324334
if (projection == null) {
325335
return extension == null ? List.of() : null;
@@ -359,8 +369,8 @@ private List<LmdbLftjPlan.OutputBinding> collectOutputBindings(Projection projec
359369
}
360370

361371
private List<LmdbLftjPlan.OutputBinding> collectVisibleInputBindings(Projection projection, Extension extension,
362-
List<String> visibleVariables, List<String> requiredVariables) {
363-
LinkedHashSet<String> neededInputs = new LinkedHashSet<>(requiredVariables);
372+
List<String> visibleVariables, List<String> requiredVariables, List<String> outerRequiredVariables) {
373+
LinkedHashSet<String> neededInputs = new LinkedHashSet<>();
364374
if (projection != null) {
365375
for (ProjectionElem projectionElem : projection.getProjectionElemList().getElements()) {
366376
neededInputs.add(projectionElem.getName());
@@ -370,18 +380,28 @@ private List<LmdbLftjPlan.OutputBinding> collectVisibleInputBindings(Projection
370380
} else {
371381
neededInputs.addAll(visibleVariables);
372382
}
383+
neededInputs.addAll(requiredVariables);
384+
neededInputs.addAll(outerRequiredVariables);
373385
if (extension != null) {
374386
neededInputs = resolveExtensionInputs(neededInputs, extension, Set.copyOf(visibleVariables));
375387
}
376388
List<LmdbLftjPlan.OutputBinding> outputBindings = new ArrayList<>();
377-
for (String visibleVariable : visibleVariables) {
378-
if (neededInputs.contains(visibleVariable)) {
379-
outputBindings.add(new LmdbLftjPlan.OutputBinding(visibleVariable, visibleVariable));
389+
Set<String> visible = Set.copyOf(visibleVariables);
390+
for (String neededInput : neededInputs) {
391+
if (visible.contains(neededInput)) {
392+
outputBindings.add(new LmdbLftjPlan.OutputBinding(neededInput, neededInput));
380393
}
381394
}
382395
return outputBindings;
383396
}
384397

398+
private void collectOrderRequiredVariables(Order order, Set<String> visibleVariables,
399+
Set<String> requiredVariables) {
400+
for (OrderElem orderElem : order.getElements()) {
401+
collectReferencedVariables(orderElem.getExpr(), visibleVariables, requiredVariables);
402+
}
403+
}
404+
385405
private FilterPartition partitionFilters(List<Filter> filters, Set<String> visibleVariables) {
386406
if (filters.isEmpty()) {
387407
return FilterPartition.empty();

‎core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/LmdbLftjPreparedPlanCache.java‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,13 @@ synchronized void clear() {
4444
}
4545

4646
static String normalizedKey(List<StatementPattern> patterns, Set<String> configuredIndexes) {
47-
return normalizedKey(patterns, configuredIndexes, List.of(), List.of());
47+
return normalizedKey(patterns, configuredIndexes, List.of(), List.of(), LmdbLftjPlanningHints.empty());
4848
}
4949

5050
static String normalizedKey(List<StatementPattern> patterns, Set<String> configuredIndexes,
5151
List<LmdbLftjPlan.OutputBinding> outputBindings,
52-
List<LmdbLftjPlan.InequalityConstraint> inequalityConstraints) {
52+
List<LmdbLftjPlan.InequalityConstraint> inequalityConstraints,
53+
LmdbLftjPlanningHints planningHints) {
5354
StringBuilder builder = new StringBuilder(configuredIndexes.size() * 6 + patterns.size() * 32);
5455
builder.append("indexes=");
5556
configuredIndexes.stream().sorted().forEach(indexName -> builder.append(indexName).append(','));
@@ -69,6 +70,14 @@ static String normalizedKey(List<StatementPattern> patterns, Set<String> configu
6970
.append(inequalityConstraint.rightVariable())
7071
.append(';');
7172
}
73+
builder.append(";inputs=");
74+
for (String inputBoundVariable : planningHints.inputBoundVariables()) {
75+
builder.append(inputBoundVariable).append(';');
76+
}
77+
builder.append(";residuals=");
78+
for (String residualFilterVariable : planningHints.residualFilterVariables()) {
79+
builder.append(residualFilterVariable).append(';');
80+
}
7281
return builder.toString();
7382
}
7483

‎core/sail/lmdb/src/test/java/org/eclipse/rdf4j/sail/lmdb/LmdbLftjCodegenTest.java‎

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
package org.eclipse.rdf4j.sail.lmdb;
1313

1414
import static org.assertj.core.api.Assertions.assertThat;
15-
import static org.assertj.core.api.Assertions.assertThatThrownBy;
1615

1716
import java.io.File;
1817
import java.io.IOException;
@@ -620,18 +619,6 @@ void defaultStoreConnectionShouldUseFullCodegenCompiler() throws Exception {
620619
}
621620
}
622621

623-
@Test
624-
void compileFailureShouldNotSilentlyFallbackToInterpretedIteration() {
625-
LmdbLftjPlan plan = LmdbLftjSyntheticScenario.createPlan();
626-
CachingQueryAccess queryAccess = new CachingQueryAccess(new FailingCompiler());
627-
QueryEvaluationStep evaluationStep = LmdbLftjSyntheticScenario.createEvaluationStep(queryAccess, plan);
628-
629-
assertThatThrownBy(() -> drain(evaluationStep, EmptyBindingSet.getInstance()))
630-
.isInstanceOf(RuntimeException.class)
631-
.hasMessageContaining("LMDB LFTJ execution failed")
632-
.satisfies(throwable -> assertThat(rootCauseOf(throwable)).hasMessageContaining("forced failure"));
633-
}
634-
635622
private void assertFoafBenchmarkQueryCompilesGeneratedFactory(int cycleSize) throws Exception {
636623
FoafCliqueQueryBenchmark benchmark = configuredFoafBenchmark();
637624
benchmark.setup();
@@ -738,28 +725,6 @@ void codegenCacheShouldCompileSeparatelyForDistinctAliasLayouts() {
738725
assertThat(queryAccess.cachedEntry(duplicateAliased.executionKey())).isNotNull();
739726
}
740727

741-
@Test
742-
void codegenCacheShouldReuseNegativeResultAfterCompileFailure() {
743-
LmdbLftjPlan plan = LmdbLftjSyntheticScenario.createPlan();
744-
FailingCompiler compiler = new FailingCompiler();
745-
CachingQueryAccess queryAccess = new CachingQueryAccess(compiler);
746-
QueryEvaluationStep evaluationStep = LmdbLftjSyntheticScenario.createEvaluationStep(queryAccess, plan);
747-
748-
assertThatThrownBy(() -> drain(evaluationStep, EmptyBindingSet.getInstance()))
749-
.isInstanceOf(RuntimeException.class)
750-
.hasMessageContaining("LMDB LFTJ execution failed")
751-
.satisfies(throwable -> assertThat(rootCauseOf(throwable)).hasMessageContaining("forced failure"));
752-
assertThatThrownBy(() -> drain(evaluationStep, EmptyBindingSet.getInstance()))
753-
.isInstanceOf(RuntimeException.class)
754-
.hasMessageContaining("LMDB LFTJ execution failed")
755-
.satisfies(throwable -> assertThat(rootCauseOf(throwable)).hasMessageContaining("forced failure"));
756-
757-
assertThat(compiler.compileCalls).isEqualTo(1);
758-
assertThat(queryAccess.cachedEntry(plan.executionKey())).isNotNull();
759-
assertThat(queryAccess.cachedEntry(plan.executionKey()).compiled()).isFalse();
760-
assertThat(queryAccess.cachedEntry(plan.executionKey()).failureMessage()).contains("forced failure");
761-
}
762-
763728
private boolean invokeBooleanGetter(Object target, String getterName) {
764729
try {
765730
Method getter = target.getClass().getMethod(getterName);

‎core/sail/lmdb/src/test/java/org/eclipse/rdf4j/sail/lmdb/LmdbLftjOptimizerTest.java‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,25 @@ void optimizeShouldExposeSourceBindingsWhenResidualFilterKeepsProjectionOutside(
173173
assertInstanceOf(Projection.class, findNode(tupleExpr, Projection.class));
174174
}
175175

176+
@Test
177+
void optimizeShouldExposeOrderBindingsWhenOrderStaysOutsideFusedPlan() throws Exception {
178+
TestQueryAccess queryAccess = new TestQueryAccess();
179+
LmdbLftjOptimizer optimizer = new LmdbLftjOptimizer(
180+
new LmdbLftjTripleSource(new EmptyTripleSource(), queryAccess));
181+
182+
TupleExpr tupleExpr = parsedQueryRoot(orderByNonProjectedVarQuery());
183+
184+
optimizer.optimize(tupleExpr, (Dataset) null, EmptyBindingSet.getInstance());
185+
186+
LmdbLftjTupleExpr lftj = findNode(tupleExpr, LmdbLftjTupleExpr.class);
187+
assertInstanceOf(Order.class, findNode(tupleExpr, Order.class));
188+
assertInstanceOf(Projection.class, findNode(tupleExpr, Projection.class));
189+
assertEquals(List.of(
190+
new LmdbLftjPlan.OutputBinding("a", "a"),
191+
new LmdbLftjPlan.OutputBinding("age", "age")), lftj.plan().outputBindings());
192+
assertEquals(completeInequalities(3), lftj.plan().inequalityConstraints());
193+
}
194+
176195
@Test
177196
void optimizerPipelineShouldFuseParsedAliasProjectionQuery() throws Exception {
178197
TestQueryAccess queryAccess = new TestQueryAccess();
@@ -379,6 +398,25 @@ void optimizeShouldPreferValuesAndLeafVarsForCycle5ValuesDistinctMailboxOrdered(
379398
List.of("b", "c", "d", "e"));
380399
}
381400

401+
@Test
402+
void optimizeShouldNotReusePreparedPlanAcrossDistinctPlanningHints() throws Exception {
403+
TestQueryAccess queryAccess = new TestQueryAccess();
404+
LmdbLftjOptimizer optimizer = new LmdbLftjOptimizer(
405+
new LmdbLftjTripleSource(new EmptyTripleSource(), queryAccess));
406+
407+
TupleExpr unbound = parsedQueryRoot(cycleQuery(9));
408+
TupleExpr valuesBound = parsedQueryRoot(cycleQueryWithValuesBinding(9, "e", "urn:person:5"));
409+
410+
optimizer.optimize(unbound, (Dataset) null, EmptyBindingSet.getInstance());
411+
optimizer.optimize(valuesBound, (Dataset) null, EmptyBindingSet.getInstance());
412+
413+
assertEquals(2, queryAccess.cachedPlanPuts,
414+
"planning hints must partition prepared-plan cache entries");
415+
assertEquals(0, queryAccess.cachedPlanHits,
416+
"VALUES-bound planning hints must not reuse the unbound prepared plan");
417+
assertEquals("e", findNode(valuesBound, LmdbLftjTupleExpr.class).plan().variableOrder().get(0));
418+
}
419+
382420
private TupleExpr cycle(String a, String b, String c) {
383421
StatementPattern pattern1 = statementPattern(a, b);
384422
StatementPattern pattern2 = statementPattern(b, c);
@@ -442,6 +480,18 @@ private String aliasProjectionWithResidualFilterQuery() {
442480
+ "}\n";
443481
}
444482

483+
private String orderByNonProjectedVarQuery() {
484+
return "PREFIX foaf: <http://xmlns.com/foaf/0.1/>\n"
485+
+ "SELECT ?a WHERE {\n"
486+
+ " ?a foaf:knows ?b .\n"
487+
+ " ?b foaf:knows ?c .\n"
488+
+ " ?c foaf:knows ?a .\n"
489+
+ " ?a foaf:age ?age .\n"
490+
+ " FILTER (?a != ?b && ?a != ?c && ?b != ?c)\n"
491+
+ "}\n"
492+
+ "ORDER BY ?age ?a\n";
493+
}
494+
445495
private String reorderedDuplicateAliasProjectionQuery() {
446496
return "PREFIX foaf: <http://xmlns.com/foaf/0.1/>\n"
447497
+ "SELECT (?c AS ?z) (?a AS ?x) (?a AS ?x2) WHERE {\n"
@@ -503,6 +553,52 @@ private String repeatedVariableQuery() {
503553
+ "}\n";
504554
}
505555

556+
private String cycleQuery(int size) {
557+
StringBuilder builder = new StringBuilder();
558+
builder.append("PREFIX foaf: <http://xmlns.com/foaf/0.1/>\n");
559+
builder.append("SELECT * WHERE {\n");
560+
appendCyclePattern(builder, size);
561+
appendPairwiseInequalities(builder, size);
562+
builder.append("}\n");
563+
return builder.toString();
564+
}
565+
566+
private String cycleQueryWithValuesBinding(int size, String variableName, String iri) {
567+
StringBuilder builder = new StringBuilder();
568+
builder.append("PREFIX foaf: <http://xmlns.com/foaf/0.1/>\n");
569+
builder.append("SELECT * WHERE {\n");
570+
builder.append(" VALUES ?").append(variableName).append(" { <").append(iri).append("> }\n");
571+
appendCyclePattern(builder, size);
572+
appendPairwiseInequalities(builder, size);
573+
builder.append("}\n");
574+
return builder.toString();
575+
}
576+
577+
private void appendCyclePattern(StringBuilder builder, int size) {
578+
for (int i = 0; i < size; i++) {
579+
builder.append(" ?")
580+
.append(variableName(i))
581+
.append(" foaf:knows ?")
582+
.append(variableName((i + 1) % size))
583+
.append(" .\n");
584+
}
585+
}
586+
587+
private void appendPairwiseInequalities(StringBuilder builder, int size) {
588+
builder.append(" FILTER (");
589+
boolean first = true;
590+
for (int i = 0; i < size; i++) {
591+
for (int j = i + 1; j < size; j++) {
592+
if (!first) {
593+
builder.append(" && ");
594+
}
595+
builder.append("?").append(variableName(i)).append(" != ?").append(variableName(j));
596+
first = false;
597+
}
598+
}
599+
builder.append(")\n");
600+
}
601+
506602
private TupleExpr parsedQueryRoot(String query) throws Exception {
507603
ParsedTupleQuery parsed = QueryParserUtil.parseTupleQuery(QueryLanguage.SPARQL, query, null);
508604
TupleExpr tupleExpr = parsed.getTupleExpr().clone();

0 commit comments

Comments
 (0)