GH-4872 FilterOptimizer retains scoping of filters (#5316)

Author: hmottestad

core/queryalgebra/evaluation/src/main/java/org/eclipse/rdf4j/query/algebra/evaluation/optimizer/ConjunctiveConstraintSplitterOptimizer.java

@@ -56,7 +56,7 @@ public void meet(Filter filter) {
 			TupleExpr filterArg = filter.getArg();
 
 			for (int i = conjunctiveConstraints.size() - 1; i >= 1; i--) {
-				Filter newFilter = new Filter(filterArg, conjunctiveConstraints.get(i));
+				Filter newFilter = new Filter(filterArg, conjunctiveConstraints.get(i).clone());
 				filterArg = newFilter;
 			}
 
@@ -77,11 +77,11 @@ public void meet(LeftJoin node) {
 
 				for (ValueExpr constraint : conjunctiveConstraints) {
 					if (isWithinBindingScope(constraint, arg)) {
-						arg = new Filter(arg, constraint);
+						arg = new Filter(arg, constraint.clone());
 					} else if (condition == null) {
 						condition = constraint;
 					} else {
-						condition = new And(condition, constraint);
+						condition = new And(condition, constraint.clone());
 					}
 				}
 

core/queryalgebra/evaluation/src/main/java/org/eclipse/rdf4j/query/algebra/evaluation/optimizer/FilterOptimizer.java

@@ -11,6 +11,7 @@
 
 package org.eclipse.rdf4j.query.algebra.evaluation.optimizer;
 
+import java.util.Objects;
 import java.util.Set;
 
 import org.eclipse.rdf4j.query.BindingSet;
@@ -31,6 +32,7 @@
 import org.eclipse.rdf4j.query.algebra.StatementPattern;
 import org.eclipse.rdf4j.query.algebra.TupleExpr;
 import org.eclipse.rdf4j.query.algebra.Union;
+import org.eclipse.rdf4j.query.algebra.VariableScopeChange;
 import org.eclipse.rdf4j.query.algebra.evaluation.QueryOptimizer;
 import org.eclipse.rdf4j.query.algebra.helpers.AbstractQueryModelVisitor;
 import org.eclipse.rdf4j.query.algebra.helpers.AbstractSimpleQueryModelVisitor;
@@ -78,9 +80,49 @@ public class FilterOptimizer implements QueryOptimizer {
 
 	@Override
 	public void optimize(TupleExpr tupleExpr, Dataset dataset, BindingSet bindings) {
-		tupleExpr.visit(new FilterUnMerger());
-		tupleExpr.visit(new FilterOrganizer());
-		tupleExpr.visit(new FilterMerger());
+		Objects.requireNonNull(tupleExpr, "tupleExpr must not be null");
+		optimizeScope(tupleExpr);
+	}
+
+	/**
+	 * Bottom‑up optimisation: optimise each child subtree that starts a new variable scope, then run the three filter
+	 * passes on the current subtree.
+	 */
+	private void optimizeScope(TupleExpr expr) {
+		// 1) recurse into nested scopes first
+		expr.visit(new AbstractQueryModelVisitor<>() {
+			final TupleExpr current = expr;
+
+			@Override
+			protected void meetNode(QueryModelNode node) {
+				if (node != current && node instanceof TupleExpr && node instanceof VariableScopeChange
+						&& ((VariableScopeChange) node).isVariableScopeChange()) {
+
+					optimizeScope(((TupleExpr) node));
+					// do NOT traverse further into that subtree with this visitor
+				} else {
+					super.meetNode(node);
+				}
+			}
+
+		});
+
+		// 2) run the three filter passes for *this* scope
+		expr.visit(new FilterUnMerger());
+		expr.visit(new FilterOrganizer());
+		expr.visit(new FilterMerger());
+	}
+
+	/**
+	 * Copies the {@code isVariableScopeChange} flag from one node to another if both implement
+	 * {@link VariableScopeChange}.
+	 */
+	private static void transferScopeChange(QueryModelNode source, QueryModelNode target) {
+		if (source instanceof VariableScopeChange && target instanceof VariableScopeChange) {
+			VariableScopeChange src = (VariableScopeChange) source;
+			VariableScopeChange tgt = (VariableScopeChange) target;
+			tgt.setVariableScopeChange(src.isVariableScopeChange());
+		}
 	}
 
 	private static class FilterUnMerger extends AbstractSimpleQueryModelVisitor<RuntimeException> {
@@ -95,6 +137,7 @@ public void meet(Filter filter) {
 				And and = (And) filter.getCondition();
 				filter.setCondition(and.getLeftArg().clone());
 				Filter newFilter = new Filter(filter.getArg().clone(), and.getRightArg().clone());
+				transferScopeChange(filter, newFilter); // preserve scope flag
 				filter.replaceChildNode(filter.getArg(), newFilter);
 			}
 			super.meet(filter);
@@ -117,6 +160,7 @@ public void meet(Filter filter) {
 				And merge = new And(childFilter.getCondition().clone(), filter.getCondition().clone());
 
 				Filter newFilter = new Filter(childFilter.getArg().clone(), merge);
+				transferScopeChange(filter, newFilter); // both have same scope flag
 				parent.replaceChildNode(filter, newFilter);
 			}
 		}
@@ -190,6 +234,7 @@ public void meet(LeftJoin leftJoin) {
 		public void meet(Union union) {
 			Filter clone = new Filter();
 			clone.setCondition(filter.getCondition().clone());
+			transferScopeChange(filter, clone);
 
 			relocate(filter, union.getLeftArg());
 			relocate(clone, union.getRightArg());
@@ -202,6 +247,7 @@ public void meet(Union union) {
 		public void meet(Difference node) {
 			Filter clone = new Filter();
 			clone.setCondition(filter.getCondition().clone());
+			transferScopeChange(filter, clone);
 
 			relocate(filter, node.getLeftArg());
 			relocate(clone, node.getRightArg());
@@ -214,6 +260,7 @@ public void meet(Difference node) {
 		public void meet(Intersection node) {
 			Filter clone = new Filter();
 			clone.setCondition(filter.getCondition().clone());
+			transferScopeChange(filter, clone);
 
 			relocate(filter, node.getLeftArg());
 			relocate(clone, node.getRightArg());
@@ -278,5 +325,4 @@ private void relocate(Filter filter, TupleExpr newFilterArg) {
 			}
 		}
 	}
-
 }