imbajin commented on code in PR #2991:
URL: https://github.com/apache/hugegraph/pull/2991#discussion_r3105758352
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/ConditionQueryFlatten.java:
##########
@@ -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();
Review Comment:
‼️ **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?
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -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));
+ 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));
+ }
Review Comment:
⚠️ **`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".)
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/Condition.java:
##########
@@ -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;
Review Comment:
⚠️ **`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.
##########
hugegraph-struct/src/main/java/org/apache/hugegraph/query/Condition.java:
##########
@@ -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(),
Review Comment:
⚠️ **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.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -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));
Review Comment:
🧹 **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.
--
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]
