OAK-12348: Property and node type indexes support costPerEntry/costPerExecution overrides - #3076
Open
bhabegger wants to merge 15 commits into
Open
OAK-12348: Property and node type indexes support costPerEntry/costPerExecution overrides#3076bhabegger wants to merge 15 commits into
bhabegger wants to merge 15 commits into
Conversation
…rExecution 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.
bhabegger
force-pushed
the
issue/OAK-12348
branch
from
August 12, 2026 08:56
80a873b to
1893ff0
Compare
bhabegger
marked this pull request as ready for review
August 12, 2026 14:57
ChlineSaurus
suggested changes
Aug 31, 2026
| * Feature toggle name for the configurable costPerEntry/costPerExecution | ||
| * cost formula (OAK-12348). | ||
| */ | ||
| public static final String FT_OAK_12348 = "FT_OAK-12348"; |
Contributor
There was a problem hiding this comment.
Is this toggle usable? Where is it wired into the whiteboard mechanism?
In my opinion this kind of wiring is missing:
Contributor
Author
There was a problem hiding this comment.
Indeed the whiteboard registration was missed.
| * 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. | ||
| */ |
Contributor
There was a problem hiding this comment.
According to AI: oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndex.java:137 has an early return, that get's around this PR. However, in my opinion it should not cause any issues, as it only happens for cost == 2.0 which is rare, and anyways a very fast case.
Contributor
There was a problem hiding this comment.
But there seem to be things no longer correct in PropertyIndex.java like getMinimumCost()
Probably worth checking.
| */ | ||
| double getCost() { | ||
| return cost; | ||
| return PropertyIndexLookup.FT_OAK_12348_ENABLE.get() ? getCostConfigurable() : getCostLegacy(); |
Contributor
There was a problem hiding this comment.
Nit: Computed each time we call getCost() (called multiple times in createPlan), but probably works best with toggle this way.
Explains that rep:security's default authorizable store also extends nt:hierarchyNode, which would otherwise skew the NT_HIERARCHYNODE count assertion.
…th multiple definitions
ChlineSaurus
approved these changes
Sep 1, 2026
…java toggle wiring
…e object PropertyIndexProvider/NodeTypeIndexProvider still own the whiteboard Feature, but now resolve it to a plain boolean once, at the point where each PropertyIndex/NodeTypeIndex is constructed. Every class below that (PropertyIndexPlan, PropertyIndexLookup, NodeTypeIndexLookup) takes a non-nullable useLegacy boolean instead of an optional Feature -- they only need the resolved answer, not the toggle mechanism itself. getMinimumCost() on PropertyIndex/NodeTypeIndex now precomputes its return value once in the constructor and returns the stored field, instead of branching on every call. Tests no longer need Mockito to fake a Feature -- they just pass true/false directly.
Both are assigned exactly once, in the constructor -- pre-existing inconsistency, every other field in this class is already final.
matchesAllTypes, matchesNodeTypes, properties, pathFilter, unique, and bestCount were all read only inside the constructor or inside a helper method called only from the constructor -- none needed to survive as instance state. useLegacy is now consumed once, in the constructor, to compute cost, so it no longer needs to be a field either. getCostLegacy/getCostConfigurable/getStrategies now take bestCount/unique as parameters instead of reading fields, since they're never called outside the constructor.
…exLookup Both resolved indexMeta, got strategies, built the same ValuePattern, and ran the identical entry-counting loop -- they only differed in which costPerExecution/costPerEntry values fed the final formula (hardcoded for legacy, read from indexMeta for configurable). Extracted the shared part into a private getCost(indexMeta, filter, value, costPerExecution, costPerEntry) helper; each public method now just resolves its own costPerExecution/costPerEntry and delegates.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
PropertyIndex/NodeTypeIndex(oak-core) compute query cost purely fromindexed entry counts plus a fixed overhead of 2, with no way to influence
the estimate from the index definition — unlike Lucene/Elastic indexes
(
oak-search), which already readcostPerEntry/costPerExecution.In production this let the
nodeType/property index win a cost comparisonagainst a more selective, purpose-built index for the same query, because
the built-in entry-count estimate can be significantly wrong at scale (seen
in three related incidents on
rep:User/rep:Groupqueries against atagged
authorizablesLucene index). The workaround each time was amanual
costPerEntry/costPerExecutionoverride — but that only exists onthe Lucene/Elastic side.
How
PropertyIndexPlanandPropertyIndexLookupeach split into:getCostLegacy(...)— the original hardcoded formula, unconditionally.getCostConfigurable(...)—cost = costPerExecution + costPerEntry * entryCount,both optionally set on the property index definition (new
IndexConstants.COST_PER_ENTRY/COST_PER_EXECUTION, same property namesoak-search'sFulltextIndexConstantsalready uses for Lucene/Elastic).
getCost(...)— dispatches between the two based onFT_OAK-12348(
PropertyIndexLookup.FT_OAK_12348_ENABLE), enabled by default: withno properties set,
getCostConfigurable()reproducesgetCostLegacy()exactly, so this is behavior-preserving for every existing index
definition — the toggle is an escape hatch, not an opt-in gate.
NodeTypeIndexneeds no code changes at all — its cost is the sum of twoPropertyIndexLookup.getCost()calls (jcr:primaryType,jcr:mixinTypes),so it picks up the override transitively (covered by a dedicated test).
IndexUtilsgains a smallgetOptionalValue(NodeState, String, double)helper (mirrors
oak-search'sIndexDefinition.getOptionalValue, whichoak-corecan't depend on directly) instead of duplicating the sameproperty read in both classes.
Out of scope:
resultCacheSize(mentioned in the originating request) is aLucene/Elastic query-result caching concept with no equivalent in
PropertyIndex'sContentMirrorStoreStrategy-based lookup.Testing
New/updated tests in
PropertyIndexTest,PropertyIndexLookupTest(unchanged,still green), and
NodeTypeIndexTestcover: default behavior unchangedregardless of toggle position, override taking effect by default, the
toggle correctly falling back to the legacy formula when disabled, the
costPerEntry=0infinity-guard (must not becomeNaN), the unique-indexshort-circuit surviving an override, and
NodeTypeIndexpicking up theoverride with zero code changes in its own package.
Full
oak-coresuite: 4779 tests, 0 failures.