From 1893ff0e971272ce51ce761c06f597cdf6687665 Mon Sep 17 00:00:00 2001 From: Benjamin Habegger Date: Wed, 12 Aug 2026 08:45:48 +0200 Subject: [PATCH 1/6] OAK-12348: Property and node type indexes support costPerEntry/costPerExecution overrides PropertyIndex/NodeTypeIndex (oak-core) computed cost purely from indexed entry counts, with a fixed overhead of 2, and no way to influence the estimate from the index definition -- unlike Lucene/Elastic indexes (oak-search), which already read costPerEntry/costPerExecution. In production this caused the nodeType/property index to win cost comparisons against a more selective, purpose-built index for the same query (three related incidents), because the built-in entry-count estimate can be significantly wrong at scale. PropertyIndexPlan and PropertyIndexLookup now each split into getCostLegacy() (the original hardcoded formula), getCostConfigurable() (cost = costPerExecution + costPerEntry * entryCount, both optionally set on the property index definition), and getCost() which dispatches between them based on FT_OAK-12348 (enabled by default: with no properties set, getCostConfigurable() reproduces getCostLegacy() exactly, so this is behavior-preserving for every existing index definition; the toggle is an escape hatch, not an opt-in gate). NodeTypeIndex needs no changes at all -- its cost is the sum of two PropertyIndexLookup.getCost() calls (jcr:primaryType, jcr:mixinTypes), so it picks up the override transitively. IndexUtils gains a small public getOptionalValue(NodeState, String, double) helper (mirroring oak-search's IndexDefinition.getOptionalValue, which oak-core cannot depend on directly) used by both getCostConfigurable methods instead of duplicating the same property read twice. --- .../oak/plugins/index/IndexConstants.java | 15 ++ .../oak/plugins/index/IndexUtils.java | 18 ++ .../index/property/PropertyIndexLookup.java | 60 ++++++ .../index/property/PropertyIndexPlan.java | 60 ++++-- .../index/nodetype/NodeTypeIndexTest.java | 52 ++++++ .../index/property/PropertyIndexTest.java | 176 ++++++++++++++++++ .../src/site/markdown/query/property-index.md | 23 ++- 7 files changed, 389 insertions(+), 15 deletions(-) diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexConstants.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexConstants.java index fe8abdb8207..6ec6de71a72 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexConstants.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexConstants.java @@ -56,6 +56,21 @@ public interface IndexConstants { String ENTRY_COUNT_PROPERTY_NAME = "entryCount"; String KEY_COUNT_PROPERTY_NAME = "keyCount"; + + /** + * Multiplier (Double) applied to the estimated entry count when computing this + * index's query cost. Optional; defaults to 1.0 (no change to the estimate). + * Same property name as oak-search's FulltextIndexConstants#COST_PER_ENTRY, + * used for the same purpose on property/node type indexes. + */ + String COST_PER_ENTRY = "costPerEntry"; + + /** + * Fixed cost (Double) added once per query plan that uses this index. Optional; + * defaults to the index's built-in overhead. Same property name as + * oak-search's FulltextIndexConstants#COST_PER_EXECUTION. + */ + String COST_PER_EXECUTION = "costPerExecution"; /** * The regular expression pattern of the values to be indexes. diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUtils.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUtils.java index 57f14a3b901..924a50cf97a 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUtils.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/IndexUtils.java @@ -256,6 +256,24 @@ public static String getAsyncLaneName(NodeState idxState, String indexPath, Prop return null; } + /** + * Reads an optional double-valued property from an index definition node, + * falling back to a default when the property is absent. Mirrors the + * {@code getOptionalValue} helper oak-search's {@code IndexDefinition} uses + * for the same purpose ({@code costPerEntry}, {@code costPerExecution}, etc.) + * so index types outside oak-search (which cannot depend on it) can read + * the same kind of tunable index-definition properties. + * + * @param defn the index definition node + * @param propertyName the property name + * @param defaultValue the value to return if the property is not set + * @return the property's double value, or {@code defaultValue} + */ + public static double getOptionalValue(NodeState defn, String propertyName, double defaultValue) { + PropertyState ps = defn.getProperty(propertyName); + return ps != null ? ps.getValue(Type.DOUBLE) : defaultValue; + } + /** * Retrieves the calling class and method from the call stack; this is determined by unwinding * the stack until it finds a combination of full qualified classname + method (separated by ".") which diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java index 2d86dbf81fa..a8fd30389f3 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java @@ -28,6 +28,7 @@ import java.util.Collections; import java.util.List; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import org.apache.jackrabbit.oak.api.PropertyState; import org.apache.jackrabbit.oak.api.PropertyValue; @@ -35,6 +36,7 @@ import org.apache.jackrabbit.oak.commons.PathUtils; import org.apache.jackrabbit.oak.commons.collections.IterableUtils; import org.apache.jackrabbit.oak.plugins.index.IndexConstants; +import org.apache.jackrabbit.oak.plugins.index.IndexUtils; import org.apache.jackrabbit.oak.plugins.index.property.strategy.IndexStoreStrategy; import org.apache.jackrabbit.oak.spi.mount.MountInfoProvider; import org.apache.jackrabbit.oak.spi.mount.Mounts; @@ -75,6 +77,24 @@ public class PropertyIndexLookup { */ static final int MAX_COST = 100; + /** + * Feature toggle name for the configurable costPerEntry/costPerExecution + * cost formula (OAK-12348). + */ + public static final String FT_OAK_12348 = "FT_OAK-12348"; + + /** + * When {@code true} (the default), {@link #getCost} reads {@code costPerEntry}/ + * {@code costPerExecution} from the index definition ({@link #getCostConfigurable}). + * When {@code false}, {@link #getCost} uses the original hardcoded formula + * ({@link #getCostLegacy}) unconditionally, ignoring those properties even if + * set. Enabled by default: the new formula reproduces the legacy one exactly + * whenever {@code costPerEntry}/{@code costPerExecution} are absent, so this is + * a behavior-preserving default for anyone not using the new properties -- the + * toggle exists as an escape hatch, not as an opt-in gate. + */ + public static final AtomicBoolean FT_OAK_12348_ENABLE = new AtomicBoolean(true); + private final NodeState root; private final MountInfoProvider mountInfoProvider; @@ -135,7 +155,22 @@ Set getStrategies(NodeState definition) { definition, INDEX_CONTENT_NODE_NAME); } + /** + * Dispatches to {@link #getCostConfigurable} or {@link #getCostLegacy} + * depending on {@link #FT_OAK_12348_ENABLE}. + */ public double getCost(Filter filter, String propertyName, PropertyValue value) { + return FT_OAK_12348_ENABLE.get() + ? getCostConfigurable(filter, propertyName, value) + : getCostLegacy(filter, propertyName, value); + } + + /** + * Original cost formula: {@code COST_OVERHEAD + entryCount}. Ignores + * {@code costPerEntry}/{@code costPerExecution} even if set on the index + * definition. + */ + public double getCostLegacy(Filter filter, String propertyName, PropertyValue value) { NodeState indexMeta = getIndexNode(root, propertyName, filter); if (indexMeta == null) { return Double.POSITIVE_INFINITY; @@ -149,6 +184,31 @@ public double getCost(Filter filter, String propertyName, PropertyValue value) { return cost; } + /** + * {@code cost = costPerExecution + costPerEntry * entryCount}, both + * optionally configured on the index definition (OAK-12348). Defaults + * ({@code costPerEntry=1.0}, {@code costPerExecution=COST_OVERHEAD}) + * reproduce {@link #getCostLegacy} exactly. + */ + public double getCostConfigurable(Filter filter, String propertyName, PropertyValue value) { + NodeState indexMeta = getIndexNode(root, propertyName, filter); + if (indexMeta == null) { + return Double.POSITIVE_INFINITY; + } + Set strategies = getStrategies(indexMeta); + if (strategies.isEmpty()) { + return MAX_COST; + } + ValuePattern pattern = new ValuePattern(indexMeta); + double entryCount = 0; + for (IndexStoreStrategy s : strategies) { + entryCount += s.count(filter, root, indexMeta, encode(value, pattern), MAX_COST); + } + double costPerEntry = IndexUtils.getOptionalValue(indexMeta, IndexConstants.COST_PER_ENTRY, 1.0); + double costPerExecution = IndexUtils.getOptionalValue(indexMeta, IndexConstants.COST_PER_EXECUTION, COST_OVERHEAD); + return costPerExecution + costPerEntry * entryCount; + } + /** * Get the node with the index definition for the given property, if there * is an applicable index with data. diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexPlan.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexPlan.java index ca0e0efc100..ead82f779ba 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexPlan.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexPlan.java @@ -77,7 +77,13 @@ public class PropertyIndexPlan { private boolean matchesNodeTypes; - private final double cost; + /** + * The number of matching entries for the best-matching property found by + * the constructor's search loop, or {@code Double.POSITIVE_INFINITY} if + * none matched. {@link #getCostLegacy} and {@link #getCostConfigurable} + * both derive the final cost from this count. + */ + private final double bestCount; private final Set values; @@ -113,7 +119,7 @@ public class PropertyIndexPlan { ValuePattern valuePattern = new ValuePattern(definition); - double bestCost = Double.POSITIVE_INFINITY; + double bestCount = Double.POSITIVE_INFINITY; Set bestValues = emptySet(); int bestDepth = 1; @@ -168,22 +174,22 @@ public class PropertyIndexPlan { } } values = PropertyIndexUtil.encode(values); - double cost = strategies.isEmpty() ? MAX_COST : 0; + double count = strategies.isEmpty() ? MAX_COST : 0; for (IndexStoreStrategy strategy : strategies) { - cost += strategy.count(filter, root, definition, + count += strategy.count(filter, root, definition, values, MAX_COST); } - if (unique && cost <= 1) { + if (unique && count <= 1) { // for unique index, for the normal case // (that is, for a regular lookup) // no further reads are needed - cost = 0; + count = 0; } - if (cost < bestCost) { + if (count < bestCount) { bestDepth = depth; bestValues = values; - bestCost = cost; - if (bestCost == 0) { + bestCount = count; + if (bestCount == 0) { // shortcut: not possible to top this break; } @@ -194,15 +200,45 @@ public class PropertyIndexPlan { this.depth = bestDepth; this.values = bestValues; - this.cost = COST_OVERHEAD + bestCost; + this.bestCount = bestCount; } String getName() { return name; } + /** + * Dispatches to {@link #getCostConfigurable} or {@link #getCostLegacy} + * depending on {@link PropertyIndexLookup#FT_OAK_12348_ENABLE}, evaluated + * once per plan (a new plan is built whenever the filter changes, so a + * toggle flip is picked up on the next query, not on this cached plan). + */ double getCost() { - return cost; + return PropertyIndexLookup.FT_OAK_12348_ENABLE.get() ? getCostConfigurable() : getCostLegacy(); + } + + /** + * Original cost formula: {@code COST_OVERHEAD + bestCount}. Ignores + * {@code costPerEntry}/{@code costPerExecution} even if set on the index + * definition. + */ + double getCostLegacy() { + return bestCount == Double.POSITIVE_INFINITY ? Double.POSITIVE_INFINITY : COST_OVERHEAD + bestCount; + } + + /** + * {@code cost = costPerExecution + costPerEntry * bestCount}, both + * optionally configured on the index definition (OAK-12348). Defaults + * ({@code costPerEntry=1.0}, {@code costPerExecution=COST_OVERHEAD}) + * reproduce {@link #getCostLegacy} exactly. + */ + double getCostConfigurable() { + if (bestCount == Double.POSITIVE_INFINITY) { + return Double.POSITIVE_INFINITY; + } + double costPerEntry = IndexUtils.getOptionalValue(definition, IndexConstants.COST_PER_ENTRY, 1.0); + double costPerExecution = IndexUtils.getOptionalValue(definition, IndexConstants.COST_PER_EXECUTION, COST_OVERHEAD); + return costPerExecution + costPerEntry * bestCount; } Cursor execute() { @@ -260,7 +296,7 @@ public String toString() { } } buffer.append("\n"); - buffer.append(" estimatedCost: ").append(cost).append("\n"); + buffer.append(" estimatedCost: ").append(getCost()).append("\n"); return buffer.toString(); } diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java index 78a8488f43e..f606d025b3f 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java @@ -17,6 +17,7 @@ package org.apache.jackrabbit.oak.plugins.index.nodetype; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.apache.jackrabbit.oak.InitialContentHelper.INITIAL_CONTENT; import java.io.ByteArrayInputStream; @@ -91,6 +92,57 @@ public void nodeType() throws Exception { checkCursor(index.query(filter, rootState), "/folder-1", "/folder-2", "/file-1"); } + /** + * NodeTypeIndex has no cost logic of its own: its cost is the sum of two + * {@code PropertyIndexLookup.getCost()} calls (jcr:primaryType, jcr:mixinTypes) + * against the single "nodetype" property index definition. Setting + * costPerEntry/costPerExecution on that definition must change the total, + * with no code changes needed in the nodetype package itself. + */ + @Test + public void nodeTypeCostOverride() throws Exception { + NodeBuilder root = store.getRoot().builder(); + + // remove "rep:security" as it interferes with tests + root.getChildNode("rep:security").remove(); + + NodeBuilder nodetypeIndex = root.getChildNode("oak:index").getChildNode("nodetype"); + // set "entryCount", so the node type index counts the nodes + // and the approximation is not used + nodetypeIndex.setProperty("entryCount", -1); + nodetypeIndex.setProperty(org.apache.jackrabbit.oak.plugins.index.IndexConstants.COST_PER_ENTRY, 3.0); + nodetypeIndex.setProperty(org.apache.jackrabbit.oak.plugins.index.IndexConstants.COST_PER_EXECUTION, 10.0); + + addFolder(root, "folder-1"); + addFolder(root, "folder-2"); + addFile(root, "file-1"); + + store.merge(root, new EditorHook(new IndexUpdateProvider( + new PropertyIndexEditorProvider())), CommitInfo.EMPTY); + + NodeState rootState = store.getRoot(); + NodeTypeIndex index = new NodeTypeIndex( + Mounts.defaultMountInfoProvider()); + FilterImpl filter; + + // NodeTypeIndex has no toggle of its own -- it inherits whichever formula + // PropertyIndexLookup.getCost() is currently dispatching to, which is + // FT_OAK_12348_ENABLE, on by default -- no opt-in needed here. + assertTrue("toggle must be on by default", + org.apache.jackrabbit.oak.plugins.index.property.PropertyIndexLookup.FT_OAK_12348_ENABLE.get()); + + // default (see nodeType() above) is 2*COST_OVERHEAD(2) + entrySum; + // with the override it is 2*costPerExecution + costPerEntry*entrySum + filter = createFilter(rootState, JcrConstants.NT_FOLDER); + assertEquals(2 * 10.0 + 3.0 * 2, index.getCost(filter, rootState), 0.0); + + filter = createFilter(rootState, JcrConstants.NT_FILE); + assertEquals(2 * 10.0 + 3.0 * 1, index.getCost(filter, rootState), 0.0); + + filter = createFilter(rootState, JcrConstants.NT_HIERARCHYNODE); + assertEquals(2 * 10.0 + 3.0 * 3, index.getCost(filter, rootState), 0.0); + } + private static FilterImpl createFilter(NodeState root, String nodeTypeName) { NodeTypeInfoProvider nodeTypes = new NodeStateNodeTypeInfoProvider(root); NodeTypeInfo type = nodeTypes.getNodeTypeInfo(nodeTypeName); diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java index a9a551c8d5b..b1c99be4a19 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java @@ -258,6 +258,182 @@ public void costMaxEstimation() throws Exception { cost < traversal); } + /** + * Default cost (no costPerEntry/costPerExecution set) must stay exactly + * COST_OVERHEAD + entryCount regardless of the FT_OAK_12348 toggle position, + * both via {@link PropertyIndexLookup#getCost} and via {@link PropertyIndex#getCost} + * (which goes through {@link PropertyIndexPlan}). The toggle is enabled by + * default, so this is what a fresh install sees with no properties set. + */ + @Test + public void costPerEntryAndCostPerExecutionDefaultUnchanged() throws Exception { + NodeState root = INITIAL_CONTENT; + + NodeBuilder builder = root.builder(); + createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), "foo", + true, false, Set.of("foo"), null) + .setProperty("entryCount", -1); + NodeState before = builder.getNodeState(); + + for (int i = 0; i < 5; i++) { + builder.child("n" + i).setProperty("foo", "x1"); + } + NodeState after = builder.getNodeState(); + NodeState indexed = HOOK.processCommit(before, after, CommitInfo.EMPTY); + + FilterImpl f = createFilter(indexed, NT_BASE); + f.restrictPropertyAsList("foo", java.util.List.of(PropertyValues.newString("x1"))); + PropertyIndexLookup lookup = new PropertyIndexLookup(indexed); + PropertyIndex pIndex = new PropertyIndex(Mounts.defaultMountInfoProvider()); + + assertTrue("toggle must be on by default", PropertyIndexLookup.FT_OAK_12348_ENABLE.get()); + assertEquals(7.0, lookup.getCost(f, "foo", PropertyValues.newString("x1")), 0.0); + assertEquals(7.0, pIndex.getCost(f, indexed), 0.0); + + PropertyIndexLookup.FT_OAK_12348_ENABLE.set(false); + try { + assertEquals(7.0, lookup.getCost(f, "foo", PropertyValues.newString("x1")), 0.0); + assertEquals(7.0, pIndex.getCost(f, indexed), 0.0); + } finally { + PropertyIndexLookup.FT_OAK_12348_ENABLE.set(true); + } + } + + /** + * Setting costPerEntry/costPerExecution on the index definition changes the + * cost by the documented formula, {@code cost = costPerExecution + costPerEntry * entryCount}, + * by default -- FT_OAK_12348 is enabled out of the box. Disabling it (the + * escape hatch) must fall back to the legacy value. + */ + @Test + public void costPerEntryAndCostPerExecutionOverride() throws Exception { + NodeState root = INITIAL_CONTENT; + + NodeBuilder builder = root.builder(); + createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), "foo", + true, false, Set.of("foo"), null) + .setProperty("entryCount", -1) + .setProperty(IndexConstants.COST_PER_ENTRY, 2.0) + .setProperty(IndexConstants.COST_PER_EXECUTION, 10.0); + NodeState before = builder.getNodeState(); + + for (int i = 0; i < 5; i++) { + builder.child("n" + i).setProperty("foo", "x1"); + } + NodeState after = builder.getNodeState(); + NodeState indexed = HOOK.processCommit(before, after, CommitInfo.EMPTY); + + FilterImpl f = createFilter(indexed, NT_BASE); + f.restrictPropertyAsList("foo", java.util.List.of(PropertyValues.newString("x1"))); + PropertyIndexLookup lookup = new PropertyIndexLookup(indexed); + PropertyIndex pIndex = new PropertyIndex(Mounts.defaultMountInfoProvider()); + + // toggle on (default): override takes effect immediately, no opt-in needed + // -- 10.0 + 2.0 * 5 == 20.0 (legacy would have been 2.0 + 5 == 7.0). + assertTrue("toggle must be on by default", PropertyIndexLookup.FT_OAK_12348_ENABLE.get()); + assertEquals(20.0, lookup.getCost(f, "foo", PropertyValues.newString("x1")), 0.0); + assertEquals(20.0, pIndex.getCost(f, indexed), 0.0); + // getCostLegacy() is callable directly regardless of the toggle, and still + // gives the old value -- proves the escape hatch's formula is intact. + assertEquals(7.0, lookup.getCostLegacy(f, "foo", PropertyValues.newString("x1")), 0.0); + + PropertyIndexLookup.FT_OAK_12348_ENABLE.set(false); + try { + assertEquals(7.0, lookup.getCost(f, "foo", PropertyValues.newString("x1")), 0.0); + assertEquals(7.0, pIndex.getCost(f, indexed), 0.0); + } finally { + PropertyIndexLookup.FT_OAK_12348_ENABLE.set(true); + } + } + + /** + * costPerEntry == 0 on an index that doesn't apply to the query must not turn + * POSITIVE_INFINITY into NaN (0 * Infinity == NaN in IEEE754) — an inapplicable + * index must never look "free". Exercises the default (enabled) toggle state. + */ + @Test + public void costPerEntryZeroDoesNotCorruptInfinityCost() throws Exception { + NodeState root = INITIAL_CONTENT; + + NodeBuilder builder = root.builder(); + createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), "foo", + true, false, Set.of("foo"), null) + .setProperty("entryCount", -1) + .setProperty(IndexConstants.COST_PER_ENTRY, 0.0) + .setProperty(IndexConstants.COST_PER_EXECUTION, 0.0); + NodeState before = builder.getNodeState(); + builder.child("n1").setProperty("foo", "x1"); + NodeState after = builder.getNodeState(); + NodeState indexed = HOOK.processCommit(before, after, CommitInfo.EMPTY); + + // filter has no restriction on "foo" (or any other indexed property) at all, + // so no candidate property matches and PropertyIndexPlan's bestCost stays + // POSITIVE_INFINITY internally. + FilterImpl f = createFilter(indexed, NT_BASE); + PropertyIndex pIndex = new PropertyIndex(Mounts.defaultMountInfoProvider()); + + assertTrue("toggle must be on by default", PropertyIndexLookup.FT_OAK_12348_ENABLE.get()); + double cost = pIndex.getCost(f, indexed); + assertFalse("cost must not be NaN", Double.isNaN(cost)); + assertEquals(Double.POSITIVE_INFINITY, cost, 0.0); + } + + /** + * The unique-index short circuit zeroes the raw per-property strategy count + * ({@code bestCost}) for a normal unique lookup — it never made the *final* + * cost 0 even before this change (default final cost was always + * {@code COST_OVERHEAD + 0 == COST_OVERHEAD}, i.e. 2.0, never 0.0). So the + * invariant an override must preserve is: the entry-count contribution stays + * zero (a huge costPerEntry must not blow up the cost of a unique lookup), + * while costPerExecution still applies as the flat cost of the lookup itself. + */ + @Test + public void uniqueIndexShortCircuitZeroesEntryCountContributionUnderOverride() throws Exception { + NodeState root = INITIAL_CONTENT; + + NodeBuilder builder = root.builder(); + createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), "uuidIndex", + true, true, Set.of("foo"), null) + .setProperty(IndexConstants.COST_PER_ENTRY, 1000.0) + .setProperty(IndexConstants.COST_PER_EXECUTION, 3.0); + NodeState before = builder.getNodeState(); + builder.child("n1").setProperty("foo", "x1"); + NodeState after = builder.getNodeState(); + NodeState indexed = HOOK.processCommit(before, after, CommitInfo.EMPTY); + + FilterImpl f = createFilter(indexed, NT_BASE); + f.restrictPropertyAsList("foo", java.util.List.of(PropertyValues.newString("x1"))); + PropertyIndex pIndex = new PropertyIndex(Mounts.defaultMountInfoProvider()); + + assertTrue("toggle must be on by default", PropertyIndexLookup.FT_OAK_12348_ENABLE.get()); + // 3.0 + 1000.0 * 0 == 3.0 -- the huge costPerEntry must not apply, since + // the short circuit means there is no per-entry contribution to multiply. + assertEquals(3.0, pIndex.getCost(f, indexed), 0.0); + } + + /** + * Same scenario without any override: default final cost for a unique + * short-circuited lookup is COST_OVERHEAD (2.0), not 0.0 -- documents the + * baseline the override test above is relative to. + */ + @Test + public void uniqueIndexShortCircuitDefaultCostIsOverheadNotZero() throws Exception { + NodeState root = INITIAL_CONTENT; + + NodeBuilder builder = root.builder(); + createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), "uuidIndex", + true, true, Set.of("foo"), null); + NodeState before = builder.getNodeState(); + builder.child("n1").setProperty("foo", "x1"); + NodeState after = builder.getNodeState(); + NodeState indexed = HOOK.processCommit(before, after, CommitInfo.EMPTY); + + FilterImpl f = createFilter(indexed, NT_BASE); + f.restrictPropertyAsList("foo", java.util.List.of(PropertyValues.newString("x1"))); + PropertyIndex pIndex = new PropertyIndex(Mounts.defaultMountInfoProvider()); + assertEquals(2.0, pIndex.getCost(f, indexed), 0.0); + } + @Test public void testPropertyLookup() throws Exception { NodeState root = INITIAL_CONTENT; diff --git a/oak-doc/src/site/markdown/query/property-index.md b/oak-doc/src/site/markdown/query/property-index.md index ebd6b3113fd..e3c16fcb097 100644 --- a/oak-doc/src/site/markdown/query/property-index.md +++ b/oak-doc/src/site/markdown/query/property-index.md @@ -82,6 +82,13 @@ Optionally you can specify: to override the cost estimation (a high key count means a lower cost and a low key count means a high cost when searching for specific keys; has no effect when searching for "is not null"). +* `costPerEntry` (Double): a multiplier applied to the estimated number of entries + when computing the cost (default `1.0`). Same property name and purpose as the + `costPerEntry` property already supported by `lucene`/`elastic` index definitions + (see [Lucene index](lucene.md)) — lets an admin correct a misestimated cost without + changing the query (OAK-12348). +* `costPerExecution` (Double): a fixed cost added once to the estimate (default `2.0`, + the same value as the built-in overhead described below) (OAK-12348). * `reindex` (Boolean): if set to `true`, the full content is re-indexed. This can take a long time, and is run synchronously with storing the index (except with an async index). See "Reindexing" below for details. @@ -168,9 +175,14 @@ The algorithm to calculate the estimated cost is roughly as follows (a bit simpl if the path filtering (`includedPaths` / `excludedPaths`) does not match the query. * For the nodetype index, the cost is the sum of the cost for the `jcr:primaryType` lookup (if the primary type is known), - plus the cost for the `jcr:mixinTypes` lookup (if that is known). -* Otherwise, the cost is based on the overhead (which is 2), - plus the estimated number of entries. + plus the cost for the `jcr:mixinTypes` lookup (if that is known). The nodetype index has + no cost logic of its own — set `costPerEntry`/`costPerExecution` (see above) on the + property index definitions for `jcr:primaryType` and/or `jcr:mixinTypes` to influence + nodetype index cost as well. +* Otherwise, the cost is based on the overhead (which is 2, or the configured + `costPerExecution`), + plus the estimated number of entries (scaled by the configured `costPerEntry`, + default `1.0`). * For an "x is not null" condition, the estimated number of entries is either the configured `entryCount` or, if not set, the @@ -188,6 +200,11 @@ The algorithm to calculate the estimated cost is roughly as follows (a bit simpl in that subtree versus the approximate number of entries in the repository, using approximation available via the `counter` index. +`costPerEntry`/`costPerExecution` are read by default (feature toggle `FT_OAK-12348`, +enabled out of the box, since with no properties set the formula above is unchanged) — +disable it only as an escape hatch if the new formula is ever suspected of causing a +regression; the pre-existing hardcoded formula remains fully intact and reachable. + For example, for a query with path restriction "/content/products/t-shirts" and property restriction "color = 'red'", if there is an index for the property "color", then the entry count approximation is read from the index. Let's say it is 10'000 for this value. From b8b8625a9fdbdf053310c4805839499904392554 Mon Sep 17 00:00:00 2001 From: Benjamin Habegger Date: Mon, 31 Aug 2026 17:12:05 +0200 Subject: [PATCH 2/6] OAK-12348: clarify why nodeTypeCostOverride removes rep:security Explains that rep:security's default authorizable store also extends nt:hierarchyNode, which would otherwise skew the NT_HIERARCHYNODE count assertion. --- .../oak/plugins/index/nodetype/NodeTypeIndexTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java index f606d025b3f..21e2953c6ff 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java @@ -103,7 +103,10 @@ public void nodeType() throws Exception { public void nodeTypeCostOverride() throws Exception { NodeBuilder root = store.getRoot().builder(); - // remove "rep:security" as it interferes with tests + // remove system security nodes to avoid skewing counts: rep:security's + // default authorizable store (rep:AuthorizableFolder, rep:User) also + // extends nt:hierarchyNode, so it would otherwise be counted by the + // NT_HIERARCHYNODE assertion below alongside the folders/file this test adds. root.getChildNode("rep:security").remove(); NodeBuilder nodetypeIndex = root.getChildNode("oak:index").getChildNode("nodetype"); From f6ec75bdb8442a4de75ac08255efd990a758ed4b Mon Sep 17 00:00:00 2001 From: Benjamin Habegger Date: Mon, 31 Aug 2026 17:34:10 +0200 Subject: [PATCH 3/6] OAK-12348: wire FT_OAK-12348 through the whiteboard Feature mechanism --- .../java/org/apache/jackrabbit/oak/Oak.java | 8 +++ .../index/property/PropertyIndexLookup.java | 23 +------- .../index/property/PropertyIndexPlan.java | 8 +-- .../oak/query/QueryEngineSettings.java | 15 +++++ .../index/nodetype/NodeTypeIndexTest.java | 8 +-- .../index/property/PropertyIndexTest.java | 57 +++++++++++-------- .../jackrabbit/oak/spi/query/QueryLimits.java | 13 +++++ .../oak/spi/query/package-info.java | 2 +- 8 files changed, 79 insertions(+), 55 deletions(-) diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java index 407e208384e..447f9bb4b2e 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java @@ -589,6 +589,10 @@ public Oak with(@NotNull Whiteboard whiteboard) { LOG.info("Registered ignore limit in index selection feature: " + QueryEngineSettings.FT_IGNORE_LIMIT_IN_INDEX_SELECTION); closer.register(ignoreLimitInIndexSelection); queryEngineSettings.setIgnoreLimitInIndexSelectionFeature(ignoreLimitInIndexSelection); + Feature costPerEntryLegacyModeFeature = newFeature(QueryEngineSettings.FT_OAK_12348, whiteboard); + LOG.info("Registered costPerEntry/costPerExecution legacy cost formula feature: " + QueryEngineSettings.FT_OAK_12348); + closer.register(costPerEntryLegacyModeFeature); + queryEngineSettings.setCostPerEntryLegacyModeFeature(costPerEntryLegacyModeFeature); } return this; @@ -1009,6 +1013,10 @@ public void setIgnoreLimitInIndexSelectionFeature(@Nullable Feature feature) { settings.setIgnoreLimitInIndexSelectionFeature(feature); } + public void setCostPerEntryLegacyModeFeature(@Nullable Feature feature) { + settings.setCostPerEntryLegacyModeFeature(feature); + } + @Override public void setQueryValidatorPattern(String key, String pattern, String comment, boolean failQuery) { settings.getQueryValidator().setPattern(key, pattern, comment, failQuery); diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java index a8fd30389f3..59490436b21 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java @@ -28,7 +28,6 @@ import java.util.Collections; import java.util.List; import java.util.Set; -import java.util.concurrent.atomic.AtomicBoolean; import org.apache.jackrabbit.oak.api.PropertyState; import org.apache.jackrabbit.oak.api.PropertyValue; @@ -77,24 +76,6 @@ public class PropertyIndexLookup { */ static final int MAX_COST = 100; - /** - * Feature toggle name for the configurable costPerEntry/costPerExecution - * cost formula (OAK-12348). - */ - public static final String FT_OAK_12348 = "FT_OAK-12348"; - - /** - * When {@code true} (the default), {@link #getCost} reads {@code costPerEntry}/ - * {@code costPerExecution} from the index definition ({@link #getCostConfigurable}). - * When {@code false}, {@link #getCost} uses the original hardcoded formula - * ({@link #getCostLegacy}) unconditionally, ignoring those properties even if - * set. Enabled by default: the new formula reproduces the legacy one exactly - * whenever {@code costPerEntry}/{@code costPerExecution} are absent, so this is - * a behavior-preserving default for anyone not using the new properties -- the - * toggle exists as an escape hatch, not as an opt-in gate. - */ - public static final AtomicBoolean FT_OAK_12348_ENABLE = new AtomicBoolean(true); - private final NodeState root; private final MountInfoProvider mountInfoProvider; @@ -157,10 +138,10 @@ Set getStrategies(NodeState definition) { /** * Dispatches to {@link #getCostConfigurable} or {@link #getCostLegacy} - * depending on {@link #FT_OAK_12348_ENABLE}. + * depending on {@link org.apache.jackrabbit.oak.spi.query.QueryLimits#isCostPerEntryOverrideEnabled}. */ public double getCost(Filter filter, String propertyName, PropertyValue value) { - return FT_OAK_12348_ENABLE.get() + return filter.getQueryLimits().isCostPerEntryOverrideEnabled() ? getCostConfigurable(filter, propertyName, value) : getCostLegacy(filter, propertyName, value); } diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexPlan.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexPlan.java index ead82f779ba..b44142c9848 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexPlan.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexPlan.java @@ -209,12 +209,12 @@ String getName() { /** * Dispatches to {@link #getCostConfigurable} or {@link #getCostLegacy} - * depending on {@link PropertyIndexLookup#FT_OAK_12348_ENABLE}, evaluated - * once per plan (a new plan is built whenever the filter changes, so a - * toggle flip is picked up on the next query, not on this cached plan). + * depending on {@link org.apache.jackrabbit.oak.spi.query.QueryLimits#isCostPerEntryOverrideEnabled}, + * evaluated once per plan (a new plan is built whenever the filter changes, + * so a toggle flip is picked up on the next query, not on this cached plan). */ double getCost() { - return PropertyIndexLookup.FT_OAK_12348_ENABLE.get() ? getCostConfigurable() : getCostLegacy(); + return filter.getQueryLimits().isCostPerEntryOverrideEnabled() ? getCostConfigurable() : getCostLegacy(); } /** diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java index 93d6eebda7f..7a4f062186d 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java @@ -67,6 +67,8 @@ public class QueryEngineSettings implements QueryEngineSettingsMBean, QueryLimit public static final String FT_IGNORE_LIMIT_IN_INDEX_SELECTION = "FT_OAK-12057"; + public static final String FT_OAK_12348 = "FT_OAK-12348"; + public static final int DEFAULT_PREFETCH_COUNT = Integer.getInteger(OAK_QUERY_PREFETCH_COUNT, -1); public static final String OAK_QUERY_FAIL_TRAVERSAL = "oak.queryFailTraversal"; @@ -125,6 +127,7 @@ public class QueryEngineSettings implements QueryEngineSettingsMBean, QueryLimit private Feature sortUnionQueryLegacyModeFeature; private Feature optimizeXPathUnion; private Feature ignoreLimitInIndexSelectionFeature; + private Feature costPerEntryLegacyModeFeature; private String autoOptionsMappingJson = "{}"; private QueryOptions.AutomaticQueryOptionsMapping autoOptionsMapping = new QueryOptions.AutomaticQueryOptionsMapping(autoOptionsMappingJson); @@ -257,6 +260,18 @@ public boolean isIgnoreLimitInIndexSelection() { return ignoreLimitInIndexSelectionFeature == null || ignoreLimitInIndexSelectionFeature.isEnabled(); } + public void setCostPerEntryLegacyModeFeature(@Nullable Feature feature) { + this.costPerEntryLegacyModeFeature = feature; + } + + @Override + public boolean isCostPerEntryOverrideEnabled() { + // Legacy (hardcoded) cost formula is disabled by default; the + // configurable costPerEntry/costPerExecution formula (OAK-12348) is + // the default behavior -- flipping this toggle on is the escape hatch. + return costPerEntryLegacyModeFeature == null || !costPerEntryLegacyModeFeature.isEnabled(); + } + public String getStrictPathRestriction() { return strictPathRestriction.name(); } diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java index 21e2953c6ff..82d044ff2b8 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java @@ -17,7 +17,6 @@ package org.apache.jackrabbit.oak.plugins.index.nodetype; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; import static org.apache.jackrabbit.oak.InitialContentHelper.INITIAL_CONTENT; import java.io.ByteArrayInputStream; @@ -129,10 +128,9 @@ public void nodeTypeCostOverride() throws Exception { FilterImpl filter; // NodeTypeIndex has no toggle of its own -- it inherits whichever formula - // PropertyIndexLookup.getCost() is currently dispatching to, which is - // FT_OAK_12348_ENABLE, on by default -- no opt-in needed here. - assertTrue("toggle must be on by default", - org.apache.jackrabbit.oak.plugins.index.property.PropertyIndexLookup.FT_OAK_12348_ENABLE.get()); + // PropertyIndexLookup.getCost() is currently dispatching to, driven by + // QueryEngineSettings.isCostPerEntryOverrideEnabled() (OAK-12348), which is + // enabled by default -- no opt-in needed here. // default (see nodeType() above) is 2*COST_OVERHEAD(2) + entrySum; // with the override it is 2*costPerExecution + costPerEntry*entrySum diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java index b1c99be4a19..e5b62b4c996 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java @@ -74,6 +74,7 @@ import org.apache.jackrabbit.oak.spi.query.Filter; import org.apache.jackrabbit.oak.spi.state.NodeBuilder; import org.apache.jackrabbit.oak.spi.state.NodeState; +import org.apache.jackrabbit.oak.spi.toggle.Feature; import org.apache.jackrabbit.oak.spi.toggle.FeatureToggle; import org.apache.sling.testing.mock.osgi.MockOsgi; import org.apache.sling.testing.mock.osgi.junit.OsgiContext; @@ -81,6 +82,7 @@ import org.junit.Assert; import org.junit.Rule; import org.junit.Test; +import org.mockito.Mockito; import org.slf4j.LoggerFactory; import ch.qos.logback.classic.Level; @@ -281,22 +283,21 @@ public void costPerEntryAndCostPerExecutionDefaultUnchanged() throws Exception { NodeState after = builder.getNodeState(); NodeState indexed = HOOK.processCommit(before, after, CommitInfo.EMPTY); + PropertyIndexLookup lookup = new PropertyIndexLookup(indexed); + FilterImpl f = createFilter(indexed, NT_BASE); f.restrictPropertyAsList("foo", java.util.List.of(PropertyValues.newString("x1"))); - PropertyIndexLookup lookup = new PropertyIndexLookup(indexed); PropertyIndex pIndex = new PropertyIndex(Mounts.defaultMountInfoProvider()); - - assertTrue("toggle must be on by default", PropertyIndexLookup.FT_OAK_12348_ENABLE.get()); assertEquals(7.0, lookup.getCost(f, "foo", PropertyValues.newString("x1")), 0.0); assertEquals(7.0, pIndex.getCost(f, indexed), 0.0); - PropertyIndexLookup.FT_OAK_12348_ENABLE.set(false); - try { - assertEquals(7.0, lookup.getCost(f, "foo", PropertyValues.newString("x1")), 0.0); - assertEquals(7.0, pIndex.getCost(f, indexed), 0.0); - } finally { - PropertyIndexLookup.FT_OAK_12348_ENABLE.set(true); - } + QueryEngineSettings legacySettings = new QueryEngineSettings(); + legacySettings.setCostPerEntryLegacyModeFeature(createLegacyModeFeature(true)); + FilterImpl legacyFilter = createFilter(indexed, NT_BASE, legacySettings); + legacyFilter.restrictPropertyAsList("foo", java.util.List.of(PropertyValues.newString("x1"))); + PropertyIndex pIndexLegacy = new PropertyIndex(Mounts.defaultMountInfoProvider()); + assertEquals(7.0, lookup.getCost(legacyFilter, "foo", PropertyValues.newString("x1")), 0.0); + assertEquals(7.0, pIndexLegacy.getCost(legacyFilter, indexed), 0.0); } /** @@ -323,27 +324,27 @@ public void costPerEntryAndCostPerExecutionOverride() throws Exception { NodeState after = builder.getNodeState(); NodeState indexed = HOOK.processCommit(before, after, CommitInfo.EMPTY); + PropertyIndexLookup lookup = new PropertyIndexLookup(indexed); + FilterImpl f = createFilter(indexed, NT_BASE); f.restrictPropertyAsList("foo", java.util.List.of(PropertyValues.newString("x1"))); - PropertyIndexLookup lookup = new PropertyIndexLookup(indexed); PropertyIndex pIndex = new PropertyIndex(Mounts.defaultMountInfoProvider()); - // toggle on (default): override takes effect immediately, no opt-in needed + // enabled (default): override takes effect immediately, no opt-in needed // -- 10.0 + 2.0 * 5 == 20.0 (legacy would have been 2.0 + 5 == 7.0). - assertTrue("toggle must be on by default", PropertyIndexLookup.FT_OAK_12348_ENABLE.get()); assertEquals(20.0, lookup.getCost(f, "foo", PropertyValues.newString("x1")), 0.0); assertEquals(20.0, pIndex.getCost(f, indexed), 0.0); // getCostLegacy() is callable directly regardless of the toggle, and still // gives the old value -- proves the escape hatch's formula is intact. assertEquals(7.0, lookup.getCostLegacy(f, "foo", PropertyValues.newString("x1")), 0.0); - PropertyIndexLookup.FT_OAK_12348_ENABLE.set(false); - try { - assertEquals(7.0, lookup.getCost(f, "foo", PropertyValues.newString("x1")), 0.0); - assertEquals(7.0, pIndex.getCost(f, indexed), 0.0); - } finally { - PropertyIndexLookup.FT_OAK_12348_ENABLE.set(true); - } + QueryEngineSettings legacySettings = new QueryEngineSettings(); + legacySettings.setCostPerEntryLegacyModeFeature(createLegacyModeFeature(true)); + FilterImpl legacyFilter = createFilter(indexed, NT_BASE, legacySettings); + legacyFilter.restrictPropertyAsList("foo", java.util.List.of(PropertyValues.newString("x1"))); + PropertyIndex pIndexLegacy = new PropertyIndex(Mounts.defaultMountInfoProvider()); + assertEquals(7.0, lookup.getCost(legacyFilter, "foo", PropertyValues.newString("x1")), 0.0); + assertEquals(7.0, pIndexLegacy.getCost(legacyFilter, indexed), 0.0); } /** @@ -372,7 +373,6 @@ public void costPerEntryZeroDoesNotCorruptInfinityCost() throws Exception { FilterImpl f = createFilter(indexed, NT_BASE); PropertyIndex pIndex = new PropertyIndex(Mounts.defaultMountInfoProvider()); - assertTrue("toggle must be on by default", PropertyIndexLookup.FT_OAK_12348_ENABLE.get()); double cost = pIndex.getCost(f, indexed); assertFalse("cost must not be NaN", Double.isNaN(cost)); assertEquals(Double.POSITIVE_INFINITY, cost, 0.0); @@ -405,7 +405,6 @@ public void uniqueIndexShortCircuitZeroesEntryCountContributionUnderOverride() t f.restrictPropertyAsList("foo", java.util.List.of(PropertyValues.newString("x1"))); PropertyIndex pIndex = new PropertyIndex(Mounts.defaultMountInfoProvider()); - assertTrue("toggle must be on by default", PropertyIndexLookup.FT_OAK_12348_ENABLE.get()); // 3.0 + 1000.0 * 0 == 3.0 -- the huge costPerEntry must not apply, since // the short circuit means there is no per-entry contribution to multiply. assertEquals(3.0, pIndex.getCost(f, indexed), 0.0); @@ -596,10 +595,20 @@ public void testCustomConfigNodeType() throws Exception { } private static FilterImpl createFilter(NodeState root, String nodeTypeName) { + return createFilter(root, nodeTypeName, new QueryEngineSettings()); + } + + private static FilterImpl createFilter(NodeState root, String nodeTypeName, QueryEngineSettings settings) { NodeTypeInfoProvider nodeTypes = new NodeStateNodeTypeInfoProvider(root); - NodeTypeInfo type = nodeTypes.getNodeTypeInfo(nodeTypeName); + NodeTypeInfo type = nodeTypes.getNodeTypeInfo(nodeTypeName); SelectorImpl selector = new SelectorImpl(type, nodeTypeName); - return new FilterImpl(selector, "SELECT * FROM [" + nodeTypeName + "]", new QueryEngineSettings()); + return new FilterImpl(selector, "SELECT * FROM [" + nodeTypeName + "]", settings); + } + + private static Feature createLegacyModeFeature(boolean enabled) { + Feature feature = Mockito.mock(Feature.class); + Mockito.when(feature.isEnabled()).thenReturn(enabled); + return feature; } /** diff --git a/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryLimits.java b/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryLimits.java index adb0f5c5f92..d598613114b 100644 --- a/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryLimits.java +++ b/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryLimits.java @@ -80,4 +80,17 @@ default boolean isIgnoreLimitInIndexSelection() { return true; } + /** + * See OAK-12348. By default, {@code PropertyIndex}/{@code NodeTypeIndex} + * read {@code costPerEntry}/{@code costPerExecution} from the index + * definition to compute cost. When {@code false}, the original hardcoded + * formula is used unconditionally, ignoring those properties even if set. + * + * @return true to use the configurable cost formula (the default), false + * for the legacy formula + */ + default boolean isCostPerEntryOverrideEnabled() { + return true; + } + } diff --git a/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/package-info.java b/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/package-info.java index 27d4cdb5a50..42ad5573dbc 100644 --- a/oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/package-info.java +++ b/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.3.0") +@Version("3.4.0") package org.apache.jackrabbit.oak.spi.query; import org.osgi.annotation.versioning.Version; From e845d81da422ca5a7f10be48fb26b8e458c41f95 Mon Sep 17 00:00:00 2001 From: Benjamin Habegger Date: Tue, 1 Sep 2026 07:59:59 +0200 Subject: [PATCH 4/6] OAK-12348: getMinimumCost() must not assume a floor above 0 --- .../plugins/index/nodetype/NodeTypeIndexLookup.java | 7 +++++-- .../oak/plugins/index/property/PropertyIndex.java | 7 ++++++- .../plugins/index/nodetype/NodeTypeIndexTest.java | 9 +++++++++ .../plugins/index/property/PropertyIndexTest.java | 12 ++++++++++++ 4 files changed, 32 insertions(+), 3 deletions(-) diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexLookup.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexLookup.java index fb5a9f8c21f..1b528fbff2b 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexLookup.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexLookup.java @@ -32,9 +32,12 @@ class NodeTypeIndexLookup implements JcrConstants { /** - * Derived from {@link #getCost(Filter)} + * Since OAK-12348, the underlying PropertyIndexLookup cost can be + * configured arbitrarily low via costPerExecution, and this constant has + * no Filter/NodeState to inspect the active index definitions -- so 0 is + * the only value that stays a sound lower bound in every configuration. */ - static final double MINIMUM_COST = 2.05; + static final double MINIMUM_COST = 0; private final NodeState root; diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndex.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndex.java index 5a54ed4ac8f..190a0fb71b3 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndex.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndex.java @@ -221,7 +221,12 @@ private static String[] getOptionalStrings(NodeState defn, String propertyName) @Override public double getMinimumCost() { - return PropertyIndexPlan.COST_OVERHEAD; + // Since OAK-12348, costPerExecution can be configured arbitrarily low + // (including 0) on any property index definition, and this method has + // no Filter/NodeState to inspect index definitions -- so COST_OVERHEAD + // is no longer a sound floor. 0 is the only value that stays a valid + // lower bound in every configuration. + return 0; } @Override diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java index 82d044ff2b8..cef423aa21f 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/nodetype/NodeTypeIndexTest.java @@ -144,6 +144,15 @@ public void nodeTypeCostOverride() throws Exception { assertEquals(2 * 10.0 + 3.0 * 3, index.getCost(filter, rootState), 0.0); } + /** + * Same rationale as PropertyIndex#getMinimumCost -- see PropertyIndexTest#getMinimumCostIsZero. + */ + @Test + public void getMinimumCostIsZero() { + NodeTypeIndex index = new NodeTypeIndex(Mounts.defaultMountInfoProvider()); + assertEquals(0.0, index.getMinimumCost(), 0.0); + } + private static FilterImpl createFilter(NodeState root, String nodeTypeName) { NodeTypeInfoProvider nodeTypes = new NodeStateNodeTypeInfoProvider(root); NodeTypeInfo type = nodeTypes.getNodeTypeInfo(nodeTypeName); diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java index e5b62b4c996..56850570a4d 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java @@ -433,6 +433,18 @@ public void uniqueIndexShortCircuitDefaultCostIsOverheadNotZero() throws Excepti assertEquals(2.0, pIndex.getCost(f, indexed), 0.0); } + /** + * getMinimumCost() takes no Filter/NodeState, so it cannot know whether any + * definition has overridden costPerExecution below the old hardcoded floor + * (OAK-12348) -- 0 is the only value that stays a sound lower bound in every + * configuration. + */ + @Test + public void getMinimumCostIsZero() { + PropertyIndex pIndex = new PropertyIndex(Mounts.defaultMountInfoProvider()); + assertEquals(0.0, pIndex.getMinimumCost(), 0.0); + } + @Test public void testPropertyLookup() throws Exception { NodeState root = INITIAL_CONTENT; From 92edc4cd0acac6acd12f561d773d45aee491df0f Mon Sep 17 00:00:00 2001 From: Benjamin Habegger Date: Tue, 1 Sep 2026 08:16:09 +0200 Subject: [PATCH 5/6] OAK-12348: createPlan's early-break must use the active formula's floor --- .../plugins/index/property/PropertyIndex.java | 13 ++++++- .../index/property/PropertyIndexTest.java | 38 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndex.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndex.java index 190a0fb71b3..532a678370a 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndex.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndex.java @@ -116,6 +116,15 @@ private static PropertyIndexPlan createPlan(NodeState root, Filter filter, MountInfoProvider mountInfoProvider) { PropertyIndexPlan bestPlan = null; + // The lowest cost any candidate plan can possibly report, given the + // formula currently in effect (OAK-12348): COST_OVERHEAD under the + // legacy formula (bestCount can't go below 0), or 0 under the + // configurable formula (costPerExecution can be set to 0 on any + // not-yet-scanned definition, so no positive floor is safe). Used + // below to stop scanning once that floor is hit. + double minimumPossibleCost = filter.getQueryLimits().isCostPerEntryOverrideEnabled() + ? 0 : PropertyIndexPlan.COST_OVERHEAD; + // TODO support indexes on a path // currently, only indexes on the root node are supported NodeState state = root.getChildNode(INDEX_DEFINITIONS_NAME); @@ -133,8 +142,8 @@ private static PropertyIndexPlan createPlan(NodeState root, Filter filter, plan.getName(), plan.getCost()); if (bestPlan == null || plan.getCost() < bestPlan.getCost()) { bestPlan = plan; - // Stop comparing if the costs are the minimum - if (plan.getCost() == PropertyIndexPlan.COST_OVERHEAD) { + // Stop comparing if the cost can't possibly be beaten + if (plan.getCost() == minimumPossibleCost) { break; } } diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java index 56850570a4d..6f33b46f685 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java @@ -433,6 +433,44 @@ public void uniqueIndexShortCircuitDefaultCostIsOverheadNotZero() throws Excepti assertEquals(2.0, pIndex.getCost(f, indexed), 0.0); } + /** + * Regression for the early-break in PropertyIndex#createPlan: under the + * configurable formula (OAK-12348), hitting the legacy COST_OVERHEAD (2.0) + * on one definition does not mean no other definition can be cheaper -- + * costPerExecution can be configured below 2.0. bIndex is unique (so its + * one matching entry short-circuits to bestCount=0, giving the *default* + * cost of exactly COST_OVERHEAD with no override needed) and is the entry + * createPlan() scans first for this pair of names (child node order here + * is hash-based, not alphabetical), reproducing the exact old break + * condition. aIndex overrides costPerExecution to 0.5 and is genuinely + * cheaper, but the old break would stop scanning right after bIndex and + * never see it. + */ + @Test + public void createPlanDoesNotBreakEarlyWhenCheaperOverrideFollows() throws Exception { + NodeState root = INITIAL_CONTENT; + + NodeBuilder builder = root.builder(); + createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), "bIndex", + true, true, Set.of("foo"), null); + createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), "aIndex", + true, false, Set.of("foo"), null) + .setProperty(IndexConstants.COST_PER_ENTRY, 0.0) + .setProperty(IndexConstants.COST_PER_EXECUTION, 0.5); + NodeState before = builder.getNodeState(); + + builder.child("n1").setProperty("foo", "x1"); + NodeState after = builder.getNodeState(); + NodeState indexed = HOOK.processCommit(before, after, CommitInfo.EMPTY); + + FilterImpl f = createFilter(indexed, NT_BASE); + f.restrictPropertyAsList("foo", java.util.List.of(PropertyValues.newString("x1"))); + PropertyIndex pIndex = new PropertyIndex(Mounts.defaultMountInfoProvider()); + + assertEquals("aIndex", pIndex.getIndexName(f, indexed)); + assertEquals(0.5, pIndex.getCost(f, indexed), 0.0); + } + /** * getMinimumCost() takes no Filter/NodeState, so it cannot know whether any * definition has overridden costPerExecution below the old hardcoded floor From ac0e350b3211f03a9d87dc22076eae480aeb707f Mon Sep 17 00:00:00 2001 From: Benjamin Habegger Date: Tue, 1 Sep 2026 08:29:32 +0200 Subject: [PATCH 6/6] OAK-12348: add legacy-mode regression for createPlan's break floor with multiple definitions --- .../index/property/PropertyIndexTest.java | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java index 6f33b46f685..c9d974494f7 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexTest.java @@ -471,6 +471,73 @@ public void createPlanDoesNotBreakEarlyWhenCheaperOverrideFollows() throws Excep assertEquals(0.5, pIndex.getCost(f, indexed), 0.0); } + /** + * Regression for the break condition's floor value under legacy mode, with + * two competing definitions present. Under the legacy formula, overrides + * are ignored entirely ({@code getCostLegacy()} = {@code COST_OVERHEAD + + * bestCount}, no override read at all), so the true minimum achievable + * legacy cost is always exactly {@code COST_OVERHEAD} -- a definition that + * hits it can never legitimately be beaten by another definition's legacy + * cost. That means the *winning* plan alone can't tell us which floor the + * break actually used: both the correct floor ({@code COST_OVERHEAD}) and + * a hypothetical reversed bug (a floor of 0 that never matches in legacy + * mode) produce the exact same winner here, since nothing can beat + * {@code COST_OVERHEAD} under legacy math regardless of the break. + *

+ * What *does* distinguish them is whether the loop keeps scanning after + * bIndex hits {@code COST_OVERHEAD}: {@code LOG.debug} in + * {@code createPlan()} logs every candidate's cost unconditionally, before + * the break check runs. If the break correctly fires right after bIndex, + * aIndex is never even constructed/evaluated and its line never reaches + * the log. If the floor were wrongly 0 in legacy mode, the break would + * never fire on bIndex's 2.0, the loop would keep going, and aIndex's cost + * would be logged too -- even though it still can't win. This test asserts + * on that log evidence, since the winning plan/cost by itself would pass + * either way. + */ + @Test + public void createPlanBreaksImmediatelyOnCostOverheadUnderLegacyModeWithMultipleDefinitions() throws Exception { + NodeState root = INITIAL_CONTENT; + + NodeBuilder builder = root.builder(); + createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), "bIndex", + true, true, Set.of("foo"), null); + createIndexDefinition(builder.child(INDEX_DEFINITIONS_NAME), "aIndex", + true, false, Set.of("foo"), null) + .setProperty(IndexConstants.COST_PER_ENTRY, 0.0) + .setProperty(IndexConstants.COST_PER_EXECUTION, 0.5); + NodeState before = builder.getNodeState(); + + builder.child("n1").setProperty("foo", "x1"); + NodeState after = builder.getNodeState(); + NodeState indexed = HOOK.processCommit(before, after, CommitInfo.EMPTY); + + QueryEngineSettings legacySettings = new QueryEngineSettings(); + legacySettings.setCostPerEntryLegacyModeFeature(createLegacyModeFeature(true)); + FilterImpl legacyFilter = createFilter(indexed, NT_BASE, legacySettings); + legacyFilter.restrictPropertyAsList("foo", java.util.List.of(PropertyValues.newString("x1"))); + PropertyIndex pIndex = new PropertyIndex(Mounts.defaultMountInfoProvider()); + + LogCustomizer customLogs = LogCustomizer + .forLogger(PropertyIndex.class.getName()).enable(Level.DEBUG).create(); + try { + customLogs.starting(); + + // Baseline correctness: under legacy math, bIndex's unique + // short-circuit still wins even with a second definition present. + assertEquals("bIndex", pIndex.getIndexName(legacyFilter, indexed)); + assertEquals(2.0, pIndex.getCost(legacyFilter, indexed), 0.0); + + assertTrue("Expected bIndex's cost to be logged", + customLogs.getLogs().stream().anyMatch(msg -> msg.contains("bIndex"))); + assertFalse("aIndex must never be evaluated -- the break must fire " + + "immediately after bIndex hits COST_OVERHEAD under the legacy floor", + customLogs.getLogs().stream().anyMatch(msg -> msg.contains("aIndex"))); + } finally { + customLogs.finished(); + } + } + /** * getMinimumCost() takes no Filter/NodeState, so it cannot know whether any * definition has overridden costPerExecution below the old hardcoded floor