Copilot commented on code in PR #2991:
URL: https://github.com/apache/hugegraph/pull/2991#discussion_r3186323719
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -488,6 +494,31 @@ private static Condition.Relation
convCompare2UserpropRelation(HugeGraph graph,
}
}
+ private static Condition convCompare2BooleanUserpropRelation(Compare
compare,
+ Id key,
+ Boolean
value) {
+ switch (compare) {
+ case eq:
+ return Condition.eq(key, value);
+ case neq:
+ return Condition.eq(key, !value);
+ case gt:
+ return value ? Condition.in(key, ImmutableList.of()) :
+ Condition.eq(key, true);
+ case gte:
+ return value ? Condition.eq(key, true) :
+ Condition.in(key, ImmutableList.of(false, true));
+ case lt:
+ return value ? Condition.eq(key, false) :
+ Condition.in(key, ImmutableList.of());
Review Comment:
Normalizing `gt(true)`/`lt(false)` to `IN []` breaks compound predicates
built with `AndP`/`OrP`. `convAnd()`/`convOr()` can produce trees like
`OR(IN[], EQ(true))`, and `ConditionQueryFlatten.flattenIn()` currently
rebuilds `AND`/`OR` nodes without handling a `null` child from `IN []`, so
traversals such as `has('flag', P.gt(true).or(P.eq(true)))` will now fail
during flattening instead of simplifying to the remaining branch or to an empty
result.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/core/ConditionQueryFlattenTest.java:
##########
@@ -256,5 +256,65 @@ public void testFlattenWithNotIn() {
Collection<Condition> actual = queries.iterator().next().conditions();
Assert.assertEquals(expect, actual);
}
+
+ @Test
+ public void testFlattenWithBooleanRangeUpperBound() {
+ Id key = IdGenerator.of("c1");
+
+ ConditionQuery query = new ConditionQuery(HugeType.VERTEX);
+ query.query(Condition.lt(key, true));
+ query.query(Condition.lt(key, false));
+
+ List<ConditionQuery> queries = ConditionQueryFlatten.flatten(query);
+ Assert.assertEquals(1, queries.size());
+
+ Collection<Condition> actual = queries.iterator().next().conditions();
+ Assert.assertEquals(ImmutableList.of(Condition.lt(key, false)),
actual);
+ }
+
+ @Test
+ public void testFlattenWithBooleanRangeWindow() {
+ Id key = IdGenerator.of("c1");
+
+ ConditionQuery query = new ConditionQuery(HugeType.VERTEX);
+ query.query(Condition.gte(key, false));
+ query.query(Condition.lt(key, true));
+
+ List<ConditionQuery> queries = ConditionQueryFlatten.flatten(query);
+ Assert.assertEquals(1, queries.size());
+
+ Collection<Condition> actual = queries.iterator().next().conditions();
+ Assert.assertEquals(ImmutableList.of(Condition.gte(key, false),
+ Condition.lt(key, true)), actual);
+ }
+
+ @Test
+ public void testFlattenWithConflictingBooleanRange() {
+ Id key = IdGenerator.of("c1");
+
+ ConditionQuery query = new ConditionQuery(HugeType.VERTEX);
+ query.query(Condition.gt(key, false).and(Condition.lt(key, true)));
+
+ List<ConditionQuery> queries = ConditionQueryFlatten.flatten(query);
+ Assert.assertEquals(0, queries.size());
Review Comment:
These new flattening tests cover only standalone boolean ranges, but the new
normalization in `TraversalUtil` can also introduce `IN []` inside compound
`AND`/`OR` trees. A regression like `has('flag', P.gt(true).or(P.eq(true)))` is
currently untested, even though that path now depends on the flattening logic
handling impossible boolean branches correctly.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/VertexCoreTest.java:
##########
@@ -6470,6 +6470,37 @@ public void testQueryVertexByPropertyWithEmptyString() {
Assert.assertEquals("", vertex.value("city"));
}
+ @Test
+ public void testQueryVertexByBooleanNeqPredicate() {
+ HugeGraph graph = graph();
+ Assume.assumeFalse("skip this test for hstore",
+ Objects.equals("hstore", graph.backend()));
+
+ graph.schema().indexLabel("languageByDynamic").onV("language")
+ .secondary().by("dynamic").create();
+
+ graph.addVertex(T.label, "language", "name", "java",
+ "dynamic", true);
+ graph.addVertex(T.label, "language", "name", "rust",
+ "dynamic", false);
+ graph.addVertex(T.label, "language", "name", "c");
+ this.commitTx();
+
+ List<Vertex> neqTrueVertices = graph.traversal().V()
+ .hasLabel("language")
+ .has("dynamic", P.neq(true))
+ .toList();
+ Assert.assertEquals(1, neqTrueVertices.size());
+ Assert.assertEquals("rust", neqTrueVertices.get(0).value("name"));
+
+ List<Vertex> neqFalseVertices = graph.traversal().V()
+ .hasLabel("language")
+ .has("dynamic", P.neq(false))
+ .toList();
+ Assert.assertEquals(1, neqFalseVertices.size());
+ Assert.assertEquals("java", neqFalseVertices.get(0).value("name"));
+ }
+
@Test
Review Comment:
The end-to-end regression coverage here only exercises `P.neq(...)` on
vertices, while the production change in `TraversalUtil` also rewrites boolean
`lt/lte/gt/gte` for all vertex properties. A vertex-level
`has()/where()/match()` regression similar to the edge test is still missing,
so this bug could reappear for vertices without being caught by this suite.
--
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]
