bitflicker64 commented on code in PR #3243: URL: https://github.com/apache/hugegraph/pull/3243#discussion_r4123722191
########## hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/SourceIndexCoverageTest.java: ########## @@ -0,0 +1,407 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hugegraph.core; + +import java.util.HashSet; +import java.util.Iterator; +import java.util.Set; + +import org.apache.hugegraph.backend.page.PageInfo; +import org.apache.hugegraph.backend.query.Condition; +import org.apache.hugegraph.backend.query.ConditionQuery; +import org.apache.hugegraph.exception.NoIndexException; +import org.apache.hugegraph.schema.IndexLabel; +import org.apache.hugegraph.schema.SchemaManager; +import org.apache.hugegraph.testutil.Assert; +import org.apache.hugegraph.traversal.optimize.Text; +import org.apache.hugegraph.type.HugeType; +import org.apache.hugegraph.type.define.GraphReadMode; +import org.apache.hugegraph.type.define.SchemaStatus; +import org.apache.hugegraph.type.define.WriteType; +import org.apache.tinkerpop.gremlin.process.traversal.P; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.apache.tinkerpop.gremlin.structure.T; +import org.apache.tinkerpop.gremlin.structure.Vertex; +import org.apache.tinkerpop.gremlin.structure.util.CloseableIterator; +import org.junit.Assume; +import org.junit.Before; +import org.junit.Test; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; + +public class SourceIndexCoverageTest extends BaseCoreTest { + + @Before + public void initSchema() { + SchemaManager schema = graph().schema(); + schema.propertyKey("coverageName").asText().create(); + schema.propertyKey("coverageScore").asInt().create(); + schema.propertyKey("coverageBody").asText().create(); + schema.vertexLabel("coverageA") + .properties("coverageName", "coverageScore", "coverageBody") + .useAutomaticId().create(); + schema.vertexLabel("coverageB") + .properties("coverageName", "coverageScore", "coverageBody") + .enableLabelIndex(false).useAutomaticId().create(); + schema.vertexLabel("unrelated").properties("coverageBody").useAutomaticId().create(); + } + + private Set<Vertex> addVertices() { + Vertex a = graph().addVertex(T.label, "coverageA", "coverageName", "same", + "coverageScore", 10, "coverageBody", "gold silver"); + Vertex b = graph().addVertex(T.label, "coverageB", "coverageName", "same", + "coverageScore", 20, "coverageBody", "gold bronze"); + this.commitTx(); + return ImmutableSet.of(a, b); + } + + private void nameIndex(String label) { + graph().schema().indexLabel(label + "ByName").onV(label) + .by("coverageName").secondary().create(); + } + + @Test + public void testCompleteCandidatesOrExplicitFailure() { + this.nameIndex("coverageA"); + Set<Vertex> expected = this.addVertices(); + Set<Vertex> actual; + try { + actual = graph().traversal().V().has("coverageName", "same").toSet(); + } catch (NoIndexException e) { Review Comment: Minor: `testCompleteCandidatesOrExplicitFailure` returns early on `NoIndexException`, so it passes both when the query throws and when it returns both vertices. With only `coverageA` indexed, the patched code always throws, so the equality assertion never runs and the test checks nothing that `testPartialCoverageRejectsIncompleteSource` does not already cover. Please remove it, or index both labels and assert the full result without the catch. ########## hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java: ########## @@ -805,9 +814,57 @@ private Set<MatchedIndex> collectMatchedIndexes(ConditionQuery query) { matchedIndexes.add(index); } } + // An invalid value returns no results after an index is found. Keep + // that result before deciding whether index coverage is complete. + if (requireCompleteCoverage && !matchedIndexes.isEmpty() && + validQueryConditionValues(this.graph(), query)) { + this.checkIndexCoverage(query, schemaLabels, matchedIndexes); Review Comment: Important: This check breaks the TinkerPop process suite, and the PR checks cannot show it. `TestGraph.initModernSchema()` declares `name` and `age` on the `animal` vertex label without any index, while `person`, `software` and `vertex` are indexed. TinkerPop 3.5.1 `AbstractGremlinTest.convertToVertex()` resolves test vertices with `g.V().has("name", vertexName)` and no label, so every process test that calls `convertToVertex`/`convertToVertexId` on the modern graph now fails with `NoIndexException` instead of finding `marko`. I checked this with a throwaway memory-backend core test that calls `new TestGraph(graph()).initModernSchema(IdStrategy.AUTOMATIC)`, adds one person, and runs `V().has("name", "marko")` and `V().has("age", 29)`: both throw "Incomplete index coverage" at f435a1f5 and both return the vertex with master's `GraphIndexTransaction`. `server-ci.yml` runs `run-tinkerpop-test.sh` only when `RELEASE_BRANCH` is true (release-* or test-* refs), so the next release branc h is where this surfaces. Please add the missing indexes in `TestGraph` (for example `animalByName`, a range `animalByAge`, and the same for any other schema in that file with unindexed shared keys such as `createdBy.weight`), or add the labels there, and run the structure and process suites once on a test-* branch before merge. -- 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]
