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

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(DummyQueryIndex.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(DummyQueryIndex.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,14 @@ 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]"));
     }
+
+    interface DummyQueryIndex extends QueryIndex, QueryIndex.AdvancedQueryIndex {
+        /*
+         * This is needed as AdvancedQueryIndex is what we what to mock but the QueryIndexProvider
+         * returns QueryIndex instances and, in reality implementation of AdvancedQueryIndex also implement
+         * QueryIndex, but AdvancedQueryIndex does not explicitly extend QueryIndex.
+         *
+         * Making this link explicit would break the spi versioning.
+         */
+    }
 }

oak-jcr/src/test/java/org/apache/jackrabbit/oak/IndexCostEvaluationTest.java

@@ -17,8 +17,8 @@
 package org.apache.jackrabbit.oak;
 
 import ch.qos.logback.classic.Level;
-import org.apache.jackrabbit.api.JackrabbitRepository;
-import org.apache.jackrabbit.api.JackrabbitSession;
+import org.apache.jackrabbit.oak.api.ContentRepository;
+import org.apache.jackrabbit.oak.api.ContentSession;
 import org.apache.jackrabbit.oak.commons.junit.LogCustomizer;
 import org.apache.jackrabbit.oak.jcr.Jcr;
 import org.apache.jackrabbit.oak.plugins.index.property.PropertyIndexPlan;
@@ -28,6 +28,7 @@
 import org.apache.jackrabbit.oak.spi.query.Filter;
 import org.apache.jackrabbit.oak.spi.query.QueryIndex;
 import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.security.OpenSecurityProvider;
 import org.apache.jackrabbit.oak.spi.state.NodeState;
 import org.jetbrains.annotations.NotNull;
 import org.junit.After;
@@ -36,66 +37,61 @@
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 
-import javax.jcr.Node;
-import javax.jcr.Repository;
-import javax.jcr.SimpleCredentials;
 import java.io.File;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
 
