Skip to content

GH-4160: Apply the filter-disjunction rewrite only when the disjuncts are mutually exclusive #4161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
faubulous wants to merge 1 commit into
base: main
Choose a base branch
from
+370 −43
Open

GH-4160: Apply the filter-disjunction rewrite only when the disjuncts are mutually exclusive #4161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
193 changes: 176 additions & 17 deletions ...arq/src/main/java/org/apache/jena/sparql/algebra/optimize/TransformFilterDisjunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
* <p>
* 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 {
Expand All @@ -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> || ?x = <x> ) does not
// result in two transforms.
Set<Expr> doneSoFar = new HashSet<>();

for ( Expr expr : exprList ) {
if ( !isDisjunction(expr) ) {
Expand All @@ -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 = <x> is the same as <x> = ?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 ) {
Expand All @@ -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<Expr> exprList = explodeDisjunction(new ArrayList<Expr>(), 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<Expr> distinct = new ArrayList<>(exprList.size());
Set<Object> 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<Expr> exprList2 = null;
Expand Down Expand Up @@ -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
* <pre>
* FILTER(?x = :c || ?x != :d)</pre>
* must not be expanded: a solution with {@code ?x = :c} satisfies both disjuncts.
* <p>
* 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<Expr> exprList) {
Var var = null;
List<NodeValue> 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<Integer> literals = new ArrayList<>();
List<Integer> nonLiterals = new ArrayList<>();
Set<Node> distinctTerms = new HashSet<>();
for ( int i = 0 ; i < constants.size() ; i++ ) {

@ThomasThelen ThomasThelen Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks O(n^2) to me - is this a hot path that could have performance degraded?

@faubulous faubulous Aug 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is rather tricky: We are looking at expressions like FILTER(A || B || A). In such scenarios we need to pairwise compare all possible variants to be on the safe side. However, this is done at query optimisation time and not for every solution. So it's only run once. Moreover, the amount of expressions to be evaluated is rather limited so the exponential O is not a huge problem here in practise.

But I thought of an alternative where we'd compute a hash value for every expression in the previous loop and then just check if we already have found a match. The issue is that this only works for non-literal values because of SPARQL literal comparison semantics (see explanation below).

So we could add an optimised path here that checks for equality of non-literals quickly and only compare the remaining literals with O(n^2). Would that be OK with you or do you have other ideas?

Literal Comparison Semantics

This is what Claude tells me.. but it sounds plausible:

The chain is provablyDistinctValues → NodeValue.notSameValueAs → NVCompare.sameValueAs → XSDFuncOp.compareNumeric, and that last one calls classifyNumeric(fName, nv1, nv2), which picks the wider of the two operand types (integer → decimal → float → double) and compares in that type. The comparison type is a property of the pair, not of either element — so there is no per-element value to hash.

The concrete consequence is that numeric equality is not transitive:

"0.1"^^xsd:decimal vs "0.1"^^xsd:double → promoted to OP_DOUBLE → equal
"0.1"^^xsd:double vs "0.1000000000000000055511151231257827…"^^xsd:decimal → OP_DOUBLE → equal
"0.1"^^xsd:decimal vs that same decimal → OP_DECIMAL → BigDecimal.compareTo → not equal

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<Expr> exprList, List<NodeValue> 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) {

@ThomasThelen ThomasThelen Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there are two places this sort of logic is being done; maybe the intent is to keep them separate. A third, similar implementation might benefit from sharing some code; I can't say for certain whether it's warranted here though. Place 1, Place 2

@faubulous faubulous Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the spam. I did not anticipate that Claude would write responses in my name without asking. That's a no go. Never worked with it on public PRs so a newbie error. Sincere apologies.

So.. the way I see it is that these do provide similar functionality but differ in the details. There might be a way to consolidate them, but then we'd need to touch a lot more code in other classes. This widens the scope of this fix. Should this be handled in a different PR or do you want to address this here? To me the code duplication is another defect on its own already.

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<Expr> explodeDisjunction(List<Expr> exprList, Expr expr) {
if ( !(expr instanceof E_LogicalOr) ) {
Expand Down
Loading