Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ private static boolean equals(final Object first,
*
* @param first is actual value, might be Number/Date or String, It is
* probably that the `first` is serialized to String.
* @param second is value in query condition, must be Number/Date
* @param second is value in query condition, must be Number/Date/Boolean
* @return the value 0 if first is numerically equal to second;
* a value less than 0 if first is numerically less than
* second; and a value greater than 0 if first is
Expand All @@ -208,6 +208,8 @@ private static int compare(final Object first, final Object second) {
(Number) second);
} else if (second instanceof Date) {
return compareDate(first, (Date) second);
} else if (second instanceof Boolean) {
return compareBoolean(first, (Boolean) second);
}

throw new IllegalArgumentException(String.format(
Expand All @@ -230,6 +232,20 @@ private static int compareDate(Object first, Date second) {
second, second.getClass().getSimpleName()));
}

private static int compareBoolean(Object first, Boolean second) {
if (first == null) {
first = false;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ compareBoolean: null → false coercion is surprising

When first is null, it's silently treated as false. This is asymmetric with compareNumber and compareDate which don't do null coercion. If a property is absent (null), treating it as false could cause incorrect query results — e.g., a vertex with no boolean property set would match lte(false).

Consider whether null should throw or return a defined comparison result (e.g., null sorts before any non-null value), consistent with how other types handle it.

}
if (first instanceof Boolean) {
return Boolean.compare((Boolean) first, second);
}

throw new IllegalArgumentException(String.format(
"Can't compare between %s(%s) and %s(%s)",
first, first.getClass().getSimpleName(),
second, second.getClass().getSimpleName()));
}

