You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2022/01/21 20:49:35 UTC
[GitHub] [tinkerpop] mikepersonick opened a new pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
mikepersonick opened a new pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553
https://issues.apache.org/jira/browse/TINKERPOP-2687
Ready for initial review, not ready for merge. I still need to add support for Contains and TextP, additional test coverage, and documentation, but I'd like an initial sanity check on the ternary boolean logics tests. I also think we need further discussion on NaN.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r795529737
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AndStep.java
##########
@@ -33,10 +34,18 @@ public AndStep(final Traversal.Admin traversal, final Traversal<S, ?>... travers
@Override
protected boolean filter(final Traverser.Admin<S> traverser) {
+ GremlinTypeErrorException typeError = null;
for (final Traversal.Admin<S, ?> traversal : this.traversals) {
- if (!TraversalUtil.test(traverser, traversal))
- return false;
+ try {
+ if (!TraversalUtil.test(traverser, traversal))
+ return false;
+ } catch (GremlinTypeErrorException ex) {
+ // hold onto it until the end in case any other arguments evaluate to FALSE
Review comment:
aahh... the `NOT(P.lt(NaN)) = TRUE` is a really good example to explain this. Thank you Mike.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mschmidt00 commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r790770528
##########
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 don't see how anything but a string could end up here in the reference implementation. The only type error possible is NULL
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r794492420
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/BinaryReductionStep.java
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.filter;
+
+import org.apache.tinkerpop.gremlin.process.traversal.GremlinTypeErrorException;
+
+/**
+ * Any {@link FilterStep} marked as a BinaryReductionStep will catch {@link GremlinTypeErrorException}s produced by
Review comment:
1. Child traversals need an opportunity to reduce from ternary to binary. We can't say all FilterSteps convert, because of (2) propagation.
2. We need to propagate `ERROR` up to the last minute before reducing to `FALSE`, since it can be combined in interesting ways via `AND/OR/NOT`.
It seems extremely unlikely to me there would be another step added that would need to worry about this. Any normal filter step would not. It's a quasi-top level filter like .where and .filter that might need to worry about binary reduction in the context of a child traversal.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r790775363
##########
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:
p.not is tested in the Xor connective test below, but I will be adding significant additional test coverage so I will add some more not testing.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r794489192
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AndStep.java
##########
@@ -33,10 +34,18 @@ public AndStep(final Traversal.Admin traversal, final Traversal<S, ?>... travers
@Override
protected boolean filter(final Traverser.Admin<S> traverser) {
+ GremlinTypeErrorException typeError = null;
for (final Traversal.Admin<S, ?> traversal : this.traversals) {
- if (!TraversalUtil.test(traverser, traversal))
- return false;
+ try {
+ if (!TraversalUtil.test(traverser, traversal))
+ return false;
+ } catch (GremlinTypeErrorException ex) {
+ // hold onto it until the end in case any other arguments evaluate to FALSE
Review comment:
Check out the table in the semantics document (in this PR). There is a description of AND/OR/XOR semantics:
`ERROR && FALSE -> FALSE`
`AND` might be wrapped by an operator that handles ERROR differently from FALSE (e.g. `NOT`), so we need to wait on propagating the ERROR until we've seen all the arguments.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette merged pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
spmallette merged pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mschmidt00 commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mschmidt00 commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r793249337
##########
File path: docs/src/dev/provider/gremlin-semantics.asciidoc
##########
@@ -316,59 +337,58 @@ gremlin> g.E().has("weight", gt(1))
* For numbers,
** it should be aligned to equality conceptually as far as type promotion is concerned. e.g. `1.0 < 2 < 3L`
-* Comparison should not result in undefined behavior, but can return `nulltype` if and only if we are comparing
-incomparable data types. How this `nulltype` result is handled is Graph provider dependent.
+* Comparison should not result in undefined behavior, but can return `ERROR` if we are comparing
+different data types. How this `ERROR` result is handled is Graph provider dependent. The reference implementation
Review comment:
nit: "if we are comparing different data types (and for comparisons with NaN values)".
##########
File path: docs/src/dev/provider/gremlin-semantics.asciidoc
##########
@@ -386,33 +406,62 @@ IDs are of any primitive types defined, so comparability for a corresponding typ
Comparability of String applies.
+===== Iterable (Path, List, Set, Map)
+
+* Iterables must be of the same type, otherwise `ERROR`.
+* Iterables are compared via their natural iteration order.
+* Elements are compared element by element according to these semantics from the two iterations being compared.
+* Empty iterations are equal and are less than non-empty iterations.
+* If iteration `A` exhausts its elements before iteration `B` then `A < B`.
+
===== Path
-It is not comparable, throw an Exception.
+* Only comparable to other Paths, otherwise `ERROR`.
+* Iterable semantics apply.
+
+===== Set
+
+* Only comparable to other Sets, otherwise `ERROR`.
+* Iterable semantics apply.
===== List
-It is not comparable, throw an Exception.
+* Only comparable to other Lists, otherwise `ERROR`.
+* Iterable semantics apply.
===== Map
-It is not comparable, throw an Exception.
+* Only comparable to other Maps, otherwise `ERROR`.
+* Iterable semantics apply (using `Map.entrySet()`)
+* Map entries are compared first by key, then by value according to these semantics.
-===== Map.Entry
-
-It is not comparable, throw an Exception.
+=== Mapping for P
-===== Set
+The following table maps the notions proposed above to the various `P` operators:
Review comment:
nice!
##########
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:
So you're saying it would be a type error already at parsing time. Talking about some sort of "startingWith(1)" somewhere in the query.
##########
File path: docs/src/dev/provider/gremlin-semantics.asciidoc
##########
@@ -386,33 +406,62 @@ IDs are of any primitive types defined, so comparability for a corresponding typ
Comparability of String applies.
+===== Iterable (Path, List, Set, Map)
+
+* Iterables must be of the same type, otherwise `ERROR`.
+* Iterables are compared via their natural iteration order.
Review comment:
As per our discussion, you may want to add that the behavior for iterables without inherent order (such as maps) is implementation specific (and using the iteration order is what TP does)?
##########
File path: docs/src/dev/provider/gremlin-semantics.asciidoc
##########
@@ -423,7 +472,7 @@ For a different type, we have a dedicated order as described in the section belo
To sort across any types of values, we define the order between each type as follows:
(In this order, ID, label, property key and property value are considered as a part of primitive types)
-* `nulltype`
+* `null`
Review comment:
nit: I would prefer reverting that and leaving "nulltype" -- null is really the (one and only) instance of nulltype, and this list is at type level
##########
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:
Understand. @spmallette please have a quick look at this one
##########
File path: docs/src/dev/provider/gremlin-semantics.asciidoc
##########
@@ -294,12 +294,33 @@ Equivalence is identical to Equality, except for the cases listed below.
Comparability and orderability can be understood as the "dual" concepts of equality and equivalence for range
comparisons (rather than exact comparison). For the two values of the same type (except for NaN), comparability is
stronger than orderability in the sense that everything that every order between two values that holds `true` w.r.t.
-comparability also holds `true` w.r.t. orderability, but not vice versa. Comparability is what is being used in range
-predicates. It is restricted to comparison within the same type or, for numerics, class of types; comparability is
-complete within a given type, but returns `nulltype` if the two types are considered incomparable (e.g., an integer
-cannot be compared to a string). Orderability fills these gaps, by providing a stable sort order over mixed type
-results; it is consistent with comparability within a type, and complete both within and across types, i.e. it will
-never return `nulltype` or throw an exception.
+comparability also holds `true` w.r.t. orderability, but not vice versa.
+
+Comparability is what is being used in range predicates. It is restricted to comparison within the same type or,
+for numerics, class of types; comparability is complete within a given type, but returns `ERROR` if the two types are
+considered incomparable (e.g., an integer cannot be compared to a string). `ERROR` is the third option in a ternary
+boolean logics system for boolean value expression evaluation. `ERROR` is an internal representation only and will not
+be propagated back to the client as an exception - it will eventually hit a binary reduction operation and be reduced
+to `false`. Before that happens though, it will be treated as its own boolean value that can be used in other boolean
+value expressions, such as `AND`/`OR`:
+
+|===
+|A|B|=>|AND|OR
Review comment:
Maybe add the XOR semantics 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:
ack
##########
File path: docs/src/dev/provider/gremlin-semantics.asciidoc
##########
@@ -294,12 +294,33 @@ Equivalence is identical to Equality, except for the cases listed below.
Comparability and orderability can be understood as the "dual" concepts of equality and equivalence for range
comparisons (rather than exact comparison). For the two values of the same type (except for NaN), comparability is
stronger than orderability in the sense that everything that every order between two values that holds `true` w.r.t.
-comparability also holds `true` w.r.t. orderability, but not vice versa. Comparability is what is being used in range
-predicates. It is restricted to comparison within the same type or, for numerics, class of types; comparability is
-complete within a given type, but returns `nulltype` if the two types are considered incomparable (e.g., an integer
-cannot be compared to a string). Orderability fills these gaps, by providing a stable sort order over mixed type
-results; it is consistent with comparability within a type, and complete both within and across types, i.e. it will
-never return `nulltype` or throw an exception.
+comparability also holds `true` w.r.t. orderability, but not vice versa.
Review comment:
In the equivalence section above (which you did not change), we should call out that NaN == NaN for equivalence (as a difference towards equality) -- may have missed it, but I couldn't find that stated explicitly.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r794550121
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/BinaryReductionStep.java
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.filter;
+
+import org.apache.tinkerpop.gremlin.process.traversal.GremlinTypeErrorException;
+
+/**
+ * Any {@link FilterStep} marked as a BinaryReductionStep will catch {@link GremlinTypeErrorException}s produced by
Review comment:
Understood. Resolving this thread.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r794472972
##########
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:
I agree with what Mike is saying. When a traversal is executed, exceptions are directly propagated outside to the server and from server to the client. One option for "guaranteed-top-level" would be catching this at server and converting to a result but I would not be in favour of that approach since it leaks the query execution semantics to the server and introduce strong coupling.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r794716331
##########
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:
yes - i would expect a parser error at most here.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r790773176
##########
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:
The reference implementation doesn't really work like that. There is nothing to forward it to. This FilterStep (if it's at the top level of the query) has the solution and makes the call whether to filter it or not. It is not embedded in a target, the solution has already left the target. If an exception is thrown here it goes straight to the client, there is no where higher to catch it inside of the query execution layer. For Graph providers that push this down obviously that 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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r790153892
##########
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
Review comment:
fix whitespace
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r791791374
##########
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:
The whole system is built around Java Predicates. Tearing that down would be... a lot. ERROR is propagated by Exception and converted to FALSE by FilterStep (our BIN operator).
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r793828000
##########
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:
I'm not sure we can - Contains uses P.eq/neq equality semantics. No ERRORs
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r794484874
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/BinaryReductionStep.java
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.filter;
+
+import org.apache.tinkerpop.gremlin.process.traversal.GremlinTypeErrorException;
+
+/**
+ * Any {@link FilterStep} marked as a BinaryReductionStep will catch {@link GremlinTypeErrorException}s produced by
Review comment:
I am a bit apprehensive about maintainability of BinaryReductionStep. If a new step is added in future, it would be upto the implementors good intentions to add this interface. Instead, can we make it more explicit?
Some possible ideas:
1. Do we need this interface? Can we say that all FilterSteps convert `GremlinTypeErrorException` into a boolean?What are the cases where they don't?
2. Why do we wait to convert this exception into false at the top level? Why not convert it in the lowest granularity step itself?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r790763227
##########
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:
Without just calls !within
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r794549554
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AndStep.java
##########
@@ -33,10 +34,18 @@ public AndStep(final Traversal.Admin traversal, final Traversal<S, ?>... travers
@Override
protected boolean filter(final Traverser.Admin<S> traverser) {
+ GremlinTypeErrorException typeError = null;
for (final Traversal.Admin<S, ?> traversal : this.traversals) {
- if (!TraversalUtil.test(traverser, traversal))
- return false;
+ try {
+ if (!TraversalUtil.test(traverser, traversal))
+ return false;
+ } catch (GremlinTypeErrorException ex) {
+ // hold onto it until the end in case any other arguments evaluate to FALSE
Review comment:
Understood. Your comment made me think about this change in the semantics itself.
My understanding is that we are changing the semantics to say "illegal comparisons are treated as false i.e. not comparable" unlike before where we used to throw an exception on illegal comparisons. I also understand that the new change in preferred since it removes "null" as a result for a comparison evaluation and beings it closer to equality/equivalence semantics.
But why do we need ternary logic? Why can't Illegal comparison simply be false (instead of error)? (please feel free to point me to a document or discussion thread if this has already been discussed before).
I am advocating for illegal comparison to be treated as `false` at predicate evaluation itself to keep things simpler for customers. It's easy to understand "illegal comparisons are treated as false" but it is difficult to navigate the granular semantics defined in the table in this PR (the one involving XOR).
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r790153892
##########
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
Review comment:
fix
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#issuecomment-1026004012
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r793610383
##########
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:
Based on what I'm seeing that must be a parse error. @spmallette?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r794737106
##########
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:
checking for a top-level traversal is generally done with `Traversal.isRoot()`. other than that i think you have this right.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r794904865
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AndStep.java
##########
@@ -33,10 +34,18 @@ public AndStep(final Traversal.Admin traversal, final Traversal<S, ?>... travers
@Override
protected boolean filter(final Traverser.Admin<S> traverser) {
+ GremlinTypeErrorException typeError = null;
for (final Traversal.Admin<S, ?> traversal : this.traversals) {
- if (!TraversalUtil.test(traverser, traversal))
- return false;
+ try {
+ if (!TraversalUtil.test(traverser, traversal))
+ return false;
+ } catch (GremlinTypeErrorException ex) {
+ // hold onto it until the end in case any other arguments evaluate to FALSE
Review comment:
We are trying to bring things more in line with SPARQL, which has well defined logics for error expressions. There are certain value expressions that simply cannot be proved as true or false. There are some nice properties we get by using `ERROR` instead of `FALSE` for these. For example any numeric comparison to NaN is an `ERROR` - P.lt/lte/gt/gte - all `ERROR`, and all reduce to `FALSE` if evaluated on their own. When enclosed by a `NOT`, if we simply immediately used `FALSE` instead of `ERROR`, we would get things like `NOT(P.lt(NaN)) = TRUE`, which is logically inconsistent with `P.gte(NaN) = FALSE`.
It may just be that we need to beef up the documentation.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r790280670
##########
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:
@spmallette Is this the right way to test for top-level traversal?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r790774349
##########
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:
Not TBD, NaN is done for compare
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mschmidt00 commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mschmidt00 commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r793247133
##########
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:
ack, having a look now
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mschmidt00 commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mschmidt00 commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r793246908
##########
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:
ack
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r794458616
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AndStep.java
##########
@@ -33,10 +34,18 @@ public AndStep(final Traversal.Admin traversal, final Traversal<S, ?>... travers
@Override
protected boolean filter(final Traverser.Admin<S> traverser) {
+ GremlinTypeErrorException typeError = null;
for (final Traversal.Admin<S, ?> traversal : this.traversals) {
- if (!TraversalUtil.test(traverser, traversal))
- return false;
+ try {
+ if (!TraversalUtil.test(traverser, traversal))
+ return false;
+ } catch (GremlinTypeErrorException ex) {
+ // hold onto it until the end in case any other arguments evaluate to FALSE
Review comment:
I did not understand this. why not short circuit here? (given that the exception is converted to false anyways and the expression will always be false irrespective of the result of other traversals.)
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mschmidt00 commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mschmidt00 commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r793247133
##########
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:
ack, NaN tests look good
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1553: Gremlin Value Expressions 2.0 / Ternary Boolean Logics
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1553:
URL: https://github.com/apache/tinkerpop/pull/1553#discussion_r791791374
##########
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:
The whole system is built around Java Predicates. Tearing that down would be... a lot. ERROR is propagated by Exception and converted to FALSE by FilterStep (our BIN operator).
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org