Copilot commented on code in PR #3034:
URL: https://github.com/apache/hugegraph/pull/3034#discussion_r3279446813
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -171,13 +171,10 @@ public static void extractHasContainer(HugeGraphStep<?,
?> newStep,
step = 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);
}
Review Comment:
In the `HugeGraphStep` overload, if `extractHasContainers(newStep, holder)`
returns `false`, the current `step` remains a `HasStep` and is neither removed
nor advanced, but the `do/while` condition remains true—this can lead to an
infinite loop. Fix by ensuring `step` advances when extraction is skipped
(e.g., move `step = step.getNextStep()` to the end of the loop like the
`HugeVertexStep` overload, or explicitly advance/break when extraction returns
false).
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/CountStrategyCoreTest.java:
##########
@@ -125,4 +125,82 @@ public void
testWhereCountGteNegativeDoesNotBuildInvalidRange() {
Assert.assertEquals(4L, count);
}
+
+ @Test
+ public void testRepeatAfterTextRangeFilterWithEmptyResult() {
+ SchemaManager schema = graph().schema();
+ schema.propertyKey("vp4").asText().create();
+ schema.propertyKey("age").asInt().create();
+ schema.vertexLabel("vl1").properties("vp4", "age")
+ .nullableKeys("vp4", "age").create();
+ schema.edgeLabel("el2").link("vl1", "vl1").create();
+
+ Vertex v1 = graph().addVertex(T.label, "vl1", "vp4", "a", "age", 1);
+ Vertex v2 = graph().addVertex(T.label, "vl1", "vp4", "b", "age", 2);
+ v1.addEdge("el2", v2);
+ commitTx();
+
+ long direct = graph().traversal().V().has("vp4", P.lt(""))
+ .repeat(__.out("el2")).emit().times(1)
+ .count().next();
+ long viaMatch = graph().traversal().V()
+ .match(__.as("start").has("vp4", P.lt(""))
+ .out("el2").as("m"))
+ .select("m").count().next();
+
+ Assert.assertEquals(0L, direct);
+ Assert.assertEquals(viaMatch, direct);
+ }
+
+ @Test
+ public void testTextRangeFilterDoesNotStopLaterGraphHasExtraction() {
+ SchemaManager schema = graph().schema();
+ schema.propertyKey("vp4").asText().create();
+ schema.propertyKey("age").asInt().create();
+ schema.vertexLabel("vl1").properties("vp4", "age")
+ .nullableKeys("vp4", "age").create();
Review Comment:
The schema setup block (property keys + vertex label) is duplicated across
the newly added tests in this file. Consider extracting a small private helper
to create the shared schema to reduce duplication and make future schema
changes less error-prone.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/core/TraversalUtilTest.java:
##########
@@ -17,15 +17,31 @@
package org.apache.hugegraph.unit.core;
+import java.lang.reflect.Method;
+
import org.apache.hugegraph.HugeException;
+import org.apache.hugegraph.HugeGraph;
import org.apache.hugegraph.testutil.Assert;
import org.apache.hugegraph.traversal.optimize.TraversalUtil;
import org.apache.hugegraph.util.DateUtil;
import org.apache.tinkerpop.gremlin.process.traversal.P;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer;
import org.junit.Test;
public class TraversalUtilTest {
+ @Test
+ public void testCanExtractHasContainerWithoutGraph() throws Exception {
+ Method method = TraversalUtil.class.getDeclaredMethod(
+ "canExtractHasContainer", HugeGraph.class, HasContainer.class);
+ method.setAccessible(true);
+
+ Assert.assertTrue((Boolean) method.invoke(
+ null, null, new HasContainer("~label", P.eq("person"))));
+ Assert.assertFalse((Boolean) method.invoke(
+ null, null, new HasContainer("name", P.eq("marko"))));
+ }
Review Comment:
This test relies on reflective access to a private method
(`setAccessible(true)`), which is brittle and can be impacted by stricter
access controls in newer JDKs. Prefer exercising the behavior through a
public/package-private API, or consider making `canExtractHasContainer`
package-private (optionally annotated as test-visible) so the test can call it
directly without reflection.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/CountStrategyCoreTest.java:
##########
@@ -125,4 +125,82 @@ public void
testWhereCountGteNegativeDoesNotBuildInvalidRange() {
Assert.assertEquals(4L, count);
}
+
+ @Test
+ public void testRepeatAfterTextRangeFilterWithEmptyResult() {
+ SchemaManager schema = graph().schema();
+ schema.propertyKey("vp4").asText().create();
+ schema.propertyKey("age").asInt().create();
+ schema.vertexLabel("vl1").properties("vp4", "age")
+ .nullableKeys("vp4", "age").create();
+ schema.edgeLabel("el2").link("vl1", "vl1").create();
+
+ Vertex v1 = graph().addVertex(T.label, "vl1", "vp4", "a", "age", 1);
+ Vertex v2 = graph().addVertex(T.label, "vl1", "vp4", "b", "age", 2);
+ v1.addEdge("el2", v2);
+ commitTx();
+
+ long direct = graph().traversal().V().has("vp4", P.lt(""))
+ .repeat(__.out("el2")).emit().times(1)
+ .count().next();
+ long viaMatch = graph().traversal().V()
+ .match(__.as("start").has("vp4", P.lt(""))
+ .out("el2").as("m"))
+ .select("m").count().next();
+
+ Assert.assertEquals(0L, direct);
+ Assert.assertEquals(viaMatch, direct);
Review Comment:
`assertEquals` is typically written as `(expected, actual)`. Swapping the
arguments here reduces the usefulness of failure messages. Consider using
`Assert.assertEquals(direct, viaMatch)` (or `Assert.assertEquals(0L, viaMatch)`
where applicable) for clearer diagnostics.
--
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]
