OAK-12057 - Select same plan with or without LIMIT option

Author: bhabegger

oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java

@@ -1081,8 +1081,6 @@ private SelectorExecutionPlan getBestSelectorExecutionPlan(
         double almostBestCost = Double.POSITIVE_INFINITY;
         IndexPlan almostBestPlan = null;
 
-        long maxEntryCount = saturatedAdd(offset.orElse(0L), limit.orElse(Long.MAX_VALUE));
-
         // Sort the indexes according to their minimum cost to be able to skip the remaining indexes if the cost of the
         // current index is below the minimum cost of the next index.
         List<? extends QueryIndex> queryIndexes = indexProvider.getQueryIndexes(rootState).stream()
@@ -1117,12 +1115,6 @@ private SelectorExecutionPlan getBestSelectorExecutionPlan(
                     if (p.getSupportsPathRestriction()) {
                         entryCount = scaleEntryCount(rootState, filter, entryCount);
                     }
-                    if (sortOrder == null || p.getSortOrder() != null) {
-                        // if the query is unordered, or
-                        // if the query contains "order by" and the index can sort on that,
-                        // then we don't need to read all entries from the index
-                        entryCount = Math.min(maxEntryCount, entryCount);
-                    }
                     double c = p.getCostPerExecution() + entryCount * p.getCostPerEntry();
 
                     if (LOG.isDebugEnabled()) {

oak-core/src/test/java/org/apache/jackrabbit/oak/query/SQL2OptimiseQueryTest.java

@@ -36,8 +36,6 @@
 
 import java.text.ParseException;
 import java.util.List;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 
 import javax.jcr.RepositoryException;
 
@@ -88,7 +86,7 @@ public void limitUnionSize() throws ParseException {
                 + "  AND ((CONTAINS(*, '10') AND ([jcr:uuid] LIKE '11' OR [jcr:uuid] LIKE '12'))\n"
                 + "       OR (CONTAINS(*, '13') AND ([jcr:uuid] LIKE '14' OR [jcr:uuid] LIKE '15')))";
         SQL2Parser parser = SQL2ParserTest.createTestSQL2Parser(
-                getMappings(), getNodeTypes(), qeSettings);
+                getMappings(), getNodeTypes());
         Query original;
         original = parser.parse(query, false);
         assertNotNull(original);
@@ -219,7 +217,7 @@ private static Tree addChildWithProperty(@NotNull Tree father, @NotNull String n
     @Test
     public void optimise() throws ParseException {
         SQL2Parser parser = SQL2ParserTest.createTestSQL2Parser(
-                getMappings(), getNodeTypes(), qeSettings);
+                getMappings(), getNodeTypes());
         String statement;
         Query original, optimised;
 
@@ -294,7 +292,7 @@ public void optimiseAndOrAnd() throws ParseException {
 
     private void optimiseAndOrAnd(String statement, String expected) throws ParseException {
         SQL2Parser parser = SQL2ParserTest.createTestSQL2Parser(
-                getMappings(), getNodeTypes(), qeSettings);
+                getMappings(), getNodeTypes());
         Query original;
         original = parser.parse(statement, false);
         assertNotNull(original);
@@ -310,7 +308,7 @@ private void optimiseAndOrAnd(String statement, String expected) throws ParseExc
     @Test
     public void optimizeKeepsQueryOptions() throws ParseException {
         SQL2Parser parser = SQL2ParserTest.createTestSQL2Parser(
-                getMappings(), getNodeTypes(), qeSettings);
+                getMappings(), getNodeTypes());
         Query original;
         String statement = "select * from [nt:unstructured] as [c] " + 
                 "where [a]=1 or [b]=2 option(index tag x)";

oak-core/src/test/java/org/apache/jackrabbit/oak/query/SQL2ParserTest.java

@@ -20,13 +20,22 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.mock;
 
 import java.text.ParseException;
+import java.util.List;
 
 import org.apache.jackrabbit.oak.namepath.NamePathMapper;
 import org.apache.jackrabbit.oak.query.ast.NodeTypeInfoProvider;
 import org.apache.jackrabbit.oak.query.stats.QueryStatsData;
 import org.apache.jackrabbit.oak.query.xpath.XPathToSQL2Converter;
+import org.apache.jackrabbit.oak.spi.query.QueryIndex;
+import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.state.NodeState;
+import org.jetbrains.annotations.NotNull;
 import org.junit.Ignore;
 import org.junit.Test;
 
@@ -40,14 +49,12 @@ public class SQL2ParserTest {
     private static final SQL2Parser p = createTestSQL2Parser();
 
     public static SQL2Parser createTestSQL2Parser() {
-        return createTestSQL2Parser(NamePathMapper.DEFAULT, nodeTypes, new QueryEngineSettings());
+        return createTestSQL2Parser(NamePathMapper.DEFAULT, nodeTypes);
     }
 
-    public static SQL2Parser createTestSQL2Parser(NamePathMapper mappings, NodeTypeInfoProvider nodeTypes2,
-            QueryEngineSettings qeSettings) {
+    public static SQL2Parser createTestSQL2Parser(NamePathMapper mappings, NodeTypeInfoProvider nodeTypes2) {
         QueryStatsData data = new QueryStatsData("", "");
-        return new SQL2Parser(mappings, nodeTypes2, new QueryEngineSettings(),
-                data.new QueryExecutionStats());
+        return new SQL2Parser(mappings, nodeTypes2, new QueryEngineSettings(), data.new QueryExecutionStats());
     }
 
 
@@ -87,6 +94,60 @@ public void testUnwrappedOr() throws ParseException {
         assertTrue(q.contains(token));
     }
 
+    /*
+     * When a query with LIMIT option, it should still select the index with the least entries
+     * as those might require being traversed during post-filtering.
+     *
+     * See OAK-12057
+     */
+    @Test
+    public void testPlanningWithLimit() throws ParseException {
+        // Given
+        var query = "SELECT * \n" +
+                "FROM [nt:base] AS s \n" +
+                "WHERE ISDESCENDANTNODE(s, '/content') AND s.[j:c]='/conf/wknd'\n" +
+                "OPTION (LIMIT 2)";
+
+        // - the first available option
+        var indexA = mock(QueryIndex.AdvancedQueryIndex.class);
+        var planA = mock(QueryIndex.IndexPlan.class);
+        given(indexA.getPlans(any(), any(), any())).willReturn(List.of(planA));
+        given(planA.getPlanName()).willReturn("planA");
+        given(planA.getEstimatedEntryCount()).willReturn(10000L);   // more entries
+        given(planA.getCostPerEntry()).willReturn(1.0);
+        given(planA.getCostPerExecution()).willReturn(100.0);
+
+        // - the better option
+        var indexB = mock(QueryIndex.AdvancedQueryIndex.class);
+        var planB = mock(QueryIndex.IndexPlan.class);
+        given(indexB.getPlans(any(), any(), any())).willReturn(List.of(planB));
+        given(planB.getPlanName()).willReturn("planB");
+        given(planB.getEstimatedEntryCount()).willReturn(100L);     // less entries
+        given(planB.getCostPerEntry()).willReturn(1.0);
+        given(planB.getCostPerExecution()).willReturn(100.0);
+        given(indexB.getPlanDescription(eq(planB), any())).willReturn("planB");
+
+        var indexProvider = new QueryIndexProvider() {
+            @Override
+            public @NotNull List<? extends QueryIndex> getQueryIndexes(NodeState nodeState) {
+                return List.of(indexA, indexB);
+            }
+        };
+
+        var context = mock(ExecutionContext.class);
+        given(context.getIndexProvider()).willReturn(indexProvider);
+
+        // When
+        var parsedQuery = p.parse(query,false);
+        parsedQuery.init();
+        parsedQuery.setExecutionContext(context);
+        parsedQuery.setTraversalEnabled(false);
+        parsedQuery.prepare();
+
+        // Then
+        assertEquals("[nt:base] as [s] /* planB */", parsedQuery.getPlan());
+    }
+
     @Test
     public void testCoalesce() throws ParseException {
         p.parse("SELECT * FROM [nt:base] WHERE COALESCE([j:c/m/d:t], [j:c/j:t])='a'");
@@ -188,4 +249,5 @@ public void orderingWithUnionOfNodetype() throws Exception {
         xpath = "//(element(*, type1) | element(*, type2))[@a='b' or @c='d'] order by @foo";
         assertTrue("Converted xpath " + xpath + "doesn't end with 'order by [foo]'", c.convert(xpath).endsWith("order by [foo]"));
     }
+
 }

oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryIndex.java

@@ -34,7 +34,6 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import static java.util.stream.Collectors.toMap;
 import static org.apache.jackrabbit.oak.spi.query.Filter.PropertyRestriction;
 
 /**
@@ -174,7 +173,7 @@ interface AdvanceFulltextQueryIndex extends FulltextQueryIndex, AdvancedQueryInd
      * (returning the rows in a specific order), and that can provide detailed
      * information about the cost.
      */
-    interface AdvancedQueryIndex {
+    interface AdvancedQueryIndex extends QueryIndex {
 
         /**
          * Return the possible index plans for the given filter and sort order.
@@ -373,9 +372,6 @@ default boolean logWarningForPathFilterMismatch() {
          * A builder for index plans.
          */
         class Builder {
-
-            private static Logger LOG = LoggerFactory.getLogger(QueryIndex.IndexPlan.Builder.class);
-
             protected double costPerExecution = 1.0;
             protected double costPerEntry = 1.0;
             protected long estimatedEntryCount = 1000000;

oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/package-info.java

@@ -18,7 +18,7 @@
 /**
  * This package contains oak query index related classes.
  */
-@Version("3.2.0")
+@Version("4.0.0")
 package org.apache.jackrabbit.oak.spi.query;
 
 import org.osgi.annotation.versioning.Version;