mschmidt00 commented on a change in pull request #1553: URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r790386184
########## File path: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/TernaryBooleanLogicsTest.java ########## @@ -0,0 +1,317 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal.step; + +import org.apache.tinkerpop.gremlin.FeatureRequirement; +import org.apache.tinkerpop.gremlin.process.AbstractGremlinProcessTest; +import org.apache.tinkerpop.gremlin.process.GremlinProcessRunner; +import org.apache.tinkerpop.gremlin.process.traversal.P; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.hamcrest.core.Is; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.util.function.BiFunction; + +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.*; +import static org.apache.tinkerpop.gremlin.structure.Graph.Features.GraphFeatures; +import static org.hamcrest.MatcherAssert.assertThat; + +@RunWith(GremlinProcessRunner.class) +public class TernaryBooleanLogicsTest extends AbstractGremlinProcessTest { + + /** + * NaN comparisons always produce UNDEF comparison, reducing to FALSE in ternary->binary reduction. + */ + @Test + @FeatureRequirement(featureClass = GraphFeatures.class, feature = GraphFeatures.FEATURE_ORDERABILITY_SEMANTICS) + public void testCompareNaN() { + // NaN vs. NaN + // P.eq + checkHasNext(false, g.inject(Double.NaN).is(P.eq(Double.NaN))); + // P.neq + checkHasNext(true, g.inject(Double.NaN).is(P.neq(Double.NaN))); + // P.lt + checkHasNext(false, g.inject(Double.NaN).is(P.lt(Double.NaN))); + // P.lte + checkHasNext(false, g.inject(Double.NaN).is(P.lte(Double.NaN))); + // P.gt + checkHasNext(false, g.inject(Double.NaN).is(P.gt(Double.NaN))); + // P.gte + checkHasNext(false, g.inject(Double.NaN).is(P.gte(Double.NaN))); Review comment: Does it make sense to also add some tests for nested predicates? Something like "1<2 == 2<3", with more interesting values such as NULL's an NaN's? ########## File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/FilterStep.java ########## @@ -34,9 +36,22 @@ public FilterStep(final Traversal.Admin traversal) { @Override protected Traverser.Admin<S> processNextStart() { while (true) { - final Traverser.Admin<S> traverser = this.starts.next(); - if (this.filter(traverser)) - return traverser; + try { + final Traverser.Admin<S> traverser = this.starts.next(); + if (this.filter(traverser)) + return traverser; + } catch (GremlinTypeErrorException ex) { + if (this instanceof BinaryReductionStep || getTraversal().getParent() == EmptyStep.instance()) { Review comment: Yep, agreed this looks critical. I am wondering though why we couldn't *always* forward the exception here and catch it in the target steps where we want to transform the exception into FALSE. Might be a little more code on the one hand, but a little more explicit and safe (in the sense that it is "guaranteed top-level") on the other hand. Give it a thought, but certainly trusting in @spmallette and you in case you both agree this is failsafe. ########## File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/OrderabilityComparator.java ########## @@ -103,11 +101,20 @@ private static final Comparator<Map.Entry> entryComparator = Comparator.<Map.Entry,Object>comparing(Map.Entry::getKey, INSTANCE).thenComparing(Map.Entry::getValue, INSTANCE); + + + private static boolean bothAreComparableAndOfSameType(Object first, Object second) { Review comment: Name is a bit misleading, plus I do not fully understand how this works. Is the idea here that numbers would be considered the same type because one class is instance of another one? In any case, some documentation and examples would be helpful. ########## File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Contains.java ########## @@ -52,9 +52,19 @@ within { @Override public boolean test(final Object first, final Collection second) { - return first instanceof Number - ? IteratorUtils.anyMatch(second.iterator(), o -> Compare.eq.test(first, o)) - : second.contains(first); + GremlinTypeErrorException typeError = null; + for (final Object o : second) { + try { + if (Compare.eq.test(first, o)) + return true; + } catch (GremlinTypeErrorException ex) { + // hold onto it until the end in case any other arguments evaluate to TRUE Review comment: +1, makes sense ########## File path: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTest.java ########## @@ -158,6 +163,20 @@ {TextP.containing("o").and(P.gte("j")).and(TextP.endingWith("ko")), "josh", false}, {TextP.containing("o").and(P.gte("j").and(TextP.endingWith("ko"))), "marko", true}, {TextP.containing("o").and(P.gte("j").and(TextP.endingWith("ko"))), "josh", false}, + + // type errors + {P.outside(Double.NaN, Double.NaN), 0, GremlinTypeErrorException.class}, + {P.inside(-1, Double.NaN), 0, GremlinTypeErrorException.class}, + {P.inside(Double.NaN, 1), 0, GremlinTypeErrorException.class}, + {TextP.containing(null), "abc", GremlinTypeErrorException.class}, + {TextP.containing("abc"), null, GremlinTypeErrorException.class}, + {TextP.containing(null), null, GremlinTypeErrorException.class}, + {TextP.startingWith(null), "abc", GremlinTypeErrorException.class}, + {TextP.startingWith("abc"), null, GremlinTypeErrorException.class}, + {TextP.startingWith(null), null, GremlinTypeErrorException.class}, + {TextP.endingWith(null), "abc", GremlinTypeErrorException.class}, + {TextP.endingWith("abc"), null, GremlinTypeErrorException.class}, + {TextP.endingWith(null), null, GremlinTypeErrorException.class}, Review comment: nit: recommend a few tests with other (i.e., composite) types ########## File path: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/CompareTest.java ########## @@ -95,7 +87,96 @@ {Compare.gte, new B(), new C(), true}, {Compare.lte, new B(), new D(), true}, {Compare.lte, new C(), new C(), true}, - {Compare.lte, new D(), new D(), true} + {Compare.lte, new D(), new D(), true}, + + // type promotion + {Compare.eq, 1, 1.0d, true}, + {Compare.neq, 1, 1.0d, false}, + {Compare.lt, 1, 1.0d, false}, + {Compare.lte, 1, 1.0d, true}, + {Compare.gt, 1, 1.0d, false}, + {Compare.gte, 1, 1.0d, true}, + + // Incomparable types produce ERROR + {Compare.gt, 23, "23", GremlinTypeErrorException.class}, + {Compare.gte, 23, "23", GremlinTypeErrorException.class}, + {Compare.lt, 23, "23", GremlinTypeErrorException.class}, + {Compare.lte, 23, "23", GremlinTypeErrorException.class}, + {Compare.lte, new CompareTest.A(), new CompareTest.B(), GremlinTypeErrorException.class}, + {Compare.lte, new CompareTest.B(), new CompareTest.A(), GremlinTypeErrorException.class}, + {Compare.lte, new CompareTest.C(), new CompareTest.D(), GremlinTypeErrorException.class}, + {Compare.gte, new Object(), new Object(), GremlinTypeErrorException.class}, + {Compare.gte, new Object(), new Object(), GremlinTypeErrorException.class}, + {Compare.gte, new Object(), new Object(), GremlinTypeErrorException.class}, + + /* + * NaN has pretty much the same comparability behavior against any argument (including itself): + * P.eq(NaN, any) = FALSE + * P.neq(NaN, any) = TRUE + * P.lt/lte/gt/gte(NaN, any) = ERROR -> FALSE + */ + {Compare.eq, NaN, NaN, false}, + {Compare.neq, NaN, NaN, true}, + {Compare.gt, NaN, NaN, GremlinTypeErrorException.class}, + {Compare.gte, NaN, NaN, GremlinTypeErrorException.class}, + {Compare.lt, NaN, NaN, GremlinTypeErrorException.class}, + {Compare.lte, NaN, NaN, GremlinTypeErrorException.class}, + + {Compare.eq, NaN, 0, false}, + {Compare.neq, NaN, 0, true}, + {Compare.gt, NaN, 0, GremlinTypeErrorException.class}, + {Compare.gte, NaN, 0, GremlinTypeErrorException.class}, + {Compare.lt, NaN, 0, GremlinTypeErrorException.class}, + {Compare.lte, NaN, 0, GremlinTypeErrorException.class}, + + {Compare.eq, NaN, "foo", false}, + {Compare.neq, NaN, "foo", true}, + {Compare.gt, NaN, "foo", GremlinTypeErrorException.class}, + {Compare.gte, NaN, "foo", GremlinTypeErrorException.class}, + {Compare.lt, NaN, "foo", GremlinTypeErrorException.class}, + {Compare.lte, NaN, "foo", GremlinTypeErrorException.class}, + + /* + * We consider null to be in its own type space, and thus not comparable (lt/lte/gt/gte) with + * anything other than null: + * + * P.eq(null, any non-null) = FALSE + * P.neq(null, any non-null) = TRUE + * P.lt/lte/gt/gte(null, any non-null) = ERROR -> FALSE + */ + {Compare.eq, null, null, true}, + {Compare.neq, null, null, false}, + {Compare.gt, null, null, false}, + {Compare.gte, null, null, true}, + {Compare.lt, null, null, false}, + {Compare.lte, null, null, true}, + + {Compare.eq, "foo", null, false}, + {Compare.neq, "foo", null, true}, + {Compare.gt, "foo", null, GremlinTypeErrorException.class}, + {Compare.gte, "foo", null, GremlinTypeErrorException.class}, + {Compare.lt, "foo", null, GremlinTypeErrorException.class}, + {Compare.lte, "foo", null, GremlinTypeErrorException.class}, + + {Compare.eq, null, 1, false}, + {Compare.eq, 1, null, false}, + {Compare.neq, null, 1, true}, + {Compare.neq, 1, null, true}, + {Compare.gt, null, 1, GremlinTypeErrorException.class}, + {Compare.gt, 1, null, GremlinTypeErrorException.class}, + {Compare.gte, null, 1, GremlinTypeErrorException.class}, + {Compare.gte, 1, null, GremlinTypeErrorException.class}, + {Compare.lt, null, 1, GremlinTypeErrorException.class}, + {Compare.lt, 1, null, GremlinTypeErrorException.class}, + {Compare.lte, null, 1, GremlinTypeErrorException.class}, + {Compare.lte, 1, null, GremlinTypeErrorException.class}, + Review comment: Should we also add some tests for composite types? Apart from this being a gap, we should assert that lists etc. "inherit" the comparison semantics from the atomic cases (where needed, recursively). Writing test coverage is certainly an open-ended effort, but having a handful of tests for lists, maps, etc. would be important. ########## File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Compare.java ########## @@ -78,25 +77,19 @@ public Compare negate() { }, /** - * Evaluates if the first object is greater than the second. If both are of type {@link Number}, {@link NumberHelper} - * will be used for the comparison, thus enabling the comparison of only values, ignoring the number types. + * Evaluates if the first object is greater than the second per Gremlin Comparison semantics. * * @since 3.0.0-incubating */ gt { @Override public boolean test(final Object first, final Object second) { - if (null != first && null != second) { - if (bothAreNumber(first, second)) { - return NumberHelper.compare((Number) first, (Number) second) > 0; - } - if (bothAreComparableAndOfSameType(first, second)) { - return ((Comparable) first).compareTo(second) > 0; - } - throwException(first, second); - } - - return false; + // NaN takes precedence + if (eitherAreNaN(first, second)) + throwTypeError(); Review comment: Haven't gotten to the implementation yet, but I assume this throws an exception internally. Personally, I'm not a big fan of using exceptions in (somewhat) regular code paths, but this is admittedly somewhere in-between (in the sense that it would only happen in edge cases). Something to think about, but if you feel that's the best way to implement it I'm fine going forward with it. ########## File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/OrderabilityComparator.java ########## @@ -167,6 +174,20 @@ private static Object transform(Object o) { return o; } + /** + * Return true if the two objects are of the same comparison type, even if they are not the same Class. + */ + public static boolean comparable(final Object f, final Object s) { + if (f == null || s == null) + return f == s; // both in the null space + + final Type ft = Type.type(f); + final Type st = Type.type(s); + + // if they're both in the unknown type space then only return true if they are naturally Comparable + return ft == Type.Unknown && st == Type.Unknown ? bothAreComparableAndOfSameType(f, s) : ft == st; Review comment: Don't understand this, could you explain? What is the "unknown type space", i.e. why would we have unknown types at runtime? ########## File path: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/CompareTest.java ########## @@ -95,7 +87,96 @@ {Compare.gte, new B(), new C(), true}, {Compare.lte, new B(), new D(), true}, {Compare.lte, new C(), new C(), true}, - {Compare.lte, new D(), new D(), true} + {Compare.lte, new D(), new D(), true}, + + // type promotion + {Compare.eq, 1, 1.0d, true}, + {Compare.neq, 1, 1.0d, false}, + {Compare.lt, 1, 1.0d, false}, + {Compare.lte, 1, 1.0d, true}, + {Compare.gt, 1, 1.0d, false}, + {Compare.gte, 1, 1.0d, true}, + + // Incomparable types produce ERROR + {Compare.gt, 23, "23", GremlinTypeErrorException.class}, + {Compare.gte, 23, "23", GremlinTypeErrorException.class}, + {Compare.lt, 23, "23", GremlinTypeErrorException.class}, + {Compare.lte, 23, "23", GremlinTypeErrorException.class}, + {Compare.lte, new CompareTest.A(), new CompareTest.B(), GremlinTypeErrorException.class}, + {Compare.lte, new CompareTest.B(), new CompareTest.A(), GremlinTypeErrorException.class}, + {Compare.lte, new CompareTest.C(), new CompareTest.D(), GremlinTypeErrorException.class}, + {Compare.gte, new Object(), new Object(), GremlinTypeErrorException.class}, + {Compare.gte, new Object(), new Object(), GremlinTypeErrorException.class}, + {Compare.gte, new Object(), new Object(), GremlinTypeErrorException.class}, + + /* + * NaN has pretty much the same comparability behavior against any argument (including itself): + * P.eq(NaN, any) = FALSE + * P.neq(NaN, any) = TRUE + * P.lt/lte/gt/gte(NaN, any) = ERROR -> FALSE + */ + {Compare.eq, NaN, NaN, false}, Review comment: Those are still TBD I assume (did not review the NaN tests for now). ########## File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Text.java ########## @@ -117,6 +121,8 @@ public Text negate() { containing { @Override public boolean test(final String value, final String search) { + if (value == null || search == null) Review comment: nit: seeing this if (...) => throw for the third time, consider factoring it into a throwTypeErrorIfNull() method? ########## File path: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/CompareTest.java ########## @@ -95,7 +87,96 @@ {Compare.gte, new B(), new C(), true}, {Compare.lte, new B(), new D(), true}, {Compare.lte, new C(), new C(), true}, - {Compare.lte, new D(), new D(), true} + {Compare.lte, new D(), new D(), true}, + + // type promotion + {Compare.eq, 1, 1.0d, true}, + {Compare.neq, 1, 1.0d, false}, + {Compare.lt, 1, 1.0d, false}, + {Compare.lte, 1, 1.0d, true}, + {Compare.gt, 1, 1.0d, false}, + {Compare.gte, 1, 1.0d, true}, Review comment: nit: recommend adding some tests for +/- INF values as well ########## File path: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/ContainsTest.java ########## @@ -47,7 +52,12 @@ {Contains.within, 100, Arrays.asList(1, 2, 3, 4, 10), false}, {Contains.without, 10L, Arrays.asList(1, 2, 3, 4, 10), false}, {Contains.within, "test", Arrays.asList(1, 2, 3, "test", 10), true}, - {Contains.without, "testing", Arrays.asList(1, 2, 3, "test", 10), true} + {Contains.without, "testing", Arrays.asList(1, 2, 3, "test", 10), true}, + + {Contains.within, Double.NaN, Arrays.asList(Double.NaN), false}, + {Contains.within, Double.NaN, Arrays.asList(0, Double.NaN), false}, + {Contains.without, Double.NaN, Arrays.asList(Double.NaN), true}, + {Contains.without, Double.NaN, Arrays.asList(0, Double.NaN), true}, Review comment: Could add some exception tests here as well? ########## File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Contains.java ########## @@ -52,9 +52,19 @@ within { @Override public boolean test(final Object first, final Collection second) { - return first instanceof Number - ? IteratorUtils.anyMatch(second.iterator(), o -> Compare.eq.test(first, o)) - : second.contains(first); + GremlinTypeErrorException typeError = null; Review comment: Bit surprised to see you made changes to within but not to without -- wouldn't those also be required (or is the semantics for without that you would *always* forward an error -- the alternative would be to never forward it, I assume)? ########## File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Text.java ########## @@ -37,6 +37,8 @@ startingWith { @Override public boolean test(final String value, final String prefix) { Review comment: I assume the cases where value or prefix are NOT a valid string would not even end up here but trigger an exception before already? Just in case this is not covered, I'd recommend some test cases for both value and prefix not being a string. Same for the other string functions below. ########## File path: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/ConnectiveTest.java ########## @@ -0,0 +1,161 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.process.traversal; + +import org.apache.tinkerpop.gremlin.process.traversal.util.AndP; +import org.apache.tinkerpop.gremlin.process.traversal.util.OrP; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.runners.Enclosed; +import org.junit.rules.ExpectedException; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import java.util.Arrays; + +import static org.junit.Assert.assertEquals; + +/** + * @author Mike Personick (http://github.com/mikepersonick) + */ +@RunWith(Enclosed.class) +public class ConnectiveTest { + + private static final Object VAL = 1; + private static final P TRUE = P.eq(1); + private static final P FALSE = P.gt(1); + private static final P ERROR = P.lt(Double.NaN); Review comment: nice test! ########## File path: gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/CompareTest.java ########## @@ -95,7 +87,96 @@ {Compare.gte, new B(), new C(), true}, {Compare.lte, new B(), new D(), true}, {Compare.lte, new C(), new C(), true}, - {Compare.lte, new D(), new D(), true} + {Compare.lte, new D(), new D(), true}, + + // type promotion + {Compare.eq, 1, 1.0d, true}, + {Compare.neq, 1, 1.0d, false}, + {Compare.lt, 1, 1.0d, false}, + {Compare.lte, 1, 1.0d, true}, + {Compare.gt, 1, 1.0d, false}, + {Compare.gte, 1, 1.0d, true}, + + // Incomparable types produce ERROR + {Compare.gt, 23, "23", GremlinTypeErrorException.class}, + {Compare.gte, 23, "23", GremlinTypeErrorException.class}, + {Compare.lt, 23, "23", GremlinTypeErrorException.class}, + {Compare.lte, 23, "23", GremlinTypeErrorException.class}, + {Compare.lte, new CompareTest.A(), new CompareTest.B(), GremlinTypeErrorException.class}, + {Compare.lte, new CompareTest.B(), new CompareTest.A(), GremlinTypeErrorException.class}, + {Compare.lte, new CompareTest.C(), new CompareTest.D(), GremlinTypeErrorException.class}, + {Compare.gte, new Object(), new Object(), GremlinTypeErrorException.class}, + {Compare.gte, new Object(), new Object(), GremlinTypeErrorException.class}, + {Compare.gte, new Object(), new Object(), GremlinTypeErrorException.class}, + + /* + * NaN has pretty much the same comparability behavior against any argument (including itself): + * P.eq(NaN, any) = FALSE + * P.neq(NaN, any) = TRUE + * P.lt/lte/gt/gte(NaN, any) = ERROR -> FALSE + */ + {Compare.eq, NaN, NaN, false}, + {Compare.neq, NaN, NaN, true}, + {Compare.gt, NaN, NaN, GremlinTypeErrorException.class}, + {Compare.gte, NaN, NaN, GremlinTypeErrorException.class}, + {Compare.lt, NaN, NaN, GremlinTypeErrorException.class}, + {Compare.lte, NaN, NaN, GremlinTypeErrorException.class}, + + {Compare.eq, NaN, 0, false}, + {Compare.neq, NaN, 0, true}, + {Compare.gt, NaN, 0, GremlinTypeErrorException.class}, + {Compare.gte, NaN, 0, GremlinTypeErrorException.class}, + {Compare.lt, NaN, 0, GremlinTypeErrorException.class}, + {Compare.lte, NaN, 0, GremlinTypeErrorException.class}, + + {Compare.eq, NaN, "foo", false}, + {Compare.neq, NaN, "foo", true}, + {Compare.gt, NaN, "foo", GremlinTypeErrorException.class}, + {Compare.gte, NaN, "foo", GremlinTypeErrorException.class}, + {Compare.lt, NaN, "foo", GremlinTypeErrorException.class}, + {Compare.lte, NaN, "foo", GremlinTypeErrorException.class}, + + /* + * We consider null to be in its own type space, and thus not comparable (lt/lte/gt/gte) with + * anything other than null: + * + * P.eq(null, any non-null) = FALSE + * P.neq(null, any non-null) = TRUE + * P.lt/lte/gt/gte(null, any non-null) = ERROR -> FALSE + */ + {Compare.eq, null, null, true}, + {Compare.neq, null, null, false}, + {Compare.gt, null, null, false}, + {Compare.gte, null, null, true}, + {Compare.lt, null, null, false}, + {Compare.lte, null, null, true}, + + {Compare.eq, "foo", null, false}, + {Compare.neq, "foo", null, true}, + {Compare.gt, "foo", null, GremlinTypeErrorException.class}, + {Compare.gte, "foo", null, GremlinTypeErrorException.class}, + {Compare.lt, "foo", null, GremlinTypeErrorException.class}, + {Compare.lte, "foo", null, GremlinTypeErrorException.class}, + + {Compare.eq, null, 1, false}, + {Compare.eq, 1, null, false}, + {Compare.neq, null, 1, true}, + {Compare.neq, 1, null, true}, + {Compare.gt, null, 1, GremlinTypeErrorException.class}, + {Compare.gt, 1, null, GremlinTypeErrorException.class}, + {Compare.gte, null, 1, GremlinTypeErrorException.class}, + {Compare.gte, 1, null, GremlinTypeErrorException.class}, + {Compare.lt, null, 1, GremlinTypeErrorException.class}, + {Compare.lt, 1, null, GremlinTypeErrorException.class}, + {Compare.lte, null, 1, GremlinTypeErrorException.class}, + {Compare.lte, 1, null, GremlinTypeErrorException.class}, + + {Compare.eq, NaN, null, false}, + {Compare.neq, NaN, null, true}, + {Compare.gt, NaN, null, GremlinTypeErrorException.class}, + {Compare.gte, NaN, null, GremlinTypeErrorException.class}, + {Compare.lt, NaN, null, GremlinTypeErrorException.class}, + {Compare.lte, NaN, null, GremlinTypeErrorException.class}, })); Review comment: Another thing that I am missing is tests (as well as changes) for negation. I was a bit surprised to realize there is no P.not predicate (wondering if it should be added), but only a not() step. This sort of implies that there's no nesting of negation at predicate level, and I assume that's the reason why we don't need any changes? -- 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]
