imbajin commented on code in PR #3243:
URL: https://github.com/apache/hugegraph/pull/3243#discussion_r4115118235
##########
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:
🧹 This enables full-coverage validation before validQueryConditionValues()
below can short-circuit an invalid condition to an empty result. For a
low-level ConditionQuery on an Int property with a String value, a partially
indexed multi-label query now throws NoIndexException where it previously
returned empty. Please preserve the invalid-value short circuit before
rejecting incomplete coverage, while keeping index-state validation before
iterator creation.
##########
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());
+ }
+ Set<Id> covered = new HashSet<>();
+ for (MatchedIndex index : indexes) {
+ covered.add(index.schemaLabel().id());
+ }
+ for (SchemaLabel label : labels) {
+ if (covered.contains(label.id()) ||
+ !label.properties().containsAll(requiredProperties)) {
+ continue;
+ }
+ if (label instanceof EdgeLabel) {
+ EdgeLabel edgeLabel = (EdgeLabel) label;
+ // Base labels have no direct edges; their property indexes
+ // include edges of their sub-labels (see updateEdgeIndex()).
+ if (edgeLabel.isFather() ||
+ edgeLabel.hasFather() &&
covered.contains(edgeLabel.fatherId())) {
+ continue;
+ }
+ }
+ // Authorization filters elements after the backend query. Do not
+ // disclose an implicitly discovered schema label in this error.
+ throw new NoIndexException("Incomplete index coverage for
properties %s; " +
Review Comment:
🧹 The new compatibility rule is user-visible, and repository guidance
requires matching in-repository documentation for behavior changes. The PR
description links a Gist but this branch adds no query/index documentation.
Please document that unlabeled property queries require compatible indexes for
every candidate label, including the explicit-label and index remedies.
##########
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:
‼️ With GraphReadMode.ALL, an OLAP-only query such as V().has("wcc", "a")
removes its only key from requiredProperties here. The same global OLAP index
then matches each label, but MatchedIndex equality deduplicates on indexLabels
alone, so only one label remains covered and this loop throws NoIndexException
for the others. Please skip per-label coverage when no non-OLAP keys remain or
treat the shared OLAP index as covering all candidate labels, and add a
regression based on the existing OLAP secondary-property test.
--
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]
