bhabegger commented on code in PR #3076:
URL: https://github.com/apache/jackrabbit-oak/pull/3076#discussion_r4059838274
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexPlan.java:
##########
@@ -67,53 +67,49 @@ public class PropertyIndexPlan {
private final String name;
- private final Set<String> properties;
-
private final Set<IndexStoreStrategy> strategies;
private final Filter filter;
- private boolean matchesAllTypes;
-
- private boolean matchesNodeTypes;
-
- private final double cost;
-
private final Set<String> values;
private final int depth;
- private final PathFilter pathFilter;
-
- private final boolean unique;
-
private final boolean deprecated;
+ // Value returned by getCost().
+ private final double cost;
+
PropertyIndexPlan(String name, NodeState root, NodeState definition,
Filter filter){
this(name, root, definition, filter,
Mounts.defaultMountInfoProvider());
}
PropertyIndexPlan(String name, NodeState root, NodeState definition,
Filter filter, MountInfoProvider mountInfoProvider) {
+ this(name, root, definition, filter, mountInfoProvider, false);
+ }
+
+ PropertyIndexPlan(String name, NodeState root, NodeState definition,
+ Filter filter, MountInfoProvider mountInfoProvider,
boolean disableConfigurableCosts) {
this.name = name;
- this.unique =
definition.getBoolean(IndexConstants.UNIQUE_PROPERTY_NAME);
+ boolean unique =
definition.getBoolean(IndexConstants.UNIQUE_PROPERTY_NAME);
this.definition = definition;
- this.properties = SetUtils.toSet(definition.getNames(PROPERTY_NAMES));
- pathFilter = PathFilter.from(definition.builder());
- this.strategies = getStrategies(definition, mountInfoProvider);
+ Set<String> properties =
SetUtils.toSet(definition.getNames(PROPERTY_NAMES));
+ PathFilter pathFilter = PathFilter.from(definition.builder());
+ this.strategies = getStrategies(definition, mountInfoProvider, unique);
this.filter = filter;
Iterable<String> types = definition.getNames(DECLARING_NODE_TYPES);
// if there is no such property, then all nodetypes are matched
- this.matchesAllTypes = !definition.hasProperty(DECLARING_NODE_TYPES);
+ boolean matchesAllTypes =
!definition.hasProperty(DECLARING_NODE_TYPES);
this.deprecated =
definition.getBoolean(IndexConstants.INDEX_DEPRECATED);
- this.matchesNodeTypes =
+ boolean matchesNodeTypes =
matchesAllTypes ||
StreamUtils.toStream(types).anyMatch(filter.getSupertypes()::contains);
ValuePattern valuePattern = new ValuePattern(definition);
- double bestCost = Double.POSITIVE_INFINITY;
+ double bestCount = Double.POSITIVE_INFINITY;
Review Comment:
To me the type mismatch and weird assignments are a symptom that something
is not quite right (having a count as a double could just me more practical as
further down we multiply with a cost per entry which legitimately is double).
The true question is, are we counting or not ? And I think we are counting.
Otherwise this formula:
```
return costPerExecution + costPerEntry * entryCount;
```
Would become:
```
return costPerExecution + costPerEntry * entryCost;
```
Which makes way less sense... So, if fact, the bug is not in the rename but
in the name of MAX_COST which is in fact a MAX_COUNT. I just did a little local
refactor to better understand the code.
So question is this a `bestCount` (raw count without factors taken into
account) or a `bestCost` (count including factors)? Below we are comparing it
with something that, functionally, *is* a count (even is we count using
doubles).
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/property/PropertyIndexPlan.java:
##########
@@ -67,53 +67,49 @@ public class PropertyIndexPlan {
private final String name;
- private final Set<String> properties;
-
private final Set<IndexStoreStrategy> strategies;
private final Filter filter;
- private boolean matchesAllTypes;
-
- private boolean matchesNodeTypes;
-
- private final double cost;
-
private final Set<String> values;
private final int depth;
- private final PathFilter pathFilter;
-
- private final boolean unique;
-
private final boolean deprecated;
+ // Value returned by getCost().
+ private final double cost;
+
PropertyIndexPlan(String name, NodeState root, NodeState definition,
Filter filter){
this(name, root, definition, filter,
Mounts.defaultMountInfoProvider());
}
PropertyIndexPlan(String name, NodeState root, NodeState definition,
Filter filter, MountInfoProvider mountInfoProvider) {
+ this(name, root, definition, filter, mountInfoProvider, false);
+ }
+
+ PropertyIndexPlan(String name, NodeState root, NodeState definition,
+ Filter filter, MountInfoProvider mountInfoProvider,
boolean disableConfigurableCosts) {
this.name = name;
- this.unique =
definition.getBoolean(IndexConstants.UNIQUE_PROPERTY_NAME);
+ boolean unique =
definition.getBoolean(IndexConstants.UNIQUE_PROPERTY_NAME);
this.definition = definition;
- this.properties = SetUtils.toSet(definition.getNames(PROPERTY_NAMES));
- pathFilter = PathFilter.from(definition.builder());
- this.strategies = getStrategies(definition, mountInfoProvider);
+ Set<String> properties =
SetUtils.toSet(definition.getNames(PROPERTY_NAMES));
+ PathFilter pathFilter = PathFilter.from(definition.builder());
+ this.strategies = getStrategies(definition, mountInfoProvider, unique);
this.filter = filter;
Iterable<String> types = definition.getNames(DECLARING_NODE_TYPES);
// if there is no such property, then all nodetypes are matched
- this.matchesAllTypes = !definition.hasProperty(DECLARING_NODE_TYPES);
+ boolean matchesAllTypes =
!definition.hasProperty(DECLARING_NODE_TYPES);
this.deprecated =
definition.getBoolean(IndexConstants.INDEX_DEPRECATED);
- this.matchesNodeTypes =
+ boolean matchesNodeTypes =
matchesAllTypes ||
StreamUtils.toStream(types).anyMatch(filter.getSupertypes()::contains);
ValuePattern valuePattern = new ValuePattern(definition);
- double bestCost = Double.POSITIVE_INFINITY;
+ double bestCount = Double.POSITIVE_INFINITY;
Review Comment:
I think the true problem is that the original code is mixing both notions...
Before it did not really matter because cost == count (more or less), but now
that we are introducing costPerEntry... well we can't mix up both notions
anymore.
With this PR, we mostly have a count, but there are still some cost/count
inconsistencies (e.g. cost overhead is included in the count because it is deep
in the code, making bubbling it up challenging).
--
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]