private void checkBaseType(Object value, Class<?> clazz) {
if (!clazz.isInstance(value)) {
String valueClass = value == null ? "null" :
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ private static List<ConditionQuery> flattenRelations(ConditionQuery query) {
cq.query(nonRelations);
return ImmutableList.of(cq);
}
return ImmutableList.of(query);
return ImmutableList.of();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ Critical: flattenRelations returns empty list for ALL conflicting conditions, not just boolean

This change from ImmutableList.of(query) to ImmutableList.of() affects all data types, not just booleans. Previously, when optimizeRelations returned null (meaning conditions were mutually exclusive, e.g., age > 10 AND age == 9), the original query was still returned — the contradiction was preserved and evaluated downstream. Now it returns an empty list, silently dropping the query.

While returning an empty list for contradictory conditions is arguably more correct (no results possible), this is a behavioral change for numeric/date/string range contradictions too, not just booleans. This should be:

  1. Called out explicitly in the PR description since it changes existing behavior for all types
  2. Covered with a test for numeric contradictory ranges to ensure no regression

Was this intentional or is the fix only meant for the boolean case?

}

private static Relations optimizeRelations(Relations relations) {
Expand Down Expand Up @@ -427,9 +427,26 @@ private static boolean validRange(Relation low, Relation high) {
if (low == null || high == null) {
return true;
}
return compare(low, high) < 0 || compare(low, high) == 0 &&
low.relation() == Condition.RelationType.GTE &&
high.relation() == Condition.RelationType.LTE;
int compared = compare(low, high);
if (compared > 0) {
return false;
}
if (compared == 0) {
return low.relation() == Condition.RelationType.GTE &&
high.relation() == Condition.RelationType.LTE;
}
return !emptyBooleanRange(low, high);
}

private static boolean emptyBooleanRange(Relation low, Relation high) {
if (!(low.value() instanceof Boolean) ||
!(high.value() instanceof Boolean)) {
return false;
}
return Boolean.FALSE.equals(low.value()) &&
Boolean.TRUE.equals(high.value()) &&
low.relation() == Condition.RelationType.GT &&
high.relation() == Condition.RelationType.LT;
}

private static boolean validEq(Relation eq, Relation low, Relation high) {
Expand Down Expand Up @@ -507,6 +524,9 @@ private static int compare(Relation first, Relation second) {
return NumericUtil.compareNumber(firstValue, (Number) secondValue);
} else if (firstValue instanceof Date && secondValue instanceof Date) {
return ((Date) firstValue).compareTo((Date) secondValue);
} else if (firstValue instanceof Boolean &&
secondValue instanceof Boolean) {
return Boolean.compare((Boolean) firstValue, (Boolean) secondValue);
} else {
throw new IllegalArgumentException(String.format("Can't compare between %s and %s",
first, second));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import org.apache.hugegraph.structure.HugeElement;
import org.apache.hugegraph.structure.HugeProperty;
import org.apache.hugegraph.type.HugeType;
import org.apache.hugegraph.type.define.DataType;
import org.apache.hugegraph.type.define.Directions;
import org.apache.hugegraph.type.define.HugeKeys;
import org.apache.hugegraph.util.CollectionUtil;
Expand Down Expand Up @@ -419,9 +420,9 @@ public static Condition convOr(HugeGraph graph,
return cond;
}

private static Condition.Relation convCompare2Relation(HugeGraph graph,
HugeType type,
HasContainer has) {
private static Condition convCompare2Relation(HugeGraph graph,
HugeType type,
HasContainer has) {
assert type.isGraph();
BiPredicate<?, ?> bp = has.getPredicate().getBiPredicate();
assert bp instanceof Compare;
Expand Down Expand Up @@ -459,16 +460,21 @@ private static Condition.Relation convCompare2SyspropRelation(HugeGraph graph,
}
}

private static Condition.Relation convCompare2UserpropRelation(HugeGraph graph,
HugeType type,
HasContainer has) {
private static Condition convCompare2UserpropRelation(HugeGraph graph,
HugeType type,
HasContainer has) {
BiPredicate<?, ?> bp = has.getPredicate().getBiPredicate();
assert bp instanceof Compare;

String key = has.getKey();
PropertyKey pkey = graph.propertyKey(key);
Id pkeyId = pkey.id();
Object value = validPropertyValue(has.getValue(), pkey);
if (pkey.dataType() == DataType.BOOLEAN &&
value instanceof Boolean) {
return convCompare2BooleanUserpropRelation((Compare) bp, pkeyId,
(Boolean) value);
}

switch ((Compare) bp) {
case eq:
Expand All @@ -488,6 +494,36 @@ 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.neq(key, value);
case gt:
return value ? impossibleBooleanCondition(key) :
Condition.eq(key, true);
case gte:
return value ? Condition.eq(key, true) :
Condition.in(key, ImmutableList.of(false, true));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Minor: gte(false) and lte(true) normalize to IN(false, true) — could simplify to "no filter"

When gte(false) or lte(true) is applied to a boolean property, the code normalizes to Condition.in(key, [false, true]). Since these two values are the entire boolean domain, this is semantically equivalent to "match everything" (no filter). The IN(false, true) condition still gets evaluated at runtime for every candidate element.

This isn't a bug, but for a performance-conscious path, you could detect this case and skip adding the condition entirely. Low priority since boolean properties are typically small cardinality.

case lt:
return value ? Condition.eq(key, false) :
impossibleBooleanCondition(key);
case lte:
return value ? Condition.in(key, ImmutableList.of(false, true)) :
Condition.eq(key, false);
default:
throw new AssertionError(compare);
}
}

private static Condition impossibleBooleanCondition(Id key) {
return Condition.and(Condition.eq(key, false),
Condition.eq(key, true));
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ impossibleBooleanCondition creates an AND node that may not be optimized away cleanly

This creates AND(eq(key, false), eq(key, true)) to represent an impossible condition. This is clever but has a potential issue: this compound Condition.And flows into the query pipeline where ConditionQueryFlatten.flattenRelations extracts only Relation conditions. An And node is NOT a Relation, so it goes into nonRelations and may bypass the merge/optimize logic entirely, potentially reaching the backend as a real filter condition.

Consider whether returning an empty ConditionQuery or a sentinel "always-false" condition would be cleaner. Alternatively, verify that the And(eq(false), eq(true)) is properly handled in the flatten/optimize pipeline without passing contradictory conditions to the backend.

(If Condition.in(key, ImmutableList.of()) — i.e., IN with empty list — is supported by the backend, that might be a cleaner way to express "matches nothing".)


private static Condition convRelationType2Relation(HugeGraph graph,
HugeType type,
HasContainer has) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7117,6 +7117,95 @@ public void testQueryEdgeWithNullablePropertyInCompositeIndex() {
Assert.assertEquals(1, (int) el.get(0).value("id"));
}

@Test
public void testQueryEdgeByBooleanRangePredicate() {
HugeGraph graph = graph();
initStrikeIndex();
graph.schema().indexLabel("strikeByArrested").onE("strike").secondary()
.by("arrested").create();

Vertex louise = graph.addVertex(T.label, "person", "name", "Louise",
"city", "Beijing", "age", 21);
Vertex sean = graph.addVertex(T.label, "person", "name", "Sean",
"city", "Beijing", "age", 23);
long current = System.currentTimeMillis();
louise.addEdge("strike", sean, "id", 1,
"timestamp", current, "place", "park",
"tool", "shovel", "reason", "jeer",
"arrested", false);
louise.addEdge("strike", sean, "id", 2,
"timestamp", current + 1, "place", "street",
"tool", "shovel", "reason", "jeer",
"arrested", true);

List<Edge> hasLtEdges = graph.traversal().E()
.has("arrested", P.lt(true))
.toList();
Assert.assertEquals(1, hasLtEdges.size());
Assert.assertEquals(1, (int) hasLtEdges.get(0).value("id"));

List<Edge> whereEdges = graph.traversal().E()
.where(__.has("arrested", P.lt(true)))
.toList();
Assert.assertEquals(1, whereEdges.size());
Assert.assertEquals(1, (int) whereEdges.get(0).value("id"));

List<Edge> matchEdges = graph.traversal().E()
.match(__.as("start")
.where(__.has("arrested",
P.lt(true)))
.as("matched"))
.<Edge>select("matched")
.toList();
Assert.assertEquals(1, matchEdges.size());
Assert.assertEquals(1, (int) matchEdges.get(0).value("id"));

List<Edge> hasLteFalseEdges = graph.traversal().E()
.has("arrested", P.lte(false))
.toList();
Assert.assertEquals(1, hasLteFalseEdges.size());
Assert.assertEquals(1, (int) hasLteFalseEdges.get(0).value("id"));

List<Edge> hasGtFalseEdges = graph.traversal().E()
.has("arrested", P.gt(false))
.toList();
Assert.assertEquals(1, hasGtFalseEdges.size());
Assert.assertEquals(2, (int) hasGtFalseEdges.get(0).value("id"));

List<Edge> hasGteTrueEdges = graph.traversal().E()
.has("arrested", P.gte(true))
.toList();
Assert.assertEquals(1, hasGteTrueEdges.size());
Assert.assertEquals(2, (int) hasGteTrueEdges.get(0).value("id"));

List<Edge> hasGteFalseEdges = graph.traversal().E()
.has("arrested", P.gte(false))
.toList();
Assert.assertEquals(2, hasGteFalseEdges.size());
Set<Integer> gteFalseIds = new HashSet<>();
for (Edge edge : hasGteFalseEdges) {
gteFalseIds.add(edge.value("id"));
}
Assert.assertEquals(ImmutableSet.of(1, 2), gteFalseIds);

List<Edge> hasLteTrueEdges = graph.traversal().E()
.has("arrested", P.lte(true))
.toList();
Assert.assertEquals(2, hasLteTrueEdges.size());
Set<Integer> lteTrueIds = new HashSet<>();
for (Edge edge : hasLteTrueEdges) {
lteTrueIds.add(edge.value("id"));
}
Assert.assertEquals(ImmutableSet.of(1, 2), lteTrueIds);

Assert.assertEquals(0, graph.traversal().E()
.has("arrested", P.lt(false))
.toList().size());
Assert.assertEquals(0, graph.traversal().E()
.has("arrested", P.gt(true))
.toList().size());
}

@Test
public void testQueryEdgeByPage() {
Assume.assumeTrue("Not support paging",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,5 +256,48 @@ 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));
query.query(Condition.lt(key, true));

List<ConditionQuery> queries = ConditionQueryFlatten.flatten(query);
Assert.assertEquals(0, queries.size());
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,32 @@ public void testConditionLte() {
});
}

@Test
public void testConditionBooleanRange() {
Condition lt = Condition.lt(HugeKeys.ID, true);
Assert.assertTrue(lt.test(false));
Assert.assertFalse(lt.test(true));

Condition lte = Condition.lte(HugeKeys.ID, false);
Assert.assertTrue(lte.test(false));
Assert.assertFalse(lte.test(true));

Condition gt = Condition.gt(HugeKeys.ID, false);
Assert.assertTrue(gt.test(true));
Assert.assertFalse(gt.test(false));

Condition gte = Condition.gte(HugeKeys.ID, true);
Assert.assertTrue(gte.test(true));
Assert.assertFalse(gte.test(false));

Assert.assertThrows(IllegalArgumentException.class, () -> {
Condition.lt(HugeKeys.ID, true).test(1);
}, e -> {
String err = "Can't compare between 1(Integer) and true(Boolean)";
Assert.assertEquals(err, e.getMessage());
});
}

@Test
public void testConditionNeq() {
Condition c1 = Condition.neq(HugeKeys.ID, 123);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ private static boolean equals(final Object first,
*
* @param first is actual value, might be Number/Date or String, It is
* probably that the `first` is serialized to String.
* @param second is value in query condition, must be Number/Date
* @param second is value in query condition, must be Number/Date/Boolean
* @return the value 0 if first is numerically equal to second;
* a value less than 0 if first is numerically less than
* second; and a value greater than 0 if first is
Expand All @@ -450,6 +450,8 @@ private static int compare(final Object first, final Object second) {
(Number) second);
} else if (second instanceof Date) {
return compareDate(first, (Date) second);
} else if (second instanceof Boolean) {
return compareBoolean(first, (Boolean) second);
}

throw new IllegalArgumentException(String.format(
Expand All @@ -472,6 +474,20 @@ private static int compareDate(Object first, Date second) {
second, second.getClass().getSimpleName()));
}

private static int compareBoolean(Object first, Boolean second) {
if (first == null) {
first = false;
}
if (first instanceof Boolean) {
return Boolean.compare((Boolean) first, second);
}

throw new IllegalArgumentException(String.format(
"Can't compare between %s(%s) and %s(%s)",
first, first.getClass().getSimpleName(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Duplicate compareBoolean in two Condition classes

The exact same compareBoolean method is added in both:

  • hugegraph-server/.../backend/query/Condition.java
  • hugegraph-struct/.../query/Condition.java

This is copy-paste duplication. If the null-handling semantics or error messages need to change later, both copies must be kept in sync. Is there a shared utility class or base class where this could live instead? If duplication is intentional (module isolation), consider at least adding a test for the hugegraph-struct version as well.

second, second.getClass().getSimpleName()));
}

public static List<String> tokenize(String str) {
final ArrayList<String> tokens = new ArrayList<>();
int previous = 0;
Expand Down
Loading