ChlineSaurus commented on code in PR #3076:
URL: https://github.com/apache/jackrabbit-oak/pull/3076#discussion_r3892847703
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java:
##########
@@ -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";
Review Comment:
Is this toggle usable? Where is it wired into the whiteboard mechanism?
In my opinion this kind of wiring is missing:
https://github.com/apache/jackrabbit-oak/blob/a3d05a49c0b622b0fea30f9d25c5a66a83797fe1/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java#L68
https://github.com/apache/jackrabbit-oak/blob/a3d05a49c0b622b0fea30f9d25c5a66a83797fe1/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java#L588
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java:
##########
@@ -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.
+ */
Review 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.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexPlan.java:
##########
@@ -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();
Review Comment:
Nit: Computed each time we call getCost() (called multiple times in
createPlan), but probably works best with toggle this way.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java:
##########
@@ -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);
Review Comment:
Did we decide/discuss if we want _default=on_ for bug fixes, or improvements
in this style. ? Would also be relevant for my other PR.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexLookup.java:
##########
@@ -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.
+ */
Review Comment:
But there seem to be things no longer correct in PropertyIndex.java like
getMinimumCost()
https://github.com/apache/jackrabbit-oak/blob/a3d05a49c0b622b0fea30f9d25c5a66a83797fe1/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndex.java#L223
Probably worth checking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
