Copilot commented on code in PR #3034:
URL: https://github.com/apache/hugegraph/pull/3034#discussion_r3281454418
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -166,38 +170,219 @@ public static void trySetGraph(Step<?, ?> step,
HugeGraph graph) {
public static void extractHasContainer(HugeGraphStep<?, ?> newStep,
Traversal.Admin<?, ?> traversal) {
- Step<?, ?> step = newStep;
- do {
- step = step.getNextStep();
+ Step<?, ?> step = newStep.getNextStep();
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> nextStep = step.getNextStep();
if (step instanceof HasStep) {
HasContainerHolder holder = (HasContainerHolder) step;
- for (HasContainer has : holder.getHasContainers()) {
- if (!GraphStep.processHasContainerIds(newStep, has)) {
- newStep.addHasContainer(has);
- }
+ if (extractHasContainers(newStep, holder)) {
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
}
- TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
- traversal.removeStep(step);
}
- } while (step instanceof HasStep || step instanceof NoOpBarrierStep);
+ step = nextStep;
+ }
}
public static void extractHasContainer(HugeVertexStep<?> newStep,
Traversal.Admin<?, ?> traversal) {
Step<?, ?> step = newStep;
do {
+ Step<?, ?> nextStep = step.getNextStep();
if (step instanceof HasStep) {
HasContainerHolder holder = (HasContainerHolder) step;
- for (HasContainer has : holder.getHasContainers()) {
- newStep.addHasContainer(has);
+ if (extractHasContainers(newStep, holder)) {
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
}
- TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
- traversal.removeStep(step);
}
- step = step.getNextStep();
+ step = nextStep;
} while (step instanceof HasStep || step instanceof NoOpBarrierStep);
}
+ private static boolean extractHasContainers(HugeGraphStep<?, ?> newStep,
+ HasContainerHolder holder) {
+ HugeGraph graph = TraversalUtil.tryGetGraph(newStep);
+ List<HasContainer> hasContainers = new
ArrayList<>(holder.getHasContainers());
+ HugeType type = newStep.returnsVertex() ? HugeType.VERTEX :
HugeType.EDGE;
+ Set<Id> labelIds = collectLabelIds(graph, type,
newStep.getHasContainers(),
+ hasContainers);
+ for (HasContainer has : hasContainers) {
+ if (!canExtractHasContainer(graph, type, labelIds, has)) {
+ continue;
+ }
+ if (!GraphStep.processHasContainerIds(newStep, has)) {
+ newStep.addHasContainer(has);
+ }
+ holder.removeHasContainer(has);
+ }
+ return holder.getHasContainers().isEmpty();
+ }
+
+ private static boolean extractHasContainers(HugeVertexStep<?> newStep,
+ HasContainerHolder holder) {
+ HugeGraph graph = TraversalUtil.tryGetGraph(newStep);
+ List<HasContainer> hasContainers = new
ArrayList<>(holder.getHasContainers());
+ HugeType type = newStep.returnsVertex() ? HugeType.VERTEX :
HugeType.EDGE;
+ Set<Id> labelIds = collectLabelIds(graph, type,
newStep.getHasContainers(),
+ hasContainers);
+ for (HasContainer has : hasContainers) {
+ if (!canExtractHasContainer(graph, type, labelIds, has)) {
+ continue;
+ }
+ newStep.addHasContainer(has);
+ holder.removeHasContainer(has);
+ }
+ return holder.getHasContainers().isEmpty();
+ }
+
+ static boolean canExtractHasContainer(HugeGraph graph,
+ HasContainer has) {
+ return canExtractHasContainer(graph, HugeType.UNKNOWN, null, has);
Review Comment:
This overload always passes `labelIds = null`, and `hasIndexForProperty()`
returns `false` when `labelIds == null || size != 1`, which makes this method
effectively return `true` only for system properties (and `false` for all
non-system properties) regardless of schema/indexes. That behavior is very
surprising given the method name/signature and makes the tests less meaningful.
Consider either (a) removing this overload and testing via
`extractHasContainer()` behavior, (b) changing the overload to accept the
required context (`HugeType` + `labelIds`), or (c) documenting/renaming it to
reflect that it only answers 'sys props / no-index-check' scenarios.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtilOptimizeTest.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.traversal.optimize;
+
+import java.util.Collections;
+
+import org.apache.hugegraph.HugeGraph;
+import org.apache.hugegraph.backend.id.Id;
+import org.apache.hugegraph.backend.id.IdGenerator;
+import org.apache.hugegraph.exception.NotFoundException;
+import org.apache.hugegraph.schema.IndexLabel;
+import org.apache.hugegraph.schema.PropertyKey;
+import org.apache.hugegraph.testutil.Assert;
+import org.apache.hugegraph.type.define.DataType;
+import org.apache.tinkerpop.gremlin.process.traversal.P;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+public class TraversalUtilOptimizeTest {
+
+ @Test
+ public void testCanExtractHasContainerWithoutGraph() {
+ Assert.assertTrue(TraversalUtil.canExtractHasContainer(
+ null, new HasContainer("~label", P.eq("person"))));
+ Assert.assertTrue(TraversalUtil.canExtractHasContainer(
+ null, new HasContainer("~id", P.eq("1"))));
+ Assert.assertFalse(TraversalUtil.canExtractHasContainer(
+ null, new HasContainer("name", P.eq("marko"))));
+ }
+
+ @Test
+ public void testCanExtractHasContainerWithMissingPropertyKey() {
+ HugeGraph graph = Mockito.mock(HugeGraph.class);
+ Mockito.when(graph.propertyKey("missing"))
+ .thenThrow(new NotFoundException("missing"));
+
+ Assert.assertFalse(TraversalUtil.canExtractHasContainer(
+ graph, new HasContainer("missing", P.eq("marko"))));
+ }
+
+ @Test
+ public void testCanExtractHasContainerWithoutPropertyIndex() {
+ HugeGraph graph = Mockito.mock(HugeGraph.class);
+ PropertyKey age = propertyKey(1L, "age", DataType.INT);
+ Mockito.when(graph.propertyKey("age")).thenReturn(age);
+ Mockito.when(graph.indexLabels()).thenReturn(Collections.emptyList());
+
+ Assert.assertFalse(TraversalUtil.canExtractHasContainer(
+ graph, new HasContainer("age", P.eq(1))));
+ }
Review Comment:
`TraversalUtil.canExtractHasContainer(graph, has)` currently doesn’t consult
`graph.indexLabels()` at all, and (as implemented) can’t reach the
index-checking branch for non-sys properties because it lacks label context. As
a result, this test doesn’t actually validate the 'without property index'
condition (the `indexLabels()` stub is effectively irrelevant). Strengthen
coverage by adding at least one positive test that exercises index discovery
with label context (e.g., build a traversal containing `hasLabel('vl1')` +
`has('age', ...)`, run `extractHasContainer(...)`, and assert which
`HasContainer`s were/weren’t moved).
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtilOptimizeTest.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.traversal.optimize;
+
+import java.util.Collections;
+
+import org.apache.hugegraph.HugeGraph;
+import org.apache.hugegraph.backend.id.Id;
+import org.apache.hugegraph.backend.id.IdGenerator;
+import org.apache.hugegraph.exception.NotFoundException;
+import org.apache.hugegraph.schema.IndexLabel;
+import org.apache.hugegraph.schema.PropertyKey;
+import org.apache.hugegraph.testutil.Assert;
+import org.apache.hugegraph.type.define.DataType;
+import org.apache.tinkerpop.gremlin.process.traversal.P;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+public class TraversalUtilOptimizeTest {
+
+ @Test
+ public void testCanExtractHasContainerWithoutGraph() {
+ Assert.assertTrue(TraversalUtil.canExtractHasContainer(
+ null, new HasContainer("~label", P.eq("person"))));
+ Assert.assertTrue(TraversalUtil.canExtractHasContainer(
+ null, new HasContainer("~id", P.eq("1"))));
+ Assert.assertFalse(TraversalUtil.canExtractHasContainer(
+ null, new HasContainer("name", P.eq("marko"))));
+ }
+
+ @Test
+ public void testCanExtractHasContainerWithMissingPropertyKey() {
+ HugeGraph graph = Mockito.mock(HugeGraph.class);
+ Mockito.when(graph.propertyKey("missing"))
+ .thenThrow(new NotFoundException("missing"));
+
+ Assert.assertFalse(TraversalUtil.canExtractHasContainer(
+ graph, new HasContainer("missing", P.eq("marko"))));
+ }
+
+ @Test
+ public void testCanExtractHasContainerWithoutPropertyIndex() {
+ HugeGraph graph = Mockito.mock(HugeGraph.class);
+ PropertyKey age = propertyKey(1L, "age", DataType.INT);
+ Mockito.when(graph.propertyKey("age")).thenReturn(age);
+ Mockito.when(graph.indexLabels()).thenReturn(Collections.emptyList());
+
+ Assert.assertFalse(TraversalUtil.canExtractHasContainer(
+ graph, new HasContainer("age", P.eq(1))));
+ }
+
+ @Test
+ public void testCanExtractHasContainerWithoutLabelContext() {
+ HugeGraph graph = Mockito.mock(HugeGraph.class);
+ PropertyKey age = propertyKey(1L, "age", DataType.INT);
+ IndexLabel ageIndex = new IndexLabel(null, IdGenerator.of(2L),
+ "vl1ByAge");
+ ageIndex.indexFields(age.id());
Review Comment:
The test constructs an `IndexLabel` with minimal initialization (no index
type / base label binding). The production logic (`matchIndexType()`) calls
`indexLabel.indexType()` and will throw if this test ever starts exercising the
index path. To keep the test resilient to future refactors, consider fully
initializing the `IndexLabel` via schema APIs (preferred in integration-style
tests) or mocking `IndexLabel` and stubbing `indexType()`/`indexFields()`
explicitly.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -166,38 +170,219 @@ public static void trySetGraph(Step<?, ?> step,
HugeGraph graph) {
public static void extractHasContainer(HugeGraphStep<?, ?> newStep,
Traversal.Admin<?, ?> traversal) {
- Step<?, ?> step = newStep;
- do {
- step = step.getNextStep();
+ Step<?, ?> step = newStep.getNextStep();
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> nextStep = step.getNextStep();
if (step instanceof HasStep) {
HasContainerHolder holder = (HasContainerHolder) step;
- for (HasContainer has : holder.getHasContainers()) {
- if (!GraphStep.processHasContainerIds(newStep, has)) {
- newStep.addHasContainer(has);
- }
+ if (extractHasContainers(newStep, holder)) {
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
}
- TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
- traversal.removeStep(step);
}
- } while (step instanceof HasStep || step instanceof NoOpBarrierStep);
+ step = nextStep;
+ }
}
public static void extractHasContainer(HugeVertexStep<?> newStep,
Traversal.Admin<?, ?> traversal) {
Step<?, ?> step = newStep;
do {
+ Step<?, ?> nextStep = step.getNextStep();
if (step instanceof HasStep) {
HasContainerHolder holder = (HasContainerHolder) step;
- for (HasContainer has : holder.getHasContainers()) {
- newStep.addHasContainer(has);
+ if (extractHasContainers(newStep, holder)) {
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
}
- TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
- traversal.removeStep(step);
}
- step = step.getNextStep();
+ step = nextStep;
} while (step instanceof HasStep || step instanceof NoOpBarrierStep);
}
+ private static boolean extractHasContainers(HugeGraphStep<?, ?> newStep,
+ HasContainerHolder holder) {
+ HugeGraph graph = TraversalUtil.tryGetGraph(newStep);
+ List<HasContainer> hasContainers = new
ArrayList<>(holder.getHasContainers());
+ HugeType type = newStep.returnsVertex() ? HugeType.VERTEX :
HugeType.EDGE;
+ Set<Id> labelIds = collectLabelIds(graph, type,
newStep.getHasContainers(),
+ hasContainers);
+ for (HasContainer has : hasContainers) {
+ if (!canExtractHasContainer(graph, type, labelIds, has)) {
+ continue;
+ }
+ if (!GraphStep.processHasContainerIds(newStep, has)) {
+ newStep.addHasContainer(has);
+ }
+ holder.removeHasContainer(has);
+ }
+ return holder.getHasContainers().isEmpty();
+ }
+
+ private static boolean extractHasContainers(HugeVertexStep<?> newStep,
+ HasContainerHolder holder) {
+ HugeGraph graph = TraversalUtil.tryGetGraph(newStep);
+ List<HasContainer> hasContainers = new
ArrayList<>(holder.getHasContainers());
+ HugeType type = newStep.returnsVertex() ? HugeType.VERTEX :
HugeType.EDGE;
+ Set<Id> labelIds = collectLabelIds(graph, type,
newStep.getHasContainers(),
+ hasContainers);
+ for (HasContainer has : hasContainers) {
+ if (!canExtractHasContainer(graph, type, labelIds, has)) {
+ continue;
+ }
+ newStep.addHasContainer(has);
+ holder.removeHasContainer(has);
+ }
+ return holder.getHasContainers().isEmpty();
+ }
+
+ static boolean canExtractHasContainer(HugeGraph graph,
+ HasContainer has) {
+ return canExtractHasContainer(graph, HugeType.UNKNOWN, null, has);
+ }
+
+ private static boolean canExtractHasContainer(HugeGraph graph,
+ HugeType type,
+ Set<Id> labelIds,
+ HasContainer has) {
+ if (isSysProp(has.getKey())) {
+ return true;
+ }
+ if (graph == null) {
+ return false;
+ }
+
+ PropertyKey pkey;
+ try {
+ pkey = graph.propertyKey(has.getKey());
+ } catch (NotFoundException e) {
+ return false;
+ }
+ if (!hasIndexForProperty(graph, type, labelIds, pkey, has)) {
+ return false;
+ }
+ if (!pkey.dataType().isText()) {
+ return true;
+ }
+
+ List<P<Object>> predicates = new ArrayList<>();
+ collectPredicates(predicates, ImmutableList.of(has.getPredicate()));
+ for (P<Object> pred : predicates) {
+ BiPredicate<?, ?> bp = pred.getBiPredicate();
+ if (bp == Compare.gt || bp == Compare.gte ||
+ bp == Compare.lt || bp == Compare.lte) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ private static boolean hasIndexForProperty(HugeGraph graph,
+ HugeType type,
+ Set<Id> labelIds,
+ PropertyKey pkey,
+ HasContainer has) {
+ if (labelIds == null || labelIds.size() != 1) {
+ return false;
+ }
+
+ SchemaLabel schemaLabel = schemaLabel(graph, type,
labelIds.iterator().next());
+ for (Id indexLabelId : schemaLabel.indexLabels()) {
+ IndexLabel indexLabel = graph.indexLabel(indexLabelId);
+ if (matchIndexField(indexLabel, pkey) &&
+ matchIndexType(indexLabel, has)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private static boolean matchIndexField(IndexLabel indexLabel,
+ PropertyKey pkey) {
+ List<Id> fields = indexLabel.indexFields();
+ return !fields.isEmpty() && fields.get(0).equals(pkey.id());
+ }
+
+ private static SchemaLabel schemaLabel(HugeGraph graph, HugeType type,
+ Id label) {
+ if (type.isVertex()) {
+ return graph.vertexLabel(label);
+ } else {
+ assert type.isEdge();
+ return graph.edgeLabel(label);
+ }
+ }
+
+ private static boolean matchIndexType(IndexLabel indexLabel,
+ HasContainer has) {
+ if (indexLabel.indexType().isUnique()) {
+ return false;
+ }
+
Review Comment:
Blocking extraction for all unique indexes is likely to regress query
planning: previously, the optimizer would extract the `has()` and let backend
index selection (including unique) apply. If unique indexes are safe for
`eq`/`within` predicates (common case), consider allowing them here (and
keeping the existing disallow rules for unsupported predicates like
`neq/without`). If unique must be excluded for correctness, please add an
inline comment explaining why, since the current logic reads like an unintended
optimization loss.
--
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]
