-
Notifications
You must be signed in to change notification settings - Fork 597
fix(core): normalize boolean range predicates in Gremlin filters #2991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
98fef9a
dc441b6
4cd2049
d92050f
a07f43f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This change from 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:
Was this intentional or is the fix only meant for the boolean case? |
||
| } | ||
|
|
||
| private static Relations optimizeRelations(Relations relations) { | ||
|
|
@@ -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) { | ||
|
|
@@ -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)); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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; | ||
|
|
@@ -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: | ||
|
|
@@ -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)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Minor: When 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)); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This creates Consider whether returning an empty (If |
||
|
|
||
| private static Condition convRelationType2Relation(HugeGraph graph, | ||
| HugeType type, | ||
| HasContainer has) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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( | ||
|
|
@@ -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(), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The exact same
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 |
||
| second, second.getClass().getSimpleName())); | ||
| } | ||
|
|
||
| public static List<String> tokenize(String str) { | ||
| final ArrayList<String> tokens = new ArrayList<>(); | ||
| int previous = 0; | ||
|
|
||
There was a problem hiding this comment.
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 surprisingWhen
firstis null, it's silently treated asfalse. This is asymmetric withcompareNumberandcompareDatewhich don't do null coercion. If a property is absent (null), treating it asfalsecould cause incorrect query results — e.g., a vertex with no boolean property set would matchlte(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.