contrueCT commented on code in PR #3243:
URL: https://github.com/apache/hugegraph/pull/3243#discussion_r4121467040
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java:
##########
@@ -480,7 +480,7 @@ private IdHolderList queryByUserprop(ConditionQuery query) {
}
}
}
- Set<MatchedIndex> indexes = this.collectMatchedIndexes(query);
+ Set<MatchedIndex> indexes = this.collectMatchedIndexes(query, true);
Review Comment:
Fixed in `f435a1f5`. Index matching still happens first, preserving the
existing `NoIndexException` when no index matches. For a matched index, an
invalid property value now takes the original empty-result path before the new
coverage check; valid values still require complete coverage, and index-state
validation remains before iterator creation. The new low-level regression
checks all three cases. It passed on RocksDB and HStore; results are in the
[Gist](https://gist.github.com/contrueCT/669821a6ac6fc5a4d3adf6df6f74a342#file-source-index-coverage-md).
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java:
##########
@@ -805,9 +814,49 @@ private Set<MatchedIndex>
collectMatchedIndexes(ConditionQuery query) {
matchedIndexes.add(index);
}
}
+ if (requireCompleteCoverage && !matchedIndexes.isEmpty()) {
+ this.checkIndexCoverage(query, schemaLabels, matchedIndexes);
+ }
return matchedIndexes;
}
+ private void checkIndexCoverage(ConditionQuery query,
+ List<? extends SchemaLabel> labels,
+ Set<MatchedIndex> indexes) {
+ // Check at execution, before creating any lazy index iterator. Cleanup
+ // callers only need matching indexes and must not require full
coverage.
+ Set<Id> requiredProperties = query.userpropKeys();
+ if (this.graph().readMode().showOlap()) {
+ // OLAP properties are shared rather than declared on every label.
+ requiredProperties.removeIf(id ->
this.graph().propertyKey(id).olap());
Review Comment:
Confirmed and fixed in `f435a1f5` with the shared-OLAP-index guard and a
multi-label regression. The new test runs on RocksDB. I also ran the
repository's existing `VertexCoreTest.testAddOlapSecondaryProperties` against
unmodified master on a fresh HStore graph with `-Dbackend=hstore`: it failed
with expected 1, actual 0. Therefore the new OLAP test explicitly skips HStore
under the existing #3090 limitation. On a fresh HStore graph the other 15
coverage methods passed (16 run, 1 skipped, no failures/errors); the
[Gist](https://gist.github.com/contrueCT/669821a6ac6fc5a4d3adf6df6f74a342#file-source-index-coverage-md)
records the baseline and validation limits.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
