Skip to content
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.

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

Prevent unnecessary optimisation of already optimised Range Index expressions #4882

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion exist-core/src/main/java/org/exist/storage/NodePath.java
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,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
6 changes: 6 additions & 0 deletions extensions/indexes/range/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,12 @@
<scope>test</scope>
</dependency>

<dependency>
<groupId>com.google.code.findbugs</groupId>
<artifactId>jsr305</artifactId>
<version>3.0.2</version>
</dependency>

</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@
import org.exist.xquery.*;
import org.exist.xquery.value.*;

import javax.annotation.Nullable;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;

Expand Down Expand Up @@ -135,46 +135,78 @@
)
};

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();

Check warning on line 201 in extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/Lookup.java

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/Lookup.java#L201

Avoid reassigning parameters such as 'expression'
}
this.fallback = expression;
Comment on lines +200 to 203
Copy link
Member

Choose a reason for hiding this comment

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

@marmoure you may fix this Codacy issue by using a if /else block:

if (expression instanceof final InternalFunctionCall fcall) {
    this.fallback  = fcall.getFunction();
} else {
    this.fallback = expression;
}  

Copy link
Member

Choose a reason for hiding this comment

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

Whilst the end result is the same. I am not sure this is the same intention, it was meant to be clear that we are replacing the expression. @reinhapa How strongly do you feel about this?

Copy link
Member

Choose a reason for hiding this comment

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

Well I would like to somehow bring the total qualtiy issues down when they pop up as I think we should try to avoid new issues if possible...

// 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
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 @@
// 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 @@ -90,11 +91,11 @@
}

// 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 @@ -103,14 +104,24 @@
}

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());
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));
}

pred.replace(innerExpr, new InternalFunctionCall(func));
canOptimize = true;
}
}
Expand All @@ -126,30 +137,23 @@
return null;
}

protected static Lookup rewrite(Expression expression, NodePath path) throws XPathException {
ArrayList<Expression> eqArgs = new ArrayList<>(2);
if (expression instanceof GeneralComparison) {
GeneralComparison comparison = (GeneralComparison) expression;
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);
}

return lookup;
} else if (expression instanceof InternalFunctionCall) {
InternalFunctionCall fcall = (InternalFunctionCall) expression;
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;
final InternalFunctionCall fcall = (InternalFunctionCall) expression;
final Function function = fcall.getFunction();
if (function instanceof final Lookup lookup && lookup.getContextPath() == null) {
lookup.setContextPath(path);
return lookup;
}
}

return null;
}

Expand All @@ -161,6 +165,7 @@
InternalFunctionCall fcall = (InternalFunctionCall) expr;
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 @@ -173,16 +178,20 @@
}

public static RangeIndex.Operator getOperator(Expression expr) {
if (expr instanceof InternalFunctionCall) {
InternalFunctionCall fcall = (InternalFunctionCall) expr;
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;

Check warning on line 186 in extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/RangeQueryRewriter.java

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/RangeQueryRewriter.java#L186

Avoid reassigning parameters such as 'expr'
} else {
expr = lookup;
}
}
}

RangeIndex.Operator operator = RangeIndex.Operator.EQ;
if (expr instanceof GeneralComparison) {
GeneralComparison comparison = (GeneralComparison) expr;
if (expr instanceof final GeneralComparison comparison) {
final Comparison relation = comparison.getRelation();
switch(relation) {
case LT:
Expand Down Expand Up @@ -218,15 +227,14 @@
break;

}
} else if (expr instanceof InternalFunctionCall) {
InternalFunctionCall fcall = (InternalFunctionCall) expr;
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
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
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
Loading