Skip to content

Commit

Permalink
Merge pull request eXist-db#4882 from evolvedbinary/fix/3624
Browse files Browse the repository at this point in the history 
Prevent unnecessary optimisation of already optimised Range Index expressions
  • Loading branch information
@reinhapa
reinhapa authored Sep 2, 2024
2 parents ac639da + 90883bd commit 4909edb
Show file tree
Hide file tree
Showing 5 changed files with 118 additions and 66 deletions.
7 changes: 6 additions & 1 deletion exist-core/src/main/java/org/exist/storage/NodePath.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,10 +243,15 @@ public final boolean match(final NodePath other, int j) {
public String toString() {
final StringBuilder buf = new StringBuilder();
for (int i = 0; i < pos; i++) {
buf.append("/");

if (!(SKIP.equals(components[i]) || (i > 0 && SKIP.equals(components[i - 1])))) {
buf.append("/");
}

if (components[i].getNameType() == ElementValue.ATTRIBUTE) {
buf.append("@");
}

buf.append(components[i]);
}
return buf.toString();
Expand Down
71 changes: 52 additions & 19 deletions extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/Lookup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.exist.xquery.*;
import org.exist.xquery.value.*;

import javax.annotation.Nullable;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Iterator;
Expand Down Expand Up @@ -134,46 +135,78 @@ public class Lookup extends Function implements Optimizable {
)
};

public static Lookup create(XQueryContext context, RangeIndex.Operator operator, NodePath contextPath) {
for (FunctionSignature sig: signatures) {
public static @Nullable Lookup create(final XQueryContext context, final RangeIndex.Operator operator, final NodePath contextPath) {
for (final FunctionSignature sig : signatures) {
if (sig.getName().getLocalPart().equals(operator.toString())) {
return new Lookup(context, sig, contextPath);
}
}
return null;
}

private LocationStep contextStep = null;
protected QName contextQName = null;
protected int axis = Constants.UNKNOWN_AXIS;
private NodeSet preselectResult = null;
protected boolean canOptimize = false;
protected boolean optimizeSelf = false;
protected boolean optimizeChild = false;
protected boolean usesCollation = false;
protected Expression fallback = null;
protected NodePath contextPath = null;

public Lookup(XQueryContext context, FunctionSignature signature) {
@Nullable private LocationStep contextStep = null;
@Nullable private QName contextQName = null;
private int axis = Constants.UNKNOWN_AXIS;
@Nullable private NodeSet preselectResult = null;
private boolean canOptimize = false;
private boolean optimizeSelf = false;
private boolean optimizeChild = false;
private boolean usesCollation = false;
@Nullable private Expression fallback = null;
@Nullable private NodePath contextPath;

/**
* Constructor called via reflection from {@link Function#createFunction(XQueryContext, XQueryAST, Module, FunctionDef)}.
*
* @param context The XQuery Context.
* @param signature The signature of the Lookup function.
*/
@SuppressWarnings("unused")
public Lookup(final XQueryContext context, final FunctionSignature signature) {
this(context, signature, null);
}

public Lookup(XQueryContext context, FunctionSignature signature, NodePath contextPath) {
/**
* Constructor called via {@link #create(XQueryContext, RangeIndex.Operator, NodePath)}.
*
* @param context The XQuery Context.
* @param signature The signature of the Lookup function.
* @param contextPath the node path of the optimization.
*/
private Lookup(final XQueryContext context, final FunctionSignature signature, @Nullable final NodePath contextPath) {
super(context, signature);
this.contextPath = contextPath;
}

public void setFallback(Expression expression, int optimizeAxis) {
if (expression instanceof InternalFunctionCall) {
expression = ((InternalFunctionCall)expression).getFunction();
/**
* Get the node path of the optimization.
*
* @return the node path of the optimization, or null if unknown.
*/
public @Nullable NodePath getContextPath() {
return contextPath;
}

/**
* Set the node path of the optimization.
*
* @param contextPath the node path of the optimization, or null if unknown.
*/
public void setContextPath(@Nullable final NodePath contextPath) {
this.contextPath = contextPath;
}

public void setFallback(@Nullable Expression expression, final int optimizeAxis) {
if (expression instanceof final InternalFunctionCall fcall) {
expression = fcall.getFunction();
}
this.fallback = expression;
// we need to know the axis at this point. the optimizer will call
// getOptimizeAxis before analyze
this.axis = optimizeAxis;
}

public Expression getFallback() {
public @Nullable Expression getFallback() {
return fallback;
}

Expand Down
102 changes: 57 additions & 45 deletions ...nsions/indexes/range/src/main/java/org/exist/xquery/modules/range/RangeQueryRewriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,14 @@
*/
package org.exist.xquery.modules.range;

import org.exist.indexing.range.RangeIndex;
import org.exist.indexing.range.*;
import org.exist.storage.NodePath;
import org.exist.xquery.*;
import org.exist.xquery.Constants.Comparison;

import javax.annotation.Nullable;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

/**
Expand Down Expand Up @@ -60,16 +61,16 @@ public Pragma rewriteLocationStep(final LocationStep locationStep) throws XPathE
// will become true if optimizable expression is found
boolean canOptimize = false;
// get path of path expression before the predicates
NodePath contextPath = toNodePath(getPrecedingSteps(locationStep));
final NodePath contextPath = toNodePath(getPrecedingSteps(locationStep));
// process the remaining predicates
for (Predicate pred : preds) {
for (final Predicate pred : preds) {
if (pred.getLength() != 1) {
// can only optimize predicates with one expression
break;
}

Expression innerExpr = pred.getExpression(0);
List<LocationStep> steps = getStepsToOptimize(innerExpr);
final Expression innerExpr = pred.getExpression(0);
final List<LocationStep> steps = getStepsToOptimize(innerExpr);
if (steps == null || steps.isEmpty()) {
// no optimizable steps found
continue;
Expand All @@ -89,11 +90,11 @@ public Pragma rewriteLocationStep(final LocationStep locationStep) throws XPathE
}

// compute left hand path
NodePath innerPath = toNodePath(steps);
final NodePath innerPath = toNodePath(steps);
if (innerPath == null) {
continue;
}
NodePath path;
final NodePath path;
if (contextPath == null) {
path = innerPath;
} else {
Expand All @@ -102,14 +103,24 @@ public Pragma rewriteLocationStep(final LocationStep locationStep) throws XPathE
}

if (path.length() > 0) {
// replace with call to lookup function
// collect arguments
Lookup func = rewrite(innerExpr, path);
// preserve original comparison: may need it for in-memory lookups
func.setFallback(innerExpr, axis);
func.setLocation(innerExpr.getLine(), innerExpr.getColumn());

pred.replace(innerExpr, new InternalFunctionCall(func));
if (innerExpr instanceof final InternalFunctionCall internalFunctionCall
&& internalFunctionCall.getFunction() instanceof final Lookup lookup) {

// innerExpr was already optimized, just update the contextPath if it is missing
if (lookup.getContextPath() == null) {
lookup.setContextPath(path);;
}

} else {
// replace with call to lookup function
final Lookup func = rewrite(innerExpr, path);
// preserve original comparison: may need it for in-memory lookups
func.setFallback(innerExpr, axis);
func.setLocation(innerExpr.getLine(), innerExpr.getColumn());

pred.replace(innerExpr, new InternalFunctionCall(func));
}

canOptimize = true;
}
}
Expand All @@ -125,28 +136,22 @@ public Pragma rewriteLocationStep(final LocationStep locationStep) throws XPathE
return null;
}

protected static Lookup rewrite(Expression expression, NodePath path) throws XPathException {
ArrayList<Expression> eqArgs = new ArrayList<>(2);
if (expression instanceof GeneralComparison comparison) {
eqArgs.add(comparison.getLeft());
eqArgs.add(comparison.getRight());
Lookup func = Lookup.create(comparison.getContext(), getOperator(expression), path);
if (func != null) {
func.setArguments(eqArgs);
return func;
protected static @Nullable Lookup rewrite(final Expression expression, final NodePath path) throws XPathException {
if (expression instanceof final GeneralComparison comparison) {
final List<Expression> eqArgs = Arrays.asList(comparison.getLeft(), comparison.getRight());
final Lookup lookup = Lookup.create(comparison.getContext(), getOperator(expression), path);
if (lookup != null) {
lookup.setArguments(eqArgs);
}

} else if (expression instanceof InternalFunctionCall fcall) {
Function function = fcall.getFunction();
if (function != null && function instanceof Lookup) {
final RangeIndex.Operator operator = RangeIndex.Operator.getByName(function.getName().getLocalPart());
eqArgs.add(function.getArgument(0));
eqArgs.add(function.getArgument(1));
Lookup func = Lookup.create(function.getContext(), operator, path);
func.setArguments(eqArgs);
return func;
return lookup;
} else if (expression instanceof final InternalFunctionCall fcall) {
final Function function = fcall.getFunction();
if (function instanceof final Lookup lookup && lookup.getContextPath() == null) {
lookup.setContextPath(path);
return lookup;
}
}

return null;
}

Expand All @@ -156,6 +161,7 @@ protected static List<LocationStep> getStepsToOptimize(Expression expr) {
} else if (expr instanceof InternalFunctionCall fcall) {
Function function = fcall.getFunction();
if (function instanceof Lookup) {
// TODO(AR) is this check for range:matches needed here?
if (function.isCalledAs("matches")) {
return BasicExpressionVisitor.findLocationSteps(function.getArgument(0));
} else {
Expand All @@ -168,14 +174,20 @@ protected static List<LocationStep> getStepsToOptimize(Expression expr) {
}

public static RangeIndex.Operator getOperator(Expression expr) {
if (expr instanceof InternalFunctionCall fcall) {
Function function = fcall.getFunction();
if (function instanceof Lookup) {
expr = ((Lookup)function).getFallback();
if (expr instanceof final InternalFunctionCall fcall) {
final Function function = fcall.getFunction();
if (function instanceof final Lookup lookup) {
final Expression fallback = lookup.getFallback();
if (fallback != null) {
expr = fallback;
} else {
expr = lookup;
}
}
}

RangeIndex.Operator operator = RangeIndex.Operator.EQ;
if (expr instanceof GeneralComparison comparison) {
if (expr instanceof final GeneralComparison comparison) {
final Comparison relation = comparison.getRelation();
operator = switch (relation) {
case LT -> RangeIndex.Operator.LT;
Expand All @@ -191,14 +203,14 @@ public static RangeIndex.Operator getOperator(Expression expr) {
case NEQ -> RangeIndex.Operator.NE;
default -> operator;
};
} else if (expr instanceof InternalFunctionCall fcall) {
Function function = fcall.getFunction();
if (function instanceof Lookup && function.isCalledAs("matches")) {
operator = RangeIndex.Operator.MATCH;
}
} else if (expr instanceof Lookup && ((Function)expr).isCalledAs("matches")) {
} else if (expr instanceof final InternalFunctionCall fcall) {
expr = fcall.getFunction();
}

if (expr instanceof final Lookup lookup && lookup.isCalledAs("matches")) {
operator = RangeIndex.Operator.MATCH;
}

return operator;
}

Expand Down
2 changes: 2 additions & 0 deletions extensions/indexes/range/src/test/xquery/range/optimizer.xql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,9 @@ function ot:optimize-matches-field($city as xs:string) {
};

declare
%test:stats
%test:args("[rR]üssel.*")
%test:assertXPath("$result//stats:index[@type = 'new-range'][@optimization = 1]")
function ot:matches-field-filtered($city as xs:string) {
let $address := collection($ot:COLLECTION)//address
return
Expand Down
2 changes: 1 addition & 1 deletion extensions/indexes/range/src/test/xquery/range/range.xql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
:)
xquery version "3.0";

module namespace rt="http://exist-db.org/xquery/range/test";
module namespace rt="http://exist-db.org/xquery/range/test/range";

import module namespace test="http://exist-db.org/xquery/xqsuite" at "resource:org/exist/xquery/lib/xqsuite/xqsuite.xql";

Expand Down

0 comments on commit 4909edb

Please sign in to comment.