good query plans, but large overhead for optimizing the query

Author: hmottestad

core/sail/base/src/main/java/org/eclipse/rdf4j/sail/base/SketchJoinOrderPlanner.java

@@ -202,8 +202,16 @@ private static void addProvidedFilterConditionVars(TupleExpr tupleExpr, Set<Stri
 	}
 
 	private SketchBasedJoinEstimator.SketchPlannerPath classifyGraph() {
-		int physicalComponents = physicalComponentCount();
-		if (physicalComponents > 1 && !isConnectedWithDeferredFilters()) {
+		List<Set<Integer>> physicalComponents = physicalComponents();
+		int requiredPhysicalComponents = 0;
+		Set<Integer> requiredFactors = new LinkedHashSet<>();
+		for (Set<Integer> component : physicalComponents) {
+			if (!isIgnorableSmallBindingSetAssignmentComponent(component)) {
+				requiredPhysicalComponents++;
+				requiredFactors.addAll(component);
+			}
+		}
+		if (requiredPhysicalComponents > 1 && !isConnectedWithDeferredFilters(requiredFactors)) {
 			return SketchBasedJoinEstimator.SketchPlannerPath.UNSUPPORTED_SHAPE;
 		}
 		int edgeCount = 0;
@@ -212,30 +220,32 @@ private SketchBasedJoinEstimator.SketchPlannerPath classifyGraph() {
 			edgeCount += factor.arity();
 			variables.addAll(factor.connectivityVars());
 		}
-		boolean cyclicOrDense = edgeCount != factors.size() + variables.size() - physicalComponents;
-		recordDebug("graph classification: physicalComponents=" + physicalComponents + " variables="
+		boolean cyclicOrDense = edgeCount != factors.size() + variables.size() - physicalComponents.size();
+		recordDebug("graph classification: physicalComponents=" + physicalComponents.size()
+				+ " requiredPhysicalComponents=" + requiredPhysicalComponents + " variables="
 				+ new TreeSet<>(variables) + " edgeCount=" + edgeCount + " cyclicOrDense=" + cyclicOrDense);
 		return null;
 	}
 
-	private int physicalComponentCount() {
+	private List<Set<Integer>> physicalComponents() {
 		Set<Integer> seenFactors = new LinkedHashSet<>();
-		int components = 0;
+		List<Set<Integer>> components = new ArrayList<>();
 		for (int i = 0; i < factors.size(); i++) {
 			if (seenFactors.contains(i)) {
 				continue;
 			}
-			components++;
-			visitPhysicalComponent(i, seenFactors);
+			components.add(physicalComponent(i, seenFactors));
 		}
-		return components;
+		return List.copyOf(components);
 	}
 
-	private void visitPhysicalComponent(int startFactor, Set<Integer> seenFactors) {
+	private Set<Integer> physicalComponent(int startFactor, Set<Integer> seenFactors) {
+		Set<Integer> component = new LinkedHashSet<>();
 		Set<String> seenVariables = new LinkedHashSet<>();
 		ArrayDeque<Object> queue = new ArrayDeque<>();
 		queue.add(Integer.valueOf(startFactor));
 		seenFactors.add(startFactor);
+		component.add(startFactor);
 		while (!queue.isEmpty()) {
 			Object next = queue.removeFirst();
 			if (next instanceof Integer factorIndex) {
@@ -250,18 +260,36 @@ private void visitPhysicalComponent(int startFactor, Set<Integer> seenFactors) {
 			for (int i = 0; i < factors.size(); i++) {
 				if (!seenFactors.contains(i) && factors.get(i).connectivityVars().contains(variable)) {
 					seenFactors.add(i);
+					component.add(i);
 					queue.addLast(Integer.valueOf(i));
 				}
 			}
 		}
+		return Set.copyOf(component);
 	}
 
-	private boolean isConnectedWithDeferredFilters() {
+	private boolean isIgnorableSmallBindingSetAssignmentComponent(Set<Integer> component) {
+		if (component.isEmpty()) {
+			return false;
+		}
+		for (Integer factorIndex : component) {
+			if (!isSmallBindingSetAssignment(factorIndex.intValue())) {
+				return false;
+			}
+		}
+		return true;
+	}
+
+	private boolean isConnectedWithDeferredFilters(Set<Integer> requiredFactors) {
+		if (requiredFactors.isEmpty()) {
+			return true;
+		}
 		Set<Integer> seenFactors = new LinkedHashSet<>();
 		Set<String> seenVariables = new LinkedHashSet<>();
 		ArrayDeque<Object> queue = new ArrayDeque<>();
-		queue.add(Integer.valueOf(0));
-		seenFactors.add(0);
+		Integer startFactor = requiredFactors.iterator().next();
+		queue.add(startFactor);
+		seenFactors.add(startFactor);
 		while (!queue.isEmpty()) {
 			Object next = queue.removeFirst();
 			if (next instanceof Integer factorIndex) {
@@ -285,7 +313,7 @@ private boolean isConnectedWithDeferredFilters() {
 				}
 			}
 		}
-		return seenFactors.size() == factors.size();
+		return seenFactors.containsAll(requiredFactors);
 	}
 
 	private List<String> deferredFilterVariablesConnectedTo(String variable) {
@@ -642,7 +670,8 @@ private String connectionReason(long mask, int candidateIndex) {
 		if (!sharedVariables.isEmpty()) {
 			return sharedVariables.size() == 1 ? sharedVariables.iterator().next() : "shared:" + sharedVariables;
 		}
-		if (mask != 0L && isSmallBindingSetAssignment(candidateIndex)) {
+		if (mask != 0L && isSmallBindingSetAssignment(candidateIndex)
+				&& !hasReachableNonSmallCandidate(mask, candidateIndex)) {
 			return "small-values-anchor";
 		}
 		if (isSmallBindingSetOnlyMask(mask)) {
@@ -655,6 +684,19 @@ private String connectionReason(long mask, int candidateIndex) {
 		return null;
 	}
 
+	private boolean hasReachableNonSmallCandidate(long mask, int ignoredCandidateIndex) {
+		for (int i = 0; i < factors.size(); i++) {
+			if (i == ignoredCandidateIndex || contains(mask, i) || isSmallBindingSetAssignment(i)) {
+				continue;
+			}
+			if (!sharedVariables(mask, i).isEmpty() || deferredFilterConnection(mask, i) != null
+					|| isSmallBindingSetOnlyMask(mask)) {
+				return true;
+			}
+		}
+		return false;
+	}
+
 	private boolean isSmallBindingSetOnlyMask(long mask) {
 		if (mask == 0L) {
 			return false;

core/sail/base/src/test/java/org/eclipse/rdf4j/sail/base/SketchBasedJoinEstimatorJoinOrderPlannerTest.java

@@ -366,7 +366,7 @@ void planJoinOrderUsesBindingSetAssignmentSketchIntersectionForFinalRows() {
 	}
 
 	@Test
-	void planJoinOrderHandlesDisconnectedBindingAssignmentAlongsideConnectedChain() {
+	void planJoinOrderSupportsDisconnectedSmallBindingAssignmentAlongsideConnectedChain() {
 		StubSailStore store = new StubSailStore();
 		IRI rdfType = VF.createIRI("http://www.w3.org/1999/02/22-rdf-syntax-ns#type");
 		IRI patientType = VF.createIRI("urn:Patient");
@@ -408,17 +408,25 @@ void planJoinOrderHandlesDisconnectedBindingAssignmentAlongsideConnectedChain()
 
 		Optional<JoinOrderPlanner.JoinOrderPlan> greedyPlan = estimator.planJoinOrder(args, Set.of(),
 				JoinOrderPlanner.Algorithm.GREEDY);
-		assertTrue(greedyPlan.isEmpty(),
-				"Disconnected binding assignments should now fall back through Optional.empty()");
-		assertEquals(SketchBasedJoinEstimator.SketchPlannerPath.UNSUPPORTED_SHAPE, estimator.lastJoinOrderPlannerPath(),
-				"Disconnected binding assignments should report an unsupported planner shape");
+		assertTrue(greedyPlan.isPresent(),
+				"Small disconnected binding assignments should not make the connected graph unsupported");
+		assertEquals(SketchBasedJoinEstimator.SketchPlannerPath.ROBUST_USED, estimator.lastJoinOrderPlannerPath(),
+				"Small disconnected binding assignments should stay on the robust planner path");
+		assertEquals(bindings, greedyPlan.get().getOrderedArgs().get(greedyPlan.get().getOrderedArgs().size() - 1),
+				"Small disconnected binding assignments should be appended after the connected graph");
 
 		Optional<JoinOrderPlanner.JoinOrderPlan> dynamicProgrammingPlan = estimator.planJoinOrder(args, Set.of(),
 				JoinOrderPlanner.Algorithm.DYNAMIC_PROGRAMMING);
-		assertTrue(dynamicProgrammingPlan.isEmpty(),
-				"Disconnected binding assignments should now fall back through Optional.empty()");
-		assertEquals(SketchBasedJoinEstimator.SketchPlannerPath.UNSUPPORTED_SHAPE, estimator.lastJoinOrderPlannerPath(),
-				"Disconnected binding assignments should report an unsupported planner shape");
+		assertTrue(dynamicProgrammingPlan.isPresent(),
+				"Small disconnected binding assignments should not make the connected graph unsupported");
+		assertEquals(SketchBasedJoinEstimator.SketchPlannerPath.ROBUST_USED, estimator.lastJoinOrderPlannerPath(),
+				"Small disconnected binding assignments should stay on the robust planner path");
+		assertEquals(bindings,
+				dynamicProgrammingPlan.get()
+						.getOrderedArgs()
+						.get(dynamicProgrammingPlan.get().getOrderedArgs().size()
+								- 1),
+				"Small disconnected binding assignments should be appended after the connected graph");
 	}
 
 	@Test