diff --git a/exist-core/src/main/java/org/exist/storage/NodePath.java b/exist-core/src/main/java/org/exist/storage/NodePath.java index 1565c18fcdc..4776fa7a7fd 100644 --- a/exist-core/src/main/java/org/exist/storage/NodePath.java +++ b/exist-core/src/main/java/org/exist/storage/NodePath.java @@ -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(); diff --git a/extensions/indexes/range/pom.xml b/extensions/indexes/range/pom.xml index 907e03f0a14..938a39a440e 100644 --- a/extensions/indexes/range/pom.xml +++ b/extensions/indexes/range/pom.xml @@ -108,6 +108,12 @@ test + + com.google.code.findbugs + jsr305 + 3.0.2 + + diff --git a/extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/Lookup.java b/extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/Lookup.java index 721560240c6..f501290cc94 100644 --- a/extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/Lookup.java +++ b/extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/Lookup.java @@ -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; @@ -135,8 +135,8 @@ 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); } @@ -144,29 +144,61 @@ public static Lookup create(XQueryContext context, RangeIndex.Operator operator, 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 @@ -174,7 +206,7 @@ public void setFallback(Expression expression, int optimizeAxis) { this.axis = optimizeAxis; } - public Expression getFallback() { + public @Nullable Expression getFallback() { return fallback; } diff --git a/extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/RangeQueryRewriter.java b/extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/RangeQueryRewriter.java index b9b050690e4..890bb378ea8 100644 --- a/extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/RangeQueryRewriter.java +++ b/extensions/indexes/range/src/main/java/org/exist/xquery/modules/range/RangeQueryRewriter.java @@ -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; /** @@ -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 steps = getStepsToOptimize(innerExpr); + final Expression innerExpr = pred.getExpression(0); + final List steps = getStepsToOptimize(innerExpr); if (steps == null || steps.isEmpty()) { // no optimizable steps found continue; @@ -90,11 +91,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 { @@ -103,14 +104,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()); + 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; } } @@ -126,30 +137,23 @@ public Pragma rewriteLocationStep(final LocationStep locationStep) throws XPathE return null; } - protected static Lookup rewrite(Expression expression, NodePath path) throws XPathException { - ArrayList 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 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; } @@ -161,6 +165,7 @@ protected static List getStepsToOptimize(Expression expr) { 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 { @@ -173,16 +178,20 @@ protected static List getStepsToOptimize(Expression expr) { } 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; + } 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: @@ -218,15 +227,14 @@ public static RangeIndex.Operator getOperator(Expression expr) { 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; } diff --git a/extensions/indexes/range/src/test/xquery/range/optimizer.xql b/extensions/indexes/range/src/test/xquery/range/optimizer.xql index cac8d3c8984..a2b00c44d34 100644 --- a/extensions/indexes/range/src/test/xquery/range/optimizer.xql +++ b/extensions/indexes/range/src/test/xquery/range/optimizer.xql @@ -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 diff --git a/extensions/indexes/range/src/test/xquery/range/range.xql b/extensions/indexes/range/src/test/xquery/range/range.xql index 4ae00de3ee1..beb3062801f 100644 --- a/extensions/indexes/range/src/test/xquery/range/range.xql +++ b/extensions/indexes/range/src/test/xquery/range/range.xql @@ -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";