This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch TINKERPOP-2681 in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 8939d5ea4a1ad7dcf27c5b9c807ae17fa93078ad Author: Stephen Mallette <stepm...@amazon.com> AuthorDate: Mon Feb 7 07:27:45 2022 -0500 TINKERPOP-2681 Finalize mergeV semantics around null/empty args --- docs/src/dev/provider/gremlin-semantics.asciidoc | 66 ++++++++++++ .../language/grammar/TraversalMethodVisitor.java | 10 ++ .../grammar/TraversalSourceSpawnMethodVisitor.java | 6 ++ .../traversal/step/map/MergeVertexStep.java | 90 ++++++++++------ gremlin-language/src/main/antlr4/Gremlin.g4 | 11 +- gremlin-test/features/map/MergeVertex.feature | 116 +++++++++++++++++++++ .../traversal/step/map/TinkerMergeVertexStep.java | 4 +- 7 files changed, 263 insertions(+), 40 deletions(-) diff --git a/docs/src/dev/provider/gremlin-semantics.asciidoc b/docs/src/dev/provider/gremlin-semantics.asciidoc index 3bb1244..c6a0223 100644 --- a/docs/src/dev/provider/gremlin-semantics.asciidoc +++ b/docs/src/dev/provider/gremlin-semantics.asciidoc @@ -590,3 +590,69 @@ Same as Comparability, except orderability semantics apply for element compariso * If the unknown arguments are of different types or do not define a natural order, order first by Classname, then by `Object.toString()`. +== Steps + +While TinkerPop has a full test suite for validating functionality of Gremlin, tests alone aren't always exhaustive or +fully demonstrative of Gremlin step semantics. It is also hard to simply read the tests to understand exactly how a +step is meant to behave. This section discusses the semantics for individual steps to help users and providers +understand implementation expectations. + +=== mergeE() + + + +=== mergeV() + +*Description:* Provides upsert-like functionality for vertices. +*Syntax:* `mergeV()` | `mergeV(Map)` | `mergeV(Traversal)` + +[width="100%",options="header"] +|========================================================= +|Start Step |Mid Step |Modulated |Domain |Range +|Y |Y |`option()` |`Map` |`Vertex` + +*Arguments:* + +* `searchCreate` - A `Map` used to match a `Vertex` and if not found be the default set of data to create the new one. +* `onCreate` - A `Map` that is the override to the default of `searchCreate` +* `onMatch` - A `Map` used to update the `Vertex` that is found using the `searchCreate` criteria. + +The `searchCreate` and `onCreate` `Map` instances must consists of any combination of `T.id`, `T.label`, or arbitrary +`String` keys (which are assumed to be vertex properties). The `onMatch` `Map` instance only allows for `String` keys +as the `id` and `label` of a `Vertex` are immutable. Values for these valid keys that are `null` will be treated +according to the semantics of the `addV()` step. + +The `Map` that is used as the argument for `searchCreate` may be assigned from the incoming `Traverser` for the no-arg +`mergeV()`. If `mergeV(Map)` is used, then it will override the incoming `Traverser`. If `mergeV(Traversal)` is used, +the `Traversal` argument must resolve to a `Map` and it would also override the incoming Traverser. The `onCreate` and +`onMatch` arguments are assigned via modulation as described below. + +[width="100%",options="header"] +|========================================================= +|Event |Null `Map` |Empty `Map` +|Search |Filters all vertices |Matches all vertices +|Create |No new vertex |New vertex with defaults +|Update |No update to matched vertex |No update to matched vertex +|========================================================= + +If `T.id` is used for `searchCreate` or `onCreate`, it may be ignored for vertex creation if the `Graph` does not +support user supplied identifiers. + +*Modulation:* + +* `option(Merge, Map)` - Sets the `onCreate` or `onMatch` arguments directly. +* `option(Merge, Traversal)` - Sets the `onCreate` or `onMatch` arguments dynamically where the `Traversal` must +resolve to a `Map`. + +*Exceptions* + +* `Map` arguments are validated for their keys resulting in exception if they do not meet requirements defined above. +* Use of `T.label` should always have a value that is a `String`. + +*Considerations:* + +* `mergeV()` Can only be used mid-traversal. It is not a start step. +* As is common to Gremlin, it is expected that `Traversal` arguments may utilize `sideEffect()` steps. + +See: link:https://github.com/apache/tinkerpop/tree/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java[source], +link:https://tinkerpop.apache.org/docs/current/reference/#mergev-step[reference] \ No newline at end of file diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/TraversalMethodVisitor.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/TraversalMethodVisitor.java index ed1ae19..ed630cc 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/TraversalMethodVisitor.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/TraversalMethodVisitor.java @@ -94,6 +94,9 @@ public class TraversalMethodVisitor extends TraversalRootVisitor<GraphTraversal> */ @Override public GraphTraversal visitTraversalMethod_mergeV_Map(final GremlinParser.TraversalMethod_mergeV_MapContext ctx) { + if (ctx.nullLiteral() != null) { + return this.graphTraversal.mergeV((Map) null); + } return this.graphTraversal.mergeV(GenericLiteralVisitor.getMapLiteral(ctx.genericLiteralMap())); } @@ -126,6 +129,9 @@ public class TraversalMethodVisitor extends TraversalRootVisitor<GraphTraversal> */ @Override public GraphTraversal visitTraversalMethod_mergeE_Map(final GremlinParser.TraversalMethod_mergeE_MapContext ctx) { + if (ctx.nullLiteral() != null) { + return this.graphTraversal.mergeE((Map) null); + } return this.graphTraversal.mergeE(GenericLiteralVisitor.getMapLiteral(ctx.genericLiteralMap())); } @@ -1035,6 +1041,10 @@ public class TraversalMethodVisitor extends TraversalRootVisitor<GraphTraversal> */ @Override public GraphTraversal visitTraversalMethod_option_Merge_Map(final GremlinParser.TraversalMethod_option_Merge_MapContext ctx) { + if (ctx.nullLiteral() != null) { + return this.graphTraversal.option(TraversalEnumParser.parseTraversalEnumFromContext(Merge.class, ctx.traversalMerge()), (Map) null); + } + return graphTraversal.option(TraversalEnumParser.parseTraversalEnumFromContext(Merge.class, ctx.traversalMerge()), (Map) new GenericLiteralVisitor(antlr).visitGenericLiteralMap(ctx.genericLiteralMap())); } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/TraversalSourceSpawnMethodVisitor.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/TraversalSourceSpawnMethodVisitor.java index f7d04f7..6fdda82 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/TraversalSourceSpawnMethodVisitor.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/TraversalSourceSpawnMethodVisitor.java @@ -119,6 +119,9 @@ public class TraversalSourceSpawnMethodVisitor extends GremlinBaseVisitor<GraphT @Override public GraphTraversal visitTraversalSourceSpawnMethod_mergeV_Map(final GremlinParser.TraversalSourceSpawnMethod_mergeV_MapContext ctx) { + if (ctx.nullLiteral() != null) { + return this.traversalSource.mergeV((Map) null); + } return this.traversalSource.mergeV(GenericLiteralVisitor.getMapLiteral(ctx.genericLiteralMap())); } @@ -134,6 +137,9 @@ public class TraversalSourceSpawnMethodVisitor extends GremlinBaseVisitor<GraphT @Override public GraphTraversal visitTraversalSourceSpawnMethod_mergeE_Map(final GremlinParser.TraversalSourceSpawnMethod_mergeE_MapContext ctx) { + if (ctx.nullLiteral() != null) { + return this.traversalSource.mergeE((Map) null); + } return this.traversalSource.mergeE(GenericLiteralVisitor.getMapLiteral(ctx.genericLiteralMap())); } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java index b67621b..e92c11d 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java @@ -45,6 +45,7 @@ import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils; import java.util.ArrayList; +import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -180,7 +181,9 @@ public class MergeVertexStep<S> extends FlatMapStep<S, Vertex> implements Mutati Stream<Vertex> stream; // prioritize lookup by id - if (search.containsKey(T.id)) + if (null == search) { + return Stream.empty(); + } else if (search.containsKey(T.id)) stream = IteratorUtils.stream(graph.vertices(search.get(T.id))); else stream = IteratorUtils.stream(graph.vertices()); @@ -216,21 +219,23 @@ public class MergeVertexStep<S> extends FlatMapStep<S, Vertex> implements Mutati final Map<String, Object> onMatchMap = TraversalUtil.apply(traverser, onMatchTraversal); validateMapInput(onMatchMap, true); - onMatchMap.forEach((key, value) -> { - // trigger callbacks for eventing - in this case, it's a VertexPropertyChangedEvent. if there's no - // registry/callbacks then just set the property - if (this.callbackRegistry != null && !callbackRegistry.getCallbacks().isEmpty()) { - final EventStrategy eventStrategy = getTraversal().getStrategies().getStrategy(EventStrategy.class).get(); - final Property<?> p = v.property(key); - final Property<Object> oldValue = p.isPresent() ? eventStrategy.detach(v.property(key)) : null; - final Event.VertexPropertyChangedEvent vpce = new Event.VertexPropertyChangedEvent(eventStrategy.detach(v), oldValue, value); - this.callbackRegistry.getCallbacks().forEach(c -> c.accept(vpce)); - } - - // try to detect proper cardinality for the key according to the graph - final Graph graph = this.getTraversal().getGraph().get(); - v.property(graph.features().vertex().getCardinality(key), key, value); - }); + if (onMatchMap != null) { + onMatchMap.forEach((key, value) -> { + // trigger callbacks for eventing - in this case, it's a VertexPropertyChangedEvent. if there's no + // registry/callbacks then just set the property + if (this.callbackRegistry != null && !callbackRegistry.getCallbacks().isEmpty()) { + final EventStrategy eventStrategy = getTraversal().getStrategies().getStrategy(EventStrategy.class).get(); + final Property<?> p = v.property(key); + final Property<Object> oldValue = p.isPresent() ? eventStrategy.detach(v.property(key)) : null; + final Event.VertexPropertyChangedEvent vpce = new Event.VertexPropertyChangedEvent(eventStrategy.detach(v), oldValue, value); + this.callbackRegistry.getCallbacks().forEach(c -> c.accept(vpce)); + } + + // try to detect proper cardinality for the key according to the graph + final Graph graph = this.getTraversal().getGraph().get(); + v.property(graph.features().vertex().getCardinality(key), key, value); + }); + } return v; }); @@ -251,21 +256,26 @@ public class MergeVertexStep<S> extends FlatMapStep<S, Vertex> implements Mutati // searchCreate should have already been validated so only do it if it is overridden if (useOnCreate) validateMapInput(m, false); + // if onCreate is null then it's a do nothing final List<Object> keyValues = new ArrayList<>(); - for (Map.Entry<Object, Object> entry : m.entrySet()) { - keyValues.add(entry.getKey()); - keyValues.add(entry.getValue()); - } - vertex = this.getTraversal().getGraph().get().addVertex(keyValues.toArray(new Object[keyValues.size()])); + if (m != null) { + for (Map.Entry<Object, Object> entry : m.entrySet()) { + keyValues.add(entry.getKey()); + keyValues.add(entry.getValue()); + } + vertex = this.getTraversal().getGraph().get().addVertex(keyValues.toArray(new Object[keyValues.size()])); - // trigger callbacks for eventing - in this case, it's a VertexAddedEvent - if (this.callbackRegistry != null && !callbackRegistry.getCallbacks().isEmpty()) { - final EventStrategy eventStrategy = getTraversal().getStrategies().getStrategy(EventStrategy.class).get(); - final Event.VertexAddedEvent vae = new Event.VertexAddedEvent(eventStrategy.detach(vertex)); - this.callbackRegistry.getCallbacks().forEach(c -> c.accept(vae)); - } + // trigger callbacks for eventing - in this case, it's a VertexAddedEvent + if (this.callbackRegistry != null && !callbackRegistry.getCallbacks().isEmpty()) { + final EventStrategy eventStrategy = getTraversal().getStrategies().getStrategy(EventStrategy.class).get(); + final Event.VertexAddedEvent vae = new Event.VertexAddedEvent(eventStrategy.detach(vertex)); + this.callbackRegistry.getCallbacks().forEach(c -> c.accept(vae)); + } - return IteratorUtils.of(vertex); + return IteratorUtils.of(vertex); + } else { + return Collections.emptyIterator(); + } } } @@ -274,6 +284,7 @@ public class MergeVertexStep<S> extends FlatMapStep<S, Vertex> implements Mutati * to immutable parts of an {@link Edge} (id, label, incident vertices) so those can be ignored in the validation. */ public static void validateMapInput(final Map<?,Object> m, final boolean ignoreTokens) { + if (null == m) return; if (ignoreTokens) { m.entrySet().stream().filter(e -> { final Object k = e.getKey(); @@ -295,11 +306,24 @@ public class MergeVertexStep<S> extends FlatMapStep<S, Vertex> implements Mutati } if (!ignoreTokens) { - m.entrySet().stream().filter(e -> e.getKey() == T.label && !(e.getValue() instanceof String)).findFirst().map(e -> { - throw new IllegalArgumentException(String.format( - "mergeV() expects T.label value to be of String - found: %s", - e.getValue().getClass().getSimpleName())); - }); + if (m.containsKey(T.id)) { + if (null == m.get(T.id)) { + throw new IllegalArgumentException("Vertex id cannot be null"); + } + } + + if (m.containsKey(T.label)) { + final Object l = m.get(T.label); + if (null == l) { + throw new IllegalArgumentException("Vertex label cannot be null"); + } + + if (!(l instanceof String)) { + throw new IllegalArgumentException(String.format( + "mergeV() expects T.label value to be of String - found: %s", + l.getClass().getSimpleName())); + } + } } } diff --git a/gremlin-language/src/main/antlr4/Gremlin.g4 b/gremlin-language/src/main/antlr4/Gremlin.g4 index f359dd8..298cc3e 100644 --- a/gremlin-language/src/main/antlr4/Gremlin.g4 +++ b/gremlin-language/src/main/antlr4/Gremlin.g4 @@ -131,12 +131,12 @@ traversalSourceSpawnMethod_io ; traversalSourceSpawnMethod_mergeV - : 'mergeV' LPAREN genericLiteralMap RPAREN #traversalSourceSpawnMethod_mergeV_Map + : 'mergeV' LPAREN (genericLiteralMap | nullLiteral) RPAREN #traversalSourceSpawnMethod_mergeV_Map | 'mergeV' LPAREN nestedTraversal RPAREN #traversalSourceSpawnMethod_mergeV_Traversal ; traversalSourceSpawnMethod_mergeE - : 'mergeE' LPAREN genericLiteralMap RPAREN #traversalSourceSpawnMethod_mergeE_Map + : 'mergeE' LPAREN (genericLiteralMap | nullLiteral) RPAREN #traversalSourceSpawnMethod_mergeE_Map | 'mergeE' LPAREN nestedTraversal RPAREN #traversalSourceSpawnMethod_mergeE_Traversal ; @@ -288,17 +288,16 @@ traversalMethod_addV traversalMethod_mergeV : 'mergeV' LPAREN RPAREN #traversalMethod_mergeV_empty - | 'mergeV' LPAREN genericLiteralMap RPAREN #traversalMethod_mergeV_Map + | 'mergeV' LPAREN (genericLiteralMap | nullLiteral) RPAREN #traversalMethod_mergeV_Map | 'mergeV' LPAREN nestedTraversal RPAREN #traversalMethod_mergeV_Traversal ; traversalMethod_mergeE : 'mergeE' LPAREN RPAREN #traversalMethod_mergeE_empty - | 'mergeE' LPAREN genericLiteralMap RPAREN #traversalMethod_mergeE_Map + | 'mergeE' LPAREN (genericLiteralMap | nullLiteral) RPAREN #traversalMethod_mergeE_Map | 'mergeE' LPAREN nestedTraversal RPAREN #traversalMethod_mergeE_Traversal ; - traversalMethod_aggregate : 'aggregate' LPAREN traversalScope COMMA stringLiteral RPAREN #traversalMethod_aggregate_Scope_String | 'aggregate' LPAREN stringLiteral RPAREN #traversalMethod_aggregate_String @@ -558,7 +557,7 @@ traversalMethod_not traversalMethod_option : 'option' LPAREN traversalPredicate COMMA nestedTraversal RPAREN #traversalMethod_option_Predicate_Traversal - | 'option' LPAREN traversalMerge COMMA genericLiteralMap RPAREN #traversalMethod_option_Merge_Map + | 'option' LPAREN traversalMerge COMMA (genericLiteralMap | nullLiteral) RPAREN #traversalMethod_option_Merge_Map | 'option' LPAREN traversalMerge COMMA nestedTraversal RPAREN #traversalMethod_option_Merge_Traversal | 'option' LPAREN genericLiteral COMMA nestedTraversal RPAREN #traversalMethod_option_Object_Traversal | 'option' LPAREN nestedTraversal RPAREN #traversalMethod_option_Traversal diff --git a/gremlin-test/features/map/MergeVertex.feature b/gremlin-test/features/map/MergeVertex.feature index 4707555..7201433 100644 --- a/gremlin-test/features/map/MergeVertex.feature +++ b/gremlin-test/features/map/MergeVertex.feature @@ -64,7 +64,123 @@ Feature: Step - mergeV() # g_V_mapXmergeXlabel_person_name_joshXX # - mergeV(Map) with no option() - testing child traversal usage # - results in one new vertex and one existing vertex that was just created + # g_mergeVXnullX + # - mergeV(null) with no option() + # - results in no new vertex and nothing returned + # g_mergeVXemptyX + # - mergeV(empty) with no option() + # - results in matched vertex with no updates + # g_mergeVXemptyX_no_existing + # - mergeV(empty) with no option() + # - results in not matched updates and a creation of a vertex with default values + # g_mergeVXnullX_optionXonCreate_emptyX + # - mergeV(null) with onCreate(empty) + # - results in no matches and creates a default vertex + # g_mergeVXlabel_person_name_stephenX_optionXonCreate_nullX + # - mergeV(Map) with onCreate(null) + # - results in no match and no vertex creation + # g_mergeVXnullX_optionXonCreate_label_null_name_markoX + # - mergeV(null) with onCreate(Map) where Map has a null label + # - results in error + # g_mergeVXemptyX_optionXonMatch_nullX + # - mergeV(empty) with onMatch(null) + # - results in a match and no vertex update + Scenario: g_mergeVXemptyX_optionXonMatch_nullX + Given the empty graph + And the graph initializer of + """ + g.addV("person").property("name", "marko").property("age", 29) + """ + And the traversal of + """ + g.mergeV([:]).option(Merge.onMatch, null) + """ + When iterated to list + Then the result should have a count of 1 + And the graph should return 1 for count of "g.V().has(\"person\",\"name\",\"marko\").has(\"age\",29)" + + Scenario: g_mergeVXnullX_optionXonCreate_label_null_name_markoX + Given the empty graph + And the graph initializer of + """ + g.addV("person").property("name", "marko").property("age", 29) + """ + And using the parameter xx1 defined as "m[{\"t[label]\": null, \"name\":\"marko\"}]" + And the traversal of + """ + g.mergeV(null).option(Merge.onCreate,xx1) + """ + When iterated to list + Then the traversal will raise an error + + Scenario: g_mergeVXlabel_person_name_stephenX_optionXonCreate_nullX + Given the empty graph + And the graph initializer of + """ + g.addV("person").property("name", "marko").property("age", 29) + """ + And using the parameter xx1 defined as "m[{\"t[label]\": \"person\", \"name\":\"stephen\"}]" + And the traversal of + """ + g.mergeV(xx1).option(Merge.onCreate, null) + """ + When iterated to list + Then the result should have a count of 0 + And the graph should return 1 for count of "g.V()" + And the graph should return 1 for count of "g.V().has(\"person\",\"name\",\"marko\")" + + Scenario: g_mergeVXnullX_optionXonCreate_emptyX + Given the empty graph + And the graph initializer of + """ + g.addV("person").property("name", "marko").property("age", 29) + """ + And the traversal of + """ + g.mergeV(null).option(Merge.onCreate,[:]) + """ + When iterated to list + Then the result should have a count of 1 + And the graph should return 2 for count of "g.V()" + + Scenario: g_mergeVXemptyX_no_existing + Given the empty graph + And the traversal of + """ + g.mergeV([:]) + """ + When iterated to list + Then the result should have a count of 1 + And the graph should return 1 for count of "g.V()" + + Scenario: g_mergeVXemptyX + Given the empty graph + And the graph initializer of + """ + g.addV("person").property("name", "marko").property("age", 29) + """ + And the traversal of + """ + g.mergeV([:]) + """ + When iterated to list + Then the result should have a count of 1 + And the graph should return 1 for count of "g.V().has(\"person\",\"name\",\"marko\").has(\"age\",29)" + + Scenario: g_mergeVXnullX + Given the empty graph + And the graph initializer of + """ + g.addV("person").property("name", "marko").property("age", 29) + """ + And the traversal of + """ + g.mergeV(null) + """ + When iterated to list + Then the result should have a count of 0 + And the graph should return 1 for count of "g.V()" Scenario: g_mergeVXlabel_person_name_stephenX Given the empty graph diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/map/TinkerMergeVertexStep.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/map/TinkerMergeVertexStep.java index 6771ebd..9c3cc03 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/map/TinkerMergeVertexStep.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/map/TinkerMergeVertexStep.java @@ -52,7 +52,9 @@ public class TinkerMergeVertexStep<S> extends MergeVertexStep<S> { Stream<Vertex> stream; // prioritize lookup by id but otherwise attempt an index lookup - if (search.containsKey(T.id)) { + if (null == search) { + return Stream.empty(); + } else if (search.containsKey(T.id)) { stream = IteratorUtils.stream(graph.vertices(search.get(T.id))); } else { // look for the first index we can find - that's the lucky winner. may or may not be the most selective