This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch TINKERPOP-2312 in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 358605363f68d0d18332973518abf94ae3d0a93c Author: stephen <spmalle...@gmail.com> AuthorDate: Thu Jan 16 11:23:06 2020 -0500 TINKERPOP-2312 Empty keys to group() should group to null --- docs/src/reference/gremlin-variants.asciidoc | 58 ++++++++++ docs/src/upgrade/release-3.5.x.asciidoc | 21 ++++ ...mentValueTraversal.java => ValueTraversal.java} | 26 ++--- .../process/traversal/step/ByModulating.java | 6 +- .../process/traversal/step/PathProcessor.java | 4 +- .../process/traversal/step/map/GroupStep.java | 4 +- .../strategy/decoration/SubgraphStrategy.java | 8 +- .../process/traversal/util/TraversalHelper.java | 4 +- .../lambda/ElementValueTraversalTest.java | 78 -------------- .../traversal/lambda/ValueTraversalTest.java | 118 +++++++++++++++++++++ .../optimization/PathProcessorStrategyTest.java | 14 +-- .../Gherkin/GherkinTestRunner.cs | 1 + .../Gherkin/IgnoreException.cs | 9 +- .../Gherkin/ScenarioData.cs | 6 +- .../test/cucumber/feature-steps.js | 2 + gremlin-test/features/sideEffect/Group.feature | 11 ++ .../traversal/step/sideEffect/GroupTest.java | 27 ++++- 17 files changed, 275 insertions(+), 122 deletions(-) diff --git a/docs/src/reference/gremlin-variants.asciidoc b/docs/src/reference/gremlin-variants.asciidoc index 74a58f7..5898050 100644 --- a/docs/src/reference/gremlin-variants.asciidoc +++ b/docs/src/reference/gremlin-variants.asciidoc @@ -1135,6 +1135,28 @@ and then it can be called from the application as follows: include::../../../gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Docs/Reference/GremlinVariantsDslTests.cs[tags=dslExamples] ---- +[[gremlin-dotnet-differences]] +=== Differences + +Gremlin allows for `Map` instances to include `null` keys, but `null` keys in C# `Dictionary` instances are not allowed. +It is therefore necessary to rewrite a traversal such as: + +[source,javascript] +---- +g.V().groupCount().by('age') +---- + +where "age" is not a valid key for all vertices in a way that will remove the need for a `null` to be returned. + +[source,javascript] +---- +g.V().has('age').groupCount().by('age') +g.V().hasLabel('person').groupCount().by('age') +---- + +Either of the above two options accomplishes the desired goal as both prevent `groupCount()` from having to process +the possibility of `null`. + anchor:gremlin-dotnet-template[] [[dotnet-application-examples]] === Application Examples @@ -1323,3 +1345,39 @@ In situations where Javascript reserved words and global functions overlap with bits of conflicting Gremlin get an underscore appended as a suffix: *Steps* - <<from-step,from_()>>, <<in-step,in_()>>, <<with-step,with_()>> + +Gremlin allows for `Map` instances to include `null` keys, but `null` keys in Javascript have some interesting behavior +as in: + +[source,text] +---- +> var a = { null: 'something', 'b': 'else' }; +> JSON.stringify(a) +'{"null":"something","b":"else"}' +> JSON.parse(JSON.stringify(a)) +{ null: 'something', b: 'else' } +> a[null] +'something' +> a['null'] +'something' +---- + +This behavior needs to be considered when using Gremlin to return such results. A typical situation where this might +happen is with `group()` or `groupCount()` as in: + +[source,javascript] +---- +g.V().groupCount().by('age') +---- + +where "age" is not a valid key for all vertices. In these cases, it will return `null` for that key and group on that. +It may bet better in Javascript to filter away those vertices to avoid the return of `null` in the returned `Map`: + +[source,javascript] +---- +g.V().has('age').groupCount().by('age') +g.V().hasLabel('person').groupCount().by('age') +---- + +Either of the above two options accomplishes the desired goal as both prevent `groupCount()` from having to process +the possibility of `null`. \ No newline at end of file diff --git a/docs/src/upgrade/release-3.5.x.asciidoc b/docs/src/upgrade/release-3.5.x.asciidoc index db1426f..08b4ec2 100644 --- a/docs/src/upgrade/release-3.5.x.asciidoc +++ b/docs/src/upgrade/release-3.5.x.asciidoc @@ -164,6 +164,27 @@ gremlin> g.V().has('person','age',null) ==>v[13] ---- +The above described changes also has an effect on steps like `group()` and `groupCount()` which formerly produced +exceptions when keys could not be found: + +[source,text] +---- +gremlin> g.V().group().by('age') +The property does not exist as the key has no associated value for the provided element: v[3]:age +Type ':help' or ':h' for help. +Display stack trace? [yN]n +---- + +The solution was to filter away vertices that did not have the available key so that such steps would work properly +or to write a more complex `by()` modulator to better handle the possibility of a missing key. With the latest changes +however none of that is necessary unless desired: + +[source,text] +---- +gremlin> g.V().groupCount().by('age') +==>[null:2,32:1,35:1,27:1,29:1] +---- + In conclusion, this change in greater support of `null` may affect the behavior of existing traversals written in past versions of TinkerPop as it is no longer possible to rely on `null` to expect a filtering action for traversers. Please review existing Gremlin carefully to ensure that there are no unintended consequences of this change and that diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ValueTraversal.java similarity index 66% rename from gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversal.java rename to gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ValueTraversal.java index 73f73a6..fa5d90d 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ValueTraversal.java @@ -21,24 +21,24 @@ package org.apache.tinkerpop.gremlin.process.traversal.lambda; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil; import org.apache.tinkerpop.gremlin.structure.Element; +import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedProperty; +import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyProperty; import java.util.Map; import java.util.Objects; /** - * More efficiently extracts a value from an {@link Element} or {@code Map} to avoid strategy application costs. Note - * that as of 3.4.5 this class is now poorly named as it was originally designed to work with {@link Element} only. - * In future 3.5.0 this class will likely be renamed but to ensure that we get this revised functionality for - * {@code Map} without introducing a breaking change this name will remain the same. + * More efficiently extracts a value from an {@link Element} or {@code Map} to avoid strategy application costs. * * @author Marko A. Rodriguez (http://markorodriguez.com) + * @author Stephen Mallette (http://stephen.genoprime.com) */ -public final class ElementValueTraversal<V> extends AbstractLambdaTraversal<Element, V> { +public final class ValueTraversal<T, V> extends AbstractLambdaTraversal<T, V> { private final String propertyKey; private V value; - public ElementValueTraversal(final String propertyKey) { + public ValueTraversal(final String propertyKey) { this.propertyKey = propertyKey; } @@ -53,15 +53,11 @@ public final class ElementValueTraversal<V> extends AbstractLambdaTraversal<Elem } @Override - public void addStart(final Traverser.Admin<Element> start) { + public void addStart(final Traverser.Admin<T> start) { if (null == this.bypassTraversal) { - // playing a game of type erasure here.....technically we can process either Map or Element here in this - // case after 3.4.5. and obviously users get weird errors along those lines here anyway. will fix up the - // generics in 3.5.0 when we can take some breaking changes. seemed like this feature would make Gremlin - // a lot nicer and given the small footprint of the change this seemed like a good hack to take. - final Object o = start.get(); + final T o = start.get(); if (o instanceof Element) - this.value = ((Element) o).value(propertyKey); + this.value = (V)((Element) o).property(propertyKey).orElse(null); else if (o instanceof Map) this.value = (V) ((Map) o).get(propertyKey); else @@ -89,7 +85,7 @@ public final class ElementValueTraversal<V> extends AbstractLambdaTraversal<Elem @Override public boolean equals(final Object other) { - return other instanceof ElementValueTraversal - && Objects.equals(((ElementValueTraversal) other).propertyKey, this.propertyKey); + return other instanceof ValueTraversal + && Objects.equals(((ValueTraversal) other).propertyKey, this.propertyKey); } } \ No newline at end of file diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/ByModulating.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/ByModulating.java index 01841fa..b808bf6 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/ByModulating.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/ByModulating.java @@ -23,7 +23,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Order; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.process.traversal.lambda.ColumnTraversal; -import org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.lambda.ValueTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.FunctionTraverser; import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.TokenTraversal; @@ -47,7 +47,7 @@ public interface ByModulating { } public default void modulateBy(final String string) throws UnsupportedOperationException { - this.modulateBy(new ElementValueTraversal(string)); + this.modulateBy(new ValueTraversal(string)); } public default void modulateBy(final T token) throws UnsupportedOperationException { @@ -74,7 +74,7 @@ public interface ByModulating { } public default void modulateBy(final String key, final Comparator comparator) { - this.modulateBy(new ElementValueTraversal<>(key), comparator); + this.modulateBy(new ValueTraversal<>(key), comparator); } public default void modulateBy(final Comparator comparator) { diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/PathProcessor.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/PathProcessor.java index 8a5843a..f0b7e20 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/PathProcessor.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/PathProcessor.java @@ -20,7 +20,7 @@ package org.apache.tinkerpop.gremlin.process.traversal.step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; -import org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.lambda.ValueTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.TokenTraversal; import org.apache.tinkerpop.gremlin.structure.T; @@ -49,7 +49,7 @@ public interface PathProcessor { } else if (traversal instanceof TokenTraversal && ((TokenTraversal) traversal).getToken().equals(T.label)) { if (max.compareTo(ElementRequirement.LABEL) < 0) max = ElementRequirement.LABEL; - } else if (traversal instanceof ElementValueTraversal) { + } else if (traversal instanceof ValueTraversal) { if (max.compareTo(ElementRequirement.PROPERTIES) < 0) max = ElementRequirement.PROPERTIES; } else { diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java index fb0bf3a..4bb06f0 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java @@ -25,7 +25,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.process.traversal.lambda.ColumnTraversal; -import org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.lambda.ValueTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.FunctionTraverser; import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.TokenTraversal; @@ -223,7 +223,7 @@ public final class GroupStep<S, K, V> extends ReducingBarrierStep<S, Map<K, V>> /////////////////////// public static <S, E> Traversal.Admin<S, E> convertValueTraversal(final Traversal.Admin<S, E> valueTraversal) { - if (valueTraversal instanceof ElementValueTraversal || + if (valueTraversal instanceof ValueTraversal || valueTraversal instanceof TokenTraversal || valueTraversal instanceof IdentityTraversal || valueTraversal instanceof ColumnTraversal || diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java index 60b92a8..5333d2b 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java @@ -24,7 +24,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; -import org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.lambda.ValueTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.ClassFilterStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.FilterStep; @@ -220,19 +220,19 @@ public final class SubgraphStrategy extends AbstractTraversalStrategy<TraversalS } } else { Stream.concat(((TraversalParent) step).getGlobalChildren().stream(), ((TraversalParent) step).getLocalChildren().stream()) - .filter(t -> t instanceof ElementValueTraversal) + .filter(t -> t instanceof ValueTraversal) .forEach(t -> { final char propertyType = processesPropertyType(step.getPreviousStep()); if ('p' != propertyType) { final Traversal.Admin<?, ?> temp = new DefaultTraversal<>(); - temp.addStep(new PropertiesStep<>(temp, PropertyType.PROPERTY, ((ElementValueTraversal) t).getPropertyKey())); + temp.addStep(new PropertiesStep<>(temp, PropertyType.PROPERTY, ((ValueTraversal) t).getPropertyKey())); if ('v' == propertyType) TraversalHelper.insertTraversal(0, nonCheckPropertyCriterion.clone(), temp); else temp.addStep(checkPropertyCriterion.clone()); temp.addStep(new PropertyValueStep<>(temp)); temp.setParent((TraversalParent) step); - ((ElementValueTraversal) t).setBypassTraversal(temp); + ((ValueTraversal) t).setBypassTraversal(temp); } }); } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java index 120b2ec..4e7eb92 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java @@ -23,7 +23,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Scope; import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; -import org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.lambda.ValueTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.TokenTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.ByModulating; import org.apache.tinkerpop.gremlin.process.traversal.step.HasContainerHolder; @@ -98,7 +98,7 @@ public final class TraversalHelper { private static char isLocalStarGraph(final Traversal.Admin<?, ?> traversal, char state) { if (state == 'u' && - (traversal instanceof ElementValueTraversal || + (traversal instanceof ValueTraversal || (traversal instanceof TokenTraversal && !((TokenTraversal) traversal).getToken().equals(T.id)))) return 'x'; for (final Step step : traversal.getSteps()) { diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversalTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversalTest.java deleted file mode 100644 index 2752a3d..0000000 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversalTest.java +++ /dev/null @@ -1,78 +0,0 @@ -/* - * 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.lambda; - -import org.apache.tinkerpop.gremlin.process.traversal.traverser.B_O_Traverser; -import org.apache.tinkerpop.gremlin.structure.Edge; -import org.apache.tinkerpop.gremlin.structure.Vertex; -import org.apache.tinkerpop.gremlin.structure.VertexProperty; -import org.junit.Test; - -import java.util.HashMap; -import java.util.Map; - -import static org.junit.Assert.assertEquals; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -public class ElementValueTraversalTest { - - @Test - public void shouldWorkOnVertex() { - final ElementValueTraversal<Integer> t = new ElementValueTraversal<>("age"); - final Vertex v = mock(Vertex.class); - when(v.value("age")).thenReturn(29); - t.addStart(new B_O_Traverser(v, 1).asAdmin()); - assertEquals(29, t.next().intValue()); - } - - @Test - public void shouldWorkOnEdge() { - final ElementValueTraversal<Double> t = new ElementValueTraversal<>("weight"); - final Edge e = mock(Edge.class); - when(e.value("weight")).thenReturn(1.0d); - t.addStart(new B_O_Traverser(e, 1).asAdmin()); - assertEquals(1.0d, t.next(), 0.00001d); - } - - @Test - public void shouldWorkOnVertexProperty() { - final ElementValueTraversal<Integer> t = new ElementValueTraversal<>("age"); - final VertexProperty vp = mock(VertexProperty.class); - when(vp.value("age")).thenReturn(29); - t.addStart(new B_O_Traverser(vp, 1).asAdmin()); - assertEquals(29, t.next().intValue()); - } - - @Test - public void shouldWorkOnMap() { - final ElementValueTraversal<Integer> t = new ElementValueTraversal<>("age"); - final Map<String,Integer> m = new HashMap<>(); - m.put("age", 29); - t.addStart(new B_O_Traverser(m, 1).asAdmin()); - assertEquals(29, t.next().intValue()); - } - - @Test(expected = IllegalStateException.class) - public void shouldThrowExceptionWhenTryingUnsupportedType() { - final ElementValueTraversal<Integer> t = new ElementValueTraversal<>("age"); - t.addStart(new B_O_Traverser(29, 1).asAdmin()); - t.next(); - } -} diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ValueTraversalTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ValueTraversalTest.java new file mode 100644 index 0000000..761d0b6 --- /dev/null +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ValueTraversalTest.java @@ -0,0 +1,118 @@ +/* + * 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.lambda; + +import org.apache.tinkerpop.gremlin.process.traversal.traverser.B_O_Traverser; +import org.apache.tinkerpop.gremlin.structure.Edge; +import org.apache.tinkerpop.gremlin.structure.Property; +import org.apache.tinkerpop.gremlin.structure.Vertex; +import org.apache.tinkerpop.gremlin.structure.VertexProperty; +import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedProperty; +import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertexProperty; +import org.junit.Test; + +import java.util.HashMap; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class ValueTraversalTest { + + @Test + public void shouldWorkOnVertex() { + final ValueTraversal<Vertex, Integer> t = new ValueTraversal<>("age"); + final Vertex v = mock(Vertex.class); + when(v.property("age")).thenReturn(new DetachedVertexProperty<>(1, "age", 29, null)); + t.addStart(new B_O_Traverser<>(v, 1).asAdmin()); + assertEquals(29, t.next().intValue()); + } + + @Test + public void shouldWorkOnVertexWithMissingKey() { + final ValueTraversal<Vertex, Integer> t = new ValueTraversal<>("age"); + final Vertex v = mock(Vertex.class); + when(v.property("age")).thenReturn(VertexProperty.empty()); + t.addStart(new B_O_Traverser<>(v, 1).asAdmin()); + assertNull(t.next()); + } + + @Test + public void shouldWorkOnEdge() { + final ValueTraversal<Edge, Double> t = new ValueTraversal<>("weight"); + final Edge e = mock(Edge.class); + when(e.property("weight")).thenReturn(new DetachedProperty<>("weight", 1.0d)); + t.addStart(new B_O_Traverser<>(e, 1).asAdmin()); + assertEquals(1.0d, t.next(), 0.00001d); + } + + @Test + public void shouldWorkOnEdgeWithMissingKey() { + final ValueTraversal<Edge, Double> t = new ValueTraversal<>("weight"); + final Edge e = mock(Edge.class); + when(e.property("weight")).thenReturn(Property.empty()); + t.addStart(new B_O_Traverser<>(e, 1).asAdmin()); + assertNull(t.next()); + } + + @Test + public void shouldWorkOnVertexProperty() { + final ValueTraversal<VertexProperty, Integer> t = new ValueTraversal<>("age"); + final VertexProperty vp = mock(VertexProperty.class); + when(vp.property("age")).thenReturn(new DetachedProperty<>("age", 29)); + t.addStart(new B_O_Traverser<>(vp, 1).asAdmin()); + assertEquals(29, t.next().intValue()); + } + + @Test + public void shouldWorkOnVertexPropertyWithMissingKey() { + final ValueTraversal<VertexProperty, Integer> t = new ValueTraversal<>("age"); + final VertexProperty vp = mock(VertexProperty.class); + when(vp.property("age")).thenReturn(Property.empty()); + t.addStart(new B_O_Traverser<>(vp, 1).asAdmin()); + assertNull(t.next()); + } + + @Test + public void shouldWorkOnMap() { + final ValueTraversal<Map<String,Integer>, Integer> t = new ValueTraversal<>("age"); + final Map<String,Integer> m = new HashMap<>(); + m.put("age", 29); + t.addStart(new B_O_Traverser<>(m, 1).asAdmin()); + assertEquals(29, t.next().intValue()); + } + + @Test + public void shouldWorkOnMapWithMissingKey() { + final ValueTraversal<Map<String,Integer>, Integer> t = new ValueTraversal<>("not-age"); + final Map<String,Integer> m = new HashMap<>(); + m.put("age", 29); + t.addStart(new B_O_Traverser<>(m, 1).asAdmin()); + assertNull(t.next()); + } + + @Test(expected = IllegalStateException.class) + public void shouldThrowExceptionWhenTryingUnsupportedType() { + final ValueTraversal<Integer, Integer> t = new ValueTraversal<>("age"); + t.addStart(new B_O_Traverser<>(29, 1).asAdmin()); + t.next(); + } +} diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathProcessorStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathProcessorStrategyTest.java index 9616d5a..02635ab 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathProcessorStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathProcessorStrategyTest.java @@ -27,7 +27,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; -import org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.lambda.ValueTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversalStrategies; @@ -82,7 +82,7 @@ public class PathProcessorStrategyTest { {__.select("a"), __.select("a"), Collections.emptyList()}, {__.select("a").by(), __.select("a").by(), Collections.emptyList()}, {__.select("a").by(__.outE().count()), __.select("a").map(__.outE().count()), Collections.emptyList()}, - {__.select("a").by("name"), __.select("a").map(new ElementValueTraversal<>("name")), Collections.emptyList()}, + {__.select("a").by("name"), __.select("a").map(new ValueTraversal<>("name")), Collections.emptyList()}, {__.select("a").out(), __.select("a").out(), Collections.emptyList()}, {__.select(Pop.all, "a").by(__.values("name")), __.select(Pop.all, "a").by(__.values("name")), TraversalStrategies.GlobalCache.getStrategies(Graph.class).toList()}, {__.select(Pop.last, "a").by(__.values("name")), __.select(Pop.last, "a").map(__.values("name")), TraversalStrategies.GlobalCache.getStrategies(Graph.class).toList()}, @@ -91,11 +91,11 @@ public class PathProcessorStrategyTest { {__.select("a", "b"), __.select("a", "b"), Collections.emptyList()}, {__.select("a", "b").by(), __.select("a", "b").by(), Collections.emptyList()}, {__.select("a", "b", "c").by(), __.select("a", "b", "c").by(), Collections.emptyList()}, - {__.select("a", "b").by().by("age"), __.select("b").map(new ElementValueTraversal<>("age")).as("b").select("a").map(new IdentityTraversal<>()).as("a").select(Pop.last, "a", "b"), TraversalStrategies.GlobalCache.getStrategies(Graph.class).toList()}, - {__.select("a", "b").by("name").by("age"), __.select("b").map(new ElementValueTraversal<>("age")).as("b").select("a").map(new ElementValueTraversal<>("name")).as("a").select(Pop.last, "a", "b"), Collections.emptyList()}, - {__.select("a", "b", "c").by("name").by(__.outE().count()), __.select("c").map(new ElementValueTraversal<>("name")).as("c").select("b").map(__.outE().count()).as("b").select("a").map(new ElementValueTraversal<>("name")).as("a").select(Pop.last, "a", "b", "c"), TraversalStrategies.GlobalCache.getStrategies(Graph.class).toList()}, - {__.select(Pop.first, "a", "b").by("name").by("age"), __.select(Pop.first, "b").map(new ElementValueTraversal<>("age")).as("b").select(Pop.first, "a").map(new ElementValueTraversal<>("name")).as("a").select(Pop.last, "a", "b"), Collections.emptyList()}, - {__.select(Pop.last, "a", "b").by("name").by("age"), __.select(Pop.last, "b").map(new ElementValueTraversal<>("age")).as("b").select(Pop.last, "a").map(new ElementValueTraversal<>("name")).as("a").select(Pop.last, "a", "b"), TraversalStrategies.GlobalCache.getStrategies(Graph.class).toList()}, + {__.select("a", "b").by().by("age"), __.select("b").map(new ValueTraversal<>("age")).as("b").select("a").map(new IdentityTraversal<>()).as("a").select(Pop.last, "a", "b"), TraversalStrategies.GlobalCache.getStrategies(Graph.class).toList()}, + {__.select("a", "b").by("name").by("age"), __.select("b").map(new ValueTraversal<>("age")).as("b").select("a").map(new ValueTraversal<>("name")).as("a").select(Pop.last, "a", "b"), Collections.emptyList()}, + {__.select("a", "b", "c").by("name").by(__.outE().count()), __.select("c").map(new ValueTraversal<>("name")).as("c").select("b").map(__.outE().count()).as("b").select("a").map(new ValueTraversal<>("name")).as("a").select(Pop.last, "a", "b", "c"), TraversalStrategies.GlobalCache.getStrategies(Graph.class).toList()}, + {__.select(Pop.first, "a", "b").by("name").by("age"), __.select(Pop.first, "b").map(new ValueTraversal<>("age")).as("b").select(Pop.first, "a").map(new ValueTraversal<>("name")).as("a").select(Pop.last, "a", "b"), Collections.emptyList()}, + {__.select(Pop.last, "a", "b").by("name").by("age"), __.select(Pop.last, "b").map(new ValueTraversal<>("age")).as("b").select(Pop.last, "a").map(new ValueTraversal<>("name")).as("a").select(Pop.last, "a", "b"), TraversalStrategies.GlobalCache.getStrategies(Graph.class).toList()}, {__.select(Pop.all, "a", "b").by("name").by("age"), __.select(Pop.all, "a", "b").by("name").by("age"), Collections.emptyList()}, {__.select(Pop.mixed, "a", "b").by("name").by("age"), __.select(Pop.mixed, "a", "b").by("name").by("age"), Collections.emptyList()}, // where(as("a")...) diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/GherkinTestRunner.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/GherkinTestRunner.cs index 641b9a4..0a3d5ed 100644 --- a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/GherkinTestRunner.cs +++ b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/GherkinTestRunner.cs @@ -41,6 +41,7 @@ namespace Gremlin.Net.IntegrationTest.Gherkin new Dictionary<string, IgnoreReason> { // Add here the name of scenarios to ignore and the reason, e.g.: + { "g_V_group_byXageX", IgnoreReason.NullKeysInMapNotSupported } // { "g_V_peerPressure_withXpropertyName_clusterX_withXedges_outEXknowsXX_pageRankX1X_byXrankX_withXedges_outEXknowsX_withXtimes_2X_group_byXclusterX_byXrank_sumX_limitX100X", IgnoreReason.NoReason }, }; diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/IgnoreException.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/IgnoreException.cs index 726e684..0e25440 100644 --- a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/IgnoreException.cs +++ b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/IgnoreException.cs @@ -50,12 +50,11 @@ namespace Gremlin.Net.IntegrationTest.Gherkin public enum IgnoreReason { + NoReason, + /// <summary> - /// Deserialization of g:T on GraphSON3 is not supported. + /// C# does not allow a `null` value to be used as a key. /// </summary> - TraversalTDeserializationNotSupported, - ReceivedDataDoesntMatchExpected, - NoReason, - EmbeddedListAssertion + NullKeysInMapNotSupported } } \ No newline at end of file diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/ScenarioData.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/ScenarioData.cs index 8c80982..4a15178 100644 --- a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/ScenarioData.cs +++ b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/ScenarioData.cs @@ -104,14 +104,14 @@ namespace Gremlin.Net.IntegrationTest.Gherkin private static IDictionary<string, Vertex> GetVertices(GraphTraversalSource g) { - try + // Property name might not exist and C# doesn't support "null" keys in Dictionary + if (g.V().Count().Next() == g.V().Has("name").Count().Next()) { return g.V().Group<string, object>().By("name").By(__.Tail<Vertex>()).Next() .ToDictionary(kv => kv.Key, kv => (Vertex) kv.Value); } - catch (ResponseException) + else { - // Property name might not exist return EmptyVertices; } } diff --git a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js index b536fe6..3b3ddbf 100644 --- a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js +++ b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js @@ -58,6 +58,7 @@ const parsers = [ ].map(x => [ new RegExp('^' + x[0] + '$'), x[1] ]); const ignoreReason = { + nullKeysInMapNotSupportedWell: "Javascript does not nicely support 'null' as a key in Map instances", setNotSupported: "There is no Set support in gremlin-javascript", needsFurtherInvestigation: '', }; @@ -66,6 +67,7 @@ const ignoredScenarios = { // An associative array containing the scenario name as key, for example: 'g_withSideEffectXa_setX_V_both_name_storeXaX_capXaX': new IgnoreError(ignoreReason.setNotSupported), 'g_withSideEffectXa_setX_V_both_name_aggregateXlocal_aX_capXaX': new IgnoreError(ignoreReason.setNotSupported), + 'g_V_group_byXageX': new IgnoreError(ignoreReason.nullKeysInMapNotSupportedWell), }; defineSupportCode(function(methods) { diff --git a/gremlin-test/features/sideEffect/Group.feature b/gremlin-test/features/sideEffect/Group.feature index e69b447..44705cf 100644 --- a/gremlin-test/features/sideEffect/Group.feature +++ b/gremlin-test/features/sideEffect/Group.feature @@ -28,6 +28,17 @@ Feature: Step - group() | result | | m[{"ripple":"l[v[ripple]]", "peter":"l[v[peter]]", "vadas":"l[v[vadas]]", "josh": "l[v[josh]]", "lop":"l[v[lop]]", "marko":"l[v[marko]]"}] | + Scenario: g_V_group_byXageX + Given the modern graph + And the traversal of + """ + g.V().group().by("age") + """ + When iterated to list + Then the result should be unordered + | result | + | m[{"null":"l[v[lop],v[ripple]]", "d[35].i":"l[v[peter]]", "d[27].i":"l[v[vadas]]", "d[32].i": "l[v[josh]]", "d[29].i":"l[v[marko]]"}] | + Scenario: g_V_group_byXnameX_by Given the modern graph And the traversal of diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java index 5b0cf32..43b694f 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java @@ -59,6 +59,8 @@ public abstract class GroupTest extends AbstractGremlinProcessTest { public abstract Traversal<Vertex, Map<String, Collection<Vertex>>> get_g_V_group_byXnameX(); + public abstract Traversal<Vertex, Map<Integer, Collection<Vertex>>> get_g_V_group_byXageX(); + public abstract Traversal<Vertex, Map<String, Collection<Vertex>>> get_g_V_group_byXnameX_by(); public abstract Traversal<Vertex, Map<String, Collection<Vertex>>> get_g_V_groupXaX_byXnameX_capXaX(); @@ -110,6 +112,24 @@ public abstract class GroupTest extends AbstractGremlinProcessTest { checkSideEffects(traversal.asAdmin().getSideEffects()); } + @Test + @LoadGraphWith(MODERN) + public void g_V_group_byXageX() { + final Traversal<Vertex, Map<Integer, Collection<Vertex>>> traversal = get_g_V_group_byXageX(); + printTraversalForm(traversal); + + final Map<Integer, Collection<Vertex>> map = traversal.next(); + assertEquals(5, map.size()); + map.forEach((key, values) -> { + if (null == key) + assertEquals(2, values.size()); + else + assertEquals(1, values.size()); + }); + assertFalse(traversal.hasNext()); + + checkSideEffects(traversal.asAdmin().getSideEffects()); + } @Test @LoadGraphWith(MODERN) @@ -129,7 +149,7 @@ public abstract class GroupTest extends AbstractGremlinProcessTest { checkSideEffects(traversal.asAdmin().getSideEffects(), "a", HashMap.class); } - private void assertCommonA(Traversal<Vertex, Map<String, Collection<Vertex>>> traversal) { + private void assertCommonA(final Traversal<Vertex, Map<String, Collection<Vertex>>> traversal) { final Map<String, Collection<Vertex>> map = traversal.next(); assertEquals(6, map.size()); map.forEach((key, values) -> { @@ -507,6 +527,11 @@ public abstract class GroupTest extends AbstractGremlinProcessTest { } @Override + public Traversal<Vertex, Map<Integer, Collection<Vertex>>> get_g_V_group_byXageX() { + return g.V().<Integer, Collection<Vertex>>group().by("age"); + } + + @Override public Traversal<Vertex, Map<String, Collection<Vertex>>> get_g_V_group_byXnameX_by() { return g.V().<String, Collection<Vertex>>group().by("name").by(); }