+import static javax.jcr.query.Query.JCR_SQL2;
 import static org.junit.Assert.assertTrue;
 
 public class IndexCostEvaluationTest {
     @Rule
     public TemporaryFolder temporaryFolder = new TemporaryFolder(new File("target"));
 
-    private static final String TEST_USER_NAME = "testUserName";
-
-    private Repository repository = null;
-    private JackrabbitSession session = null;
-    private Node root = null;
-    private LogCustomizer custom;
+    private ContentSession session = null;
+    private LogCustomizer logCollector;
 
     @Before
     public void before() throws Exception {
-        custom = LogCustomizer
+        logCollector = LogCustomizer
                 .forLogger(
                         "org.apache.jackrabbit.oak.query.QueryImpl")
                 .enable(Level.DEBUG).create();
-        custom.starting();
-
-
-        TestIndexProvider testProvider = new TestIndexProvider();
-        TestIndexProvider2 testProvider2 = new TestIndexProvider2();
-        TestIndexProvider3 testProvider3 = new TestIndexProvider3();
-
-        Jcr jcr = new Jcr()
-                .with((QueryIndexProvider) testProvider)
-                .with((QueryIndexProvider) testProvider2)
-                .with((QueryIndexProvider) testProvider3);
-
-        repository = jcr.createRepository();
-        session = (JackrabbitSession) repository.login(new SimpleCredentials("admin", "admin".toCharArray()));
-        root = session.getRootNode();
+        logCollector.starting();
+        double luceneMinCost = 2.2;
+        double elasticMinCost = 2.1;
+        TestIndexProvider testProvider = new TestIndexProvider("test-index", luceneMinCost);
+        TestIndexProvider testProvider2 = new TestIndexProvider("test-index2", luceneMinCost);
+        TestIndexProvider testProvider3 = new TestIndexProvider("test-index3", elasticMinCost);
+
+        Jcr jcr = new Jcr(new Oak(), false)
+                .with(new OpenSecurityProvider())
+                .with(new InitialContent())
+                .with(testProvider)
+                .with(testProvider2)
+                .with(testProvider3);
+
+        ContentRepository repository = jcr.createContentRepository();
+        session = repository.login(null, null);
     }
 
-
     @After
-    public void after() {
-        custom.finished();
-        session.logout();
-        if (repository instanceof JackrabbitRepository) {
-            ((JackrabbitRepository) repository).shutdown();
-        }
+    public void after() throws IOException {
+        session.close();
+        logCollector.finished();
     }
 
     // In cases where two indexes have same min cost i.e. both indexes are on par, we don't skip cost evaluation
     // even of cost from previous index is less than min cost of new index.
     @Test
     public void costEvaluationTest() throws Exception {
+        String query = "SELECT * FROM [rep:Authorizable] WHERE [rep:principalName] = 'anonymous'";
+        session.getLatestRoot().getQueryEngine().executeQuery(query, JCR_SQL2, 1, 0, null, null);
+
         boolean evaluationContinueLogPresent = false;
         boolean evaluationSkipLogPresent = false;
-        for (String log : custom.getLogs()) {
+        for (String log : logCollector.getLogs()) {
             if (log.equals("minCost: 2.1 of index :test-index2 > best Cost: 2.0 from index: test-index, but both indexes have same minimum cost - cost evaluation will continue")) {
                 evaluationContinueLogPresent = true;
             }
@@ -107,44 +103,32 @@ public void costEvaluationTest() throws Exception {
         assertTrue(evaluationSkipLogPresent);
     }
 
-    private class TestIndexProvider implements QueryIndexProvider {
-        TestIndex index = new TestIndex();
+    private static class TestIndexProvider implements QueryIndexProvider {
+        private final TestIndex index;
+
+        public TestIndexProvider(String indexName, double minimumCost) {
+            this.index = new TestIndex(indexName, minimumCost);
+        }
 
         @Override
         public @NotNull List<? extends QueryIndex> getQueryIndexes(NodeState nodeState) {
             return List.of(index);
         }
     }
 
-    private class TestIndexProvider2 extends TestIndexProvider {
-        public TestIndexProvider2() {
-            this.index = new TestIndex() {
-                public String getIndexName() {
-                    return "test-index2";
-                }
-            };
-        }
-    }
+    private static class TestIndex implements QueryIndex, QueryIndex.AdvancedQueryIndex {
 
-    private class TestIndexProvider3 extends TestIndexProvider {
-        public TestIndexProvider3() {
-            this.index = new TestIndex() {
-                public String getIndexName() {
-                    return "test-index3";
-                }
-
-                public double getMinimumCost() {
-                    return PropertyIndexPlan.COST_OVERHEAD + 0.11;
-                }
-            };
-        }
-    }
+        private final String name;
+        private final double minimumCost;
 
-    private class TestIndex implements QueryIndex, QueryIndex.AdvancedQueryIndex {
+        public TestIndex(String indexName, double minimumCost) {
+            this.name = indexName;
+            this.minimumCost = minimumCost;
+        }
 
         @Override
         public double getMinimumCost() {
-            return PropertyIndexPlan.COST_OVERHEAD + 0.1;
+            return minimumCost;
         }
 
         @Override
@@ -164,15 +148,15 @@ public String getPlan(Filter filter, NodeState rootState) {
 
         @Override
         public String getIndexName() {
-            return "test-index";
+            return name;
         }
 
         @Override
         public List<IndexPlan> getPlans(Filter filter, List<OrderEntry> sortOrder, NodeState rootState) {
             IndexPlan.Builder b = new IndexPlan.Builder();
             Filter f = new FilterImpl(null, "SELECT * FROM [nt:file]", new QueryEngineSettings());
             IndexPlan plan1 = b.setEstimatedEntryCount(10).setPlanName("testIndexPlan1").setFilter(f).build();
-            List<IndexPlan> indexList = new ArrayList<IndexPlan>();
+            List<IndexPlan> indexList = new ArrayList<>();
 
             indexList.add(plan1);
             return indexList;

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;
 
 /**
@@ -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;