diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformFilterDisjunction.java b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformFilterDisjunction.java index 44d4614c63e..3eb703f6daf 100644 --- a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformFilterDisjunction.java +++ b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformFilterDisjunction.java @@ -27,13 +27,20 @@ import java.util.Set ; import org.apache.jena.atlas.logging.Log ; +import org.apache.jena.graph.Node ; import org.apache.jena.sparql.algebra.Op ; import org.apache.jena.sparql.algebra.TransformCopy ; import org.apache.jena.sparql.algebra.op.OpDisjunction ; import org.apache.jena.sparql.algebra.op.OpFilter ; +import org.apache.jena.sparql.core.Var ; +import org.apache.jena.sparql.expr.E_Equals ; import org.apache.jena.sparql.expr.E_LogicalOr ; +import org.apache.jena.sparql.expr.E_SameTerm ; import org.apache.jena.sparql.expr.Expr ; +import org.apache.jena.sparql.expr.ExprEvalException ; +import org.apache.jena.sparql.expr.ExprFunction2 ; import org.apache.jena.sparql.expr.ExprList ; +import org.apache.jena.sparql.expr.NodeValue ; /** * Filter disjunction. This covers the case of @@ -42,6 +49,14 @@ * where either or both of {@code expr1} and {@code expr2} are equalities that help * ground the pattern. This includes {@code ?x IN (....)} so this optimization can a * significant improvement. + *

+ * The rewrite evaluates the pattern once per disjunct, so it is only sound when at + * most one disjunct can be true of any one solution; otherwise a solution satisfying + * several disjuncts is returned once per satisfied disjunct where the filter returns + * it once. Repeated disjuncts are dropped first, and the rewrite is then applied only + * when every disjunct tests the same variable against a constant ({@code =} or + * {@code sameTerm}) and no one term can satisfy two of those tests. Any other + * disjunction is left as a filter. */ public class TransformFilterDisjunction extends TransformCopy { @@ -66,9 +81,6 @@ public Op transform(OpFilter opFilter, final Op subOp) { ExprList exprList2 = new ExprList(); Op newOp = subOp; - // remember what's been seen so that FILTER(?x = || ?x = ) does not - // result in two transforms. - Set doneSoFar = new HashSet<>(); for ( Expr expr : exprList ) { if ( !isDisjunction(expr) ) { @@ -77,26 +89,23 @@ public Op transform(OpFilter opFilter, final Op subOp) { continue; } -// // Relies on expression equality. -// if ( doneSoFar.contains(expr) ) -// continue ; -// // Must be canonical: ?x = is the same as = ?x -// doneSoFar.add(expr) ; - Op op2 = expandDisjunction(expr, newOp); - if ( op2 != null ) - newOp = op2; + if ( op2 == null ) { + // A disjunction this transform can not rewrite soundly. + // Leave it as a filter expression. + exprList2.add(expr); + continue; + } + newOp = op2; } + if ( newOp == subOp ) + // No disjunction was expanded. + return super.transform(opFilter, subOp); + if ( exprList2.isEmpty() ) return newOp; - // There should have been at least on disjunction. - if ( newOp == subOp ) { - Log.warn(this, "FilterDisjunction assumption failure: didn't find a disjunction after all"); - return super.transform(opFilter, subOp); - } - // Put the non-disjunctions outside the disjunction and the pattern rewrite. Op opOther = OpFilter.filterBy(exprList2, newOp); if ( opOther instanceof OpFilter ) { @@ -113,9 +122,27 @@ private boolean isDisjunction(Expr expr) { return (expr instanceof E_LogicalOr); } + /** + * Expand a disjunction into a union of the pattern grounded per disjunct, or null + * when that is not possible or not sound. + */ public static Op expandDisjunction(Expr expr, Op subOp) { List exprList = explodeDisjunction(new ArrayList(), expr); + // (A || A) is A: drop repeated disjuncts rather than build identical branches. + // Generated queries really do contain the same disjunct twice - LDBC SPB writes + // FILTER(?pf = :c || ?pf = :c) - and a single disjunct then grounds the pattern. + List distinct = new ArrayList<>(exprList.size()); + Set seen = new HashSet<>(); + for ( Expr e : exprList ) { + if ( seen.add(dedupKey(e)) ) + distinct.add(e); + } + exprList = distinct; + + if ( !isSafeDisjunction(exprList) ) + return null; + // All disjunctions - some can be done efficiently via assignments, // some can not (value tests). List exprList2 = null; @@ -148,6 +175,138 @@ public static Op expandDisjunction(Expr expr, Op subOp) { return op; } + /** + * Is at most one disjunct true of any one solution? Each branch of the expansion + * re-evaluates the pattern, so a solution that satisfies {@code k} disjuncts comes + * back {@code k} times where the filter returns it once. For example + * 
+     *  FILTER(?x = :c || ?x != :d)
+ * must not be expanded: a solution with {@code ?x = :c} satisfies both disjuncts. + *

+ * The safe case is: every disjunct tests one and the same variable against a + * constant, and the constants are pairwise known not to be satisfied by the same + * term. Constants whose comparison is indeterminate (an unknown datatype, a + * timezone-less date) are treated as possibly equal. + */ + private static boolean isSafeDisjunction(List exprList) { + Var var = null; + List constants = new ArrayList<>(exprList.size()); + for ( Expr e : exprList ) { + NodeValue constant = constantTestedAgainst(e, var); + if ( constant == null ) + return false; + if ( var == null ) + var = singleVariable(e); + constants.add(constant); + } + + // This runs once per query at optimize time, not per solution, but ?x IN (...) + // lists can be long in generated queries so avoid the pairwise comparison where + // there is a cheaper test. For IRIs and blank nodes NVCompare.sameValueAs is + // sameTerm, so they have a canonical representative and distinctness is set + // membership. Literals have no such representative - numeric comparison promotes + // to the wider of the two types, which makes value equality a property of the + // pair and not transitive - so any pair involving one stays pairwise. + List literals = new ArrayList<>(); + List nonLiterals = new ArrayList<>(); + Set distinctTerms = new HashSet<>(); + for ( int i = 0 ; i < constants.size() ; i++ ) { + NodeValue nv = constants.get(i); + if ( nv.isIRI() || nv.isBlank() ) { + if ( !distinctTerms.add(nv.asNode()) ) + // The same term twice: both disjuncts are true of that term. + return false; + nonLiterals.add(i); + } else + literals.add(i); + } + + for ( int a = 0 ; a < literals.size() ; a++ ) { + int i = literals.get(a); + for ( int b = a + 1 ; b < literals.size() ; b++ ) { + if ( !provablyExclusive(exprList, constants, i, literals.get(b)) ) + return false; + } + for ( int j : nonLiterals ) { + if ( !provablyExclusive(exprList, constants, i, j) ) + return false; + } + } + return true; + } + + /** + * Can no one term satisfy both disjuncts? {@code sameTerm} matches by term, so two + * {@code sameTerm} tests exclude each other exactly when the terms differ; value + * distinctness is not enough because {@code NaN} is not value-equal to itself, yet + * every term equal to {@code NaN} satisfies both. Where at least one disjunct is + * {@code =}, a solution satisfying both makes the constants value-equal, so proving + * the values different proves the disjuncts exclusive. + */ + private static boolean provablyExclusive(List exprList, List constants, int i, int j) { + return provablyExclusive(exprList.get(i), constants.get(i), exprList.get(j), constants.get(j)); + } + + /*package*/ static boolean provablyExclusive(Expr e1, NodeValue nv1, Expr e2, NodeValue nv2) { + if ( e1 instanceof E_SameTerm && e2 instanceof E_SameTerm ) + return !nv1.asNode().equals(nv2.asNode()); + return provablyDistinctValues(nv1, nv2); + } + + /** + * A key equating disjuncts that are the same test. {@code =} and {@code sameTerm} + * are symmetric, so a variable/constant test keys on the operator, the variable and + * the constant and not on the argument order - FILTER(sameTerm(?x, :c) || + * sameTerm(:c, ?x)) is one test written twice. Any other shape keys on itself. + */ + private static Object dedupKey(Expr e) { + NodeValue constant = constantTestedAgainst(e, null); + if ( constant == null ) + return e; + return List.of(e.getClass(), singleVariable(e), constant.asNode()); + } + + /** + * The constant of a {@code variable = constant} or {@code sameTerm(variable, constant)} + * disjunct (either argument order), where the variable is {@code var} - or any + * variable when {@code var} is null. Null when the disjunct has another shape. + */ + private static NodeValue constantTestedAgainst(Expr e, Var var) { + if ( !(e instanceof E_Equals) && !(e instanceof E_SameTerm) ) + return null; + ExprFunction2 test = (ExprFunction2)e; + Expr left = test.getArg1(); + Expr right = test.getArg2(); + Expr varExpr = null; + Expr constExpr = null; + if ( left.isVariable() && right.isConstant() ) { + varExpr = left; + constExpr = right; + } else if ( right.isVariable() && left.isConstant() ) { + varExpr = right; + constExpr = left; + } else + return null; + if ( var != null && !var.equals(varExpr.asVar()) ) + return null; + return constExpr.getConstant(); + } + + /** The variable of a disjunct {@link #constantTestedAgainst} accepted. */ + private static Var singleVariable(Expr e) { + ExprFunction2 test = (ExprFunction2)e; + return test.getArg1().isVariable() ? test.getArg1().asVar() : test.getArg2().asVar(); + } + + /*package*/ static boolean provablyDistinctValues(NodeValue nv1, NodeValue nv2) { + try { + return NodeValue.notSameValueAs(nv1, nv2); + } catch (ExprEvalException ex) { + // Indeterminate comparison: can not prove the disjuncts mutually exclusive. + return false; + } + } + /** Explode an expr into a list of disjunctions */ private static List explodeDisjunction(List exprList, Expr expr) { if ( !(expr instanceof E_LogicalOr) ) { diff --git a/jena-arq/src/test/java/org/apache/jena/sparql/algebra/optimize/TestTransformFilters.java b/jena-arq/src/test/java/org/apache/jena/sparql/algebra/optimize/TestTransformFilters.java index 53163864d4d..db6f183446b 100644 --- a/jena-arq/src/test/java/org/apache/jena/sparql/algebra/optimize/TestTransformFilters.java +++ b/jena-arq/src/test/java/org/apache/jena/sparql/algebra/optimize/TestTransformFilters.java @@ -23,14 +23,33 @@ import static org.apache.jena.sparql.algebra.optimize.TransformTests.check; import static org.apache.jena.sparql.algebra.optimize.TransformTests.testOp; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; +import org.apache.jena.atlas.iterator.Iter; import org.apache.jena.atlas.lib.StrUtils; +import org.apache.jena.datatypes.xsd.XSDDatatype; +import org.apache.jena.graph.Graph; +import org.apache.jena.graph.NodeFactory; +import org.apache.jena.query.Query; +import org.apache.jena.query.QueryExecution; +import org.apache.jena.query.QueryExecutionFactory; +import org.apache.jena.query.QueryFactory; +import org.apache.jena.rdf.model.ModelFactory; import org.apache.jena.sparql.algebra.Op; import org.apache.jena.sparql.algebra.Transform; import org.apache.jena.sparql.algebra.TransformCopy; import org.apache.jena.sparql.algebra.op.OpTable; +import org.apache.jena.sparql.core.Var; +import org.apache.jena.sparql.expr.E_Equals; +import org.apache.jena.sparql.expr.E_SameTerm; +import org.apache.jena.sparql.expr.Expr; +import org.apache.jena.sparql.expr.ExprVar; +import org.apache.jena.sparql.expr.NodeValue; +import org.apache.jena.sparql.sse.SSE; /** Tests of transforms related to filters */ public class TestTransformFilters @@ -272,23 +291,18 @@ public Op transform(OpTable opTable) { ")"); } + // A solution with ?x = satisfies both disjuncts, so expanding would return + // it from both branches where the filter returns it once. Not transformed. @Test public void disjunction02() { testOp("(filter (|| (= ?x ) (!= ?x )) (bgp ( ?s ?p ?x)) )", t_disjunction, - "(disjunction ", - "(assign ((?x )) (bgp ( ?s ?p )))", - "(filter (!= ?x ) (bgp ( ?s ?p ?x)))", - ")"); + (String[])null); } @Test public void disjunction03() { testOp("(filter (|| (!= ?x ) (= ?x )) (bgp ( ?s ?p ?x)) )", t_disjunction, - // Note - reordering of disjunction terms. - "(disjunction ", - "(assign ((?x )) (bgp ( ?s ?p )))", - "(filter (!= ?x ) (bgp ( ?s ?p ?x)))", - ")"); + (String[])null); } @Test public void disjunction04() { @@ -300,33 +314,187 @@ public Op transform(OpTable opTable) { @Test public void disjunction05() { testOp("(filter (exprlist (|| (= ?x ) (!= ?x ))) (bgp ( ?s ?p ?x)) )", t_disjunction, - " (disjunction", - " (assign ((?x )) (bgp ( ?s ?p )))", - " (filter (!= ?x ) (bgp ( ?s ?p ?x)))", - ")" - ); + (String[])null); } @Test public void disjunction06() { testOp("(filter (exprlist (lang ?x) (|| (= ?x ) (!= ?x ))) (bgp ( ?s ?p ?x)) )", t_disjunction, - "(filter (lang ?x)", - " (disjunction", - " (assign ((?x )) (bgp ( ?s ?p )))", - " (filter (!= ?x ) (bgp ( ?s ?p ?x)))", - "))" - ); + (String[])null); } @Test public void disjunction07() { testOp("(filter (exprlist (|| (= ?x ) (!= ?x )) (lang ?x) ) (bgp ( ?s ?p ?x)) )", t_disjunction, - "(filter (lang ?x)", - " (disjunction", - " (assign ((?x )) (bgp ( ?s ?p )))", - " (filter (!= ?x ) (bgp ( ?s ?p ?x)))", - "))" - ); + (String[])null); + } + + // (A || A) is A: the repeated disjunct is dropped — two identical branches would + // return every solution twice — and the single equality then grounds the pattern. + @Test public void disjunction08() { + testOp("(filter (|| (= ?x ) (= ?x )) (bgp ( ?s ?p ?x)) )", + t_disjunction, + "(assign ((?x )) (bgp ( ?s ?p )))"); + } + + // The same, for a disjunct shape the transform cannot ground: deduplicated to a + // single disjunct, there is nothing to expand and the filter is left alone. + @Test public void disjunction08a() { + testOp("(filter (|| (!= ?x ) (!= ?x )) (bgp ( ?s ?p ?x)) )", + t_disjunction, + (String[])null); + } + + // Different terms, same value: a solution with ?x = 1 satisfies both disjuncts. + @Test public void disjunction09() { + testOp("(filter (|| (= ?x 1) (= ?x \"01\"^^)) (bgp ( ?s ?p ?x)) )", + t_disjunction, + (String[])null); + } + + // Different variables: a solution can satisfy both disjuncts. + @Test public void disjunction10() { + testOp("(filter (|| (= ?x ) (= ?y )) (bgp ( ?s ?p ?x) (?s ?q ?y)) )", + t_disjunction, + (String[])null); + } + + // Simple literals are pairwise distinct values, so the expansion is sound. + @Test public void disjunction11() { + testOp("(filter (|| (= ?x \"a\") (= ?x \"b\")) (bgp ( ?s ?p ?x)) )", + t_disjunction, + "(disjunction ", + "(assign ((?x \"a\")) (bgp ( ?s ?p \"a\")))", + "(assign ((?x \"b\")) (bgp ( ?s ?p \"b\")))", + ")"); + } + + // An IRI and a number are distinct values, so the ?x IN ( 2) shape keeps its + // expansion; the numeric equality is not substitutable and stays a filter branch. + @Test public void disjunction12() { + testOp("(filter (|| (= ?x ) (= ?x 2)) (bgp ( ?s ?p ?x)) )", + t_disjunction, + "(disjunction ", + "(assign ((?x )) (bgp ( ?s ?p )))", + "(filter (= ?x 2) (bgp ( ?s ?p ?x)))", + ")"); + } + + // The expansion must not change the number of results: each solution once, + // however many disjuncts it satisfies. Executed, not matched on plan shape, + // with the default optimizer. + @Test public void disjunctionMultiplicity01() { + checkDisjunctionRowCount("FILTER(?x = || ?x = )", 1); + } + + // Only s1 qualifies (x=a passes both disjuncts; x=b passes neither) — and it must + // come back once. The unsound expansion returned it from both branches. + @Test public void disjunctionMultiplicity02() { + checkDisjunctionRowCount("FILTER(?x = || ?x != )", 1); + } + + @Test public void disjunctionMultiplicity03() { + checkDisjunctionRowCount("FILTER(?x = || ?x = )", 2); + } + + private static void checkDisjunctionRowCount(String filter, int expected) { + Graph graph = SSE.parseGraph(StrUtils.strjoinNL + ("(graph" + ," (triple )" + ," (triple )" + ,")")); + String queryString = "SELECT * { ?s ?x " + filter + " }"; + Query query = QueryFactory.create(queryString); + try ( QueryExecution qExec = QueryExecutionFactory.create(query, ModelFactory.createModelForGraph(graph)) ) { + long count = Iter.count(qExec.execSelect()); + assertEquals(expected, count, ()->"Row count differs from filter semantics: "+filter); + } + } + + // GH-4160: the exclusivity test must not judge sameTerm disjuncts by value + // distinctness. NaN is not value-equal to itself, so the two symmetric writings of + // one sameTerm test were taken to be mutually exclusive and the row came back twice. + @Test public void disjunctionMultiplicity04() { + checkRowCount(numericGraph(), + "FILTER( sameTerm(?x, \"NaN\"^^xsd:double) || sameTerm(\"NaN\"^^xsd:double, ?x) )", + 1); + } + + // The mixed IRI/literal case still expands - the IRI branch grounds the pattern and + // the double is left as a filter - and returns each solution once. + @Test public void disjunctionMultiplicity05() { + checkRowCount(numericGraph(), + "FILTER( ?x = \"1.5\"^^xsd:double || ?x = )", + 1); + } + + private static Graph numericGraph() { + return SSE.parseGraph(StrUtils.strjoinNL + ("(graph" + ," (triple \"NaN\"^^xsd:double)" + ," (triple \"1.5\"^^xsd:double)" + ,")")); + } + + private static void checkRowCount(Graph graph, String filter, int expected) { + String queryString = "PREFIX xsd: \n" + + "SELECT * { ?s ?x " + filter + " }"; + Query query = QueryFactory.create(queryString); + try ( QueryExecution qExec = QueryExecutionFactory.create(query, ModelFactory.createModelForGraph(graph)) ) { + long count = Iter.count(qExec.execSelect()); + assertEquals(expected, count, ()->"Row count differs from filter semantics: "+filter); + } + } + + private static NodeValue nvDouble(String lex) { return NodeValue.makeNode(NodeFactory.createLiteralDT(lex, XSDDatatype.XSDdouble)); } + private static NodeValue nvInteger(String lex) { return NodeValue.makeNode(NodeFactory.createLiteralDT(lex, XSDDatatype.XSDinteger)); } + private static NodeValue nvDateTime(String lex) { return NodeValue.makeNode(NodeFactory.createLiteralDT(lex, XSDDatatype.XSDdateTime)); } + private static NodeValue nvIRI(String uri) { return NodeValue.makeNode(NodeFactory.createURI(uri)); } + + // provablyDistinctValues is value distinctness. NaN is not value-equal to itself - + // the reason sameTerm disjuncts may not be judged by it. + @Test public void provablyDistinctValuesNaN() { + assertTrue(TransformFilterDisjunction.provablyDistinctValues(nvDouble("NaN"), nvDouble("NaN"))); + } + + // Value-equal constants with different terms: not distinct, so not exclusive. + @Test public void provablyDistinctValuesSameValue() { + assertFalse(TransformFilterDisjunction.provablyDistinctValues(nvInteger("1"), nvInteger("01"))); + assertFalse(TransformFilterDisjunction.provablyDistinctValues(nvInteger("1"), nvDouble("1.0"))); + } + + @Test public void provablyDistinctValuesIRIs() { + assertTrue(TransformFilterDisjunction.provablyDistinctValues(nvIRI("http://example/a"), nvIRI("http://example/b"))); + assertFalse(TransformFilterDisjunction.provablyDistinctValues(nvIRI("http://example/a"), nvIRI("http://example/a"))); + } + + // A timezone-less dateTime against one with a timezone is indeterminate, so the + // constants can not be proved different. + @Test public void provablyDistinctValuesIndeterminate() { + assertFalse(TransformFilterDisjunction.provablyDistinctValues(nvDateTime("2000-01-01T00:00:00"), + nvDateTime("2000-01-01T00:00:00Z"))); + } + + // sameTerm matches by term: identical terms are never exclusive, and differing + // terms always are, whatever the value comparison says. + @Test public void provablyExclusiveSameTerm() { + Expr vx = new ExprVar(Var.alloc("x")); + NodeValue nan = nvDouble("NaN"); + assertFalse(TransformFilterDisjunction.provablyExclusive(new E_SameTerm(vx, nan), nan, + new E_SameTerm(nan, vx), nan)); + NodeValue i1 = nvInteger("1"), i01 = nvInteger("01"); + assertTrue(TransformFilterDisjunction.provablyExclusive(new E_SameTerm(vx, i1), i1, + new E_SameTerm(vx, i01), i01)); + } + + // Where a disjunct is "=", value distinctness is the right test. + @Test public void provablyExclusiveEquals() { + Expr vx = new ExprVar(Var.alloc("x")); + NodeValue i1 = nvInteger("1"), i01 = nvInteger("01"); + assertFalse(TransformFilterDisjunction.provablyExclusive(new E_Equals(vx, i1), i1, + new E_Equals(vx, i01), i01)); + assertTrue(TransformFilterDisjunction.provablyExclusive(new E_Equals(vx, nvIRI("http://example/a")), nvIRI("http://example/a"), + new E_SameTerm(vx, nvIRI("http://example/b")), nvIRI("http://example/b"))); } @Test public void oneOf1() {