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/31 17:59:10 UTC
[GitHub] [tinkerpop] spmallette opened a new pull request #1555: TINKERPOP-2681 mergeE() and mergeV()
spmallette opened a new pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555
https://issues.apache.org/jira/browse/TINKERPOP-2681
Please see the upgrade docs for more information on this change. It should be noted that there isn't any reference documentation here and perhaps the recipes need some review. @krlawrence is going to have at look at those sorts of changes separately from this body of work.
Since this change is on the heavy side I thought I'd put it out here for review despite there being some tweaks that I may notice as I review the diff. Generally speaking though,
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] spmallette commented on a change in pull request #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r797579735
##########
File path: gremlin-test/features/sideEffect/Group.feature
##########
@@ -264,17 +264,17 @@ Feature: Step - group()
| m[{"software":"d[2.0].d", "person":"d[5.0].d"}] |
# The post-ordering really isn't really right but works around TINKERPOP-2600
- Scenario: g_withSideEffectXa__marko_666_noone_blahX_V_groupXaX_byXnameX_byXoutE_label_foldX_capXaX
- Given the modern graph
- And using the parameter xx1 defined as "m[{\"marko\":[\"666\"], \"noone\":[\"blah\"]}]"
- And the traversal of
- """
- g.withSideEffect("a", xx1).V().group("a").by("name").by(__.outE().label().fold()).cap("a").unfold().group().by(keys).by(select(values).order(Scope.local).by(Order.asc))
- """
- When iterated to list
- Then the result should be unordered
- | result |
- | m[{"ripple":[], "peter":["created"], "noone":["blah"], "vadas":[], "josh":["created", "created"], "lop":[], "marko":["666", "created", "knows", "knows"]}] |
+# Scenario: g_withSideEffectXa__marko_666_noone_blahX_V_groupXaX_byXnameX_byXoutE_label_foldX_capXaX
Review comment:
added further context in comments and created: https://issues.apache.org/jira/browse/TINKERPOP-2699
functionally the Gremlin works fine...the test frameworks just have a problem asserting the behavior.
--
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] FlorianHockmann commented on a change in pull request #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r797585325
##########
File path: docs/src/upgrade/release-3.6.x.asciidoc
##########
@@ -29,6 +29,71 @@ Please see the link:https://github.com/apache/tinkerpop/blob/3.6.0/CHANGELOG.asc
=== Upgrading for Users
+==== mergeV() and mergeE()
+
+One of the most commonly used patterns in Gremlin is the use of `fold().coalesce(unfold(), ...)` to perform upsert-like
+functionality. While this pattern is quite flexible, it can also be confusing to new users and for certain use cases
+challenging to get the pattern correctly implemented. For providers, the pattern is difficult to properly optimize
+because it can branch into complexity quite quickly making it hard to identify a section of Gremlin for an upsert and
+therefore is not executed as efficiently as it might have been otherwise.
+
+The new `mergeV()` and `mergeE()` steps greatly simplify this pattern and as the pattern is condensed into a single
+step it should be straightforward for providers to optimize as part of their implementations. The following example
+demonstrates just how much easier implementing a basic upsert of a vertex has gotten:
+
+[source,text]
+----
+// prior to 3.6.0, use fold().coalesce(unfold(), ...)
+gremlin> g.V().has('person','name','vadas').has('age', 27).
+......1> fold().
+......2> coalesce(unfold().property('age',30),
+......3> addV('person').property('name','vadas').property('age',27)).
Review comment:
With not obvious to understand I actually also meant the example below with the new `mergeV()` step because of the `onMatch` option.
But the example you added above is definitely easier to understand in my opinion and first reading that example together with the description for this example should also make that example easy enough to understand.
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r797546454
##########
File path: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsGremlinBinaryRequestDecoder.java
##########
@@ -76,6 +76,7 @@ protected void decode(final ChannelHandlerContext channelHandlerContext, final B
try {
objects.add(serializer.deserializeRequest(messageBytes.discardReadBytes()));
} catch (SerializationException se) {
+ logger.warn(se.getMessage());
Review comment:
i went in a two day hole of debugging because I couldn't figure out where that a serialization problem from here was causing trouble. just wanted some additional information in the logs here to help for that sort of problem in the future. i will change the log messages to a general message and stacktrace.
--
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] bechbd commented on pull request #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
bechbd commented on pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#issuecomment-1026413441
I built this branch and tried it out, and it is looking great. So much easier to grok than the current `coalesce()` pattern
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] FlorianHockmann commented on a change in pull request #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r797389596
##########
File path: docs/src/upgrade/release-3.6.x.asciidoc
##########
@@ -29,6 +29,71 @@ Please see the link:https://github.com/apache/tinkerpop/blob/3.6.0/CHANGELOG.asc
=== Upgrading for Users
+==== mergeV() and mergeE()
+
+One of the most commonly used patterns in Gremlin is the use of `fold().coalesce(unfold(), ...)` to perform upsert-like
+functionality. While this pattern is quite flexible, it can also be confusing to new users and for certain use cases
+challenging to get the pattern correctly implemented. For providers, the pattern is difficult to properly optimize
+because it can branch into complexity quite quickly making it hard to identify a section of Gremlin for an upsert and
+therefore is not executed as efficiently as it might have been otherwise.
+
+The new `mergeV()` and `mergeE()` steps greatly simplify this pattern and as the pattern is condensed into a single
+step it should be straightforward for providers to optimize as part of their implementations. The following example
+demonstrates just how much easier implementing a basic upsert of a vertex has gotten:
+
+[source,text]
+----
+// prior to 3.6.0, use fold().coalesce(unfold(), ...)
+gremlin> g.V().has('person','name','vadas').has('age', 27).
+......1> fold().
+......2> coalesce(unfold().property('age',30),
+......3> addV('person').property('name','vadas').property('age',27)).
Review comment:
I find the example here already quite advanced and not obvious to understand. It took me at least a bit to understand that it's really intended here to create a vertex with `age=27` if it doesn't exist already and to change its age otherwise to `30`.
I suggest that we use a simpler example here in the upgrade docs and I guess most current usages of the fold/coalesce pattern out there are more of the nature:
```
g.V().
has('person', 'name', 'vadas').
fold().
coalesce(unfold(), addV('person').property('name', 'vadas')).
property('age', 30)
```
If I understand the new functionality correctly, then this doesn't need any `option()` at all, right? We could then afterwards maybe simply mention these additional options and then either link to the reference docs where they are explained in more details or add additional more advanced examples here that show these options.
##########
File path: CHANGELOG.asciidoc
##########
@@ -27,6 +27,8 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
* Changed TinkerGraph to allow identifiers to be heterogeneous when filtering.
* Prevented values of `T` to `property()` from being `null`.
* Added `fail()` step.
+* Added `mergeV()` and `mergeE()` steps.
+* Moved `TraversalOptionParent.Pick` to it's own class as `Pick`.
Review comment:
```suggestion
* Moved `TraversalOptionParent.Pick` to its own class as `Pick`.
```
##########
File path: docs/src/upgrade/release-3.6.x.asciidoc
##########
@@ -29,6 +29,71 @@ Please see the link:https://github.com/apache/tinkerpop/blob/3.6.0/CHANGELOG.asc
=== Upgrading for Users
+==== mergeV() and mergeE()
+
+One of the most commonly used patterns in Gremlin is the use of `fold().coalesce(unfold(), ...)` to perform upsert-like
+functionality. While this pattern is quite flexible, it can also be confusing to new users and for certain use cases
+challenging to get the pattern correctly implemented. For providers, the pattern is difficult to properly optimize
+because it can branch into complexity quite quickly making it hard to identify a section of Gremlin for an upsert and
+therefore is not executed as efficiently as it might have been otherwise.
+
+The new `mergeV()` and `mergeE()` steps greatly simplify this pattern and as the pattern is condensed into a single
+step it should be straightforward for providers to optimize as part of their implementations. The following example
+demonstrates just how much easier implementing a basic upsert of a vertex has gotten:
+
+[source,text]
+----
+// prior to 3.6.0, use fold().coalesce(unfold(), ...)
+gremlin> g.V().has('person','name','vadas').has('age', 27).
+......1> fold().
+......2> coalesce(unfold().property('age',30),
+......3> addV('person').property('name','vadas').property('age',27)).
+......4> elementMap()
+==>[id:2,label:person,name:vadas,age:30]
+
+// 3.6.0
+gremlin> g.mergeV([(T.label): 'person', name:'vadas', age: 27]).
+......1> option(onMatch, [age: 30]).
+......2> elementMap()
+==>[id:2,label:person,name:vadas,age:30]
+----
+
+The `fold().coalesce(unfold(), ...)` pattern was even more complicated for upserting edges, but the following example
+demonstrates how much easier `mergeE()` is to follow:
+
+[source,text]
+----
+// prior to 3.6.0, use fold().coalesce(unfold(), ...)
Review comment:
(nitpick) This comment says that `fold().coalesce(unfold(), ...)` is used but the Gremlin traversal doesn't use `fold()` and `unfold()`. Maybe we should really change this traversal to also use `fold()` and `unfold()`?
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r797533385
##########
File path: docs/src/upgrade/release-3.6.x.asciidoc
##########
@@ -29,6 +29,71 @@ Please see the link:https://github.com/apache/tinkerpop/blob/3.6.0/CHANGELOG.asc
=== Upgrading for Users
+==== mergeV() and mergeE()
+
+One of the most commonly used patterns in Gremlin is the use of `fold().coalesce(unfold(), ...)` to perform upsert-like
+functionality. While this pattern is quite flexible, it can also be confusing to new users and for certain use cases
+challenging to get the pattern correctly implemented. For providers, the pattern is difficult to properly optimize
+because it can branch into complexity quite quickly making it hard to identify a section of Gremlin for an upsert and
+therefore is not executed as efficiently as it might have been otherwise.
+
+The new `mergeV()` and `mergeE()` steps greatly simplify this pattern and as the pattern is condensed into a single
+step it should be straightforward for providers to optimize as part of their implementations. The following example
+demonstrates just how much easier implementing a basic upsert of a vertex has gotten:
+
+[source,text]
+----
+// prior to 3.6.0, use fold().coalesce(unfold(), ...)
+gremlin> g.V().has('person','name','vadas').has('age', 27).
+......1> fold().
+......2> coalesce(unfold().property('age',30),
+......3> addV('person').property('name','vadas').property('age',27)).
+......4> elementMap()
+==>[id:2,label:person,name:vadas,age:30]
+
+// 3.6.0
+gremlin> g.mergeV([(T.label): 'person', name:'vadas', age: 27]).
+......1> option(onMatch, [age: 30]).
+......2> elementMap()
+==>[id:2,label:person,name:vadas,age:30]
+----
+
+The `fold().coalesce(unfold(), ...)` pattern was even more complicated for upserting edges, but the following example
+demonstrates how much easier `mergeE()` is to follow:
+
+[source,text]
+----
+// prior to 3.6.0, use fold().coalesce(unfold(), ...)
Review comment:
i fixed that up.
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r801705069
##########
File path: docs/src/dev/io/graphson.asciidoc
##########
@@ -2819,6 +2779,16 @@ The following `Bytecode` example represents the traversal of `g.V().hasLabel('pe
}
----
+==== Merge
Review comment:
we only show one example for most of these. it's (hopefully) understood that any enum value of `Merge` would follow this pattern.
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r797529098
##########
File path: docs/src/upgrade/release-3.6.x.asciidoc
##########
@@ -29,6 +29,71 @@ Please see the link:https://github.com/apache/tinkerpop/blob/3.6.0/CHANGELOG.asc
=== Upgrading for Users
+==== mergeV() and mergeE()
+
+One of the most commonly used patterns in Gremlin is the use of `fold().coalesce(unfold(), ...)` to perform upsert-like
+functionality. While this pattern is quite flexible, it can also be confusing to new users and for certain use cases
+challenging to get the pattern correctly implemented. For providers, the pattern is difficult to properly optimize
+because it can branch into complexity quite quickly making it hard to identify a section of Gremlin for an upsert and
+therefore is not executed as efficiently as it might have been otherwise.
+
+The new `mergeV()` and `mergeE()` steps greatly simplify this pattern and as the pattern is condensed into a single
+step it should be straightforward for providers to optimize as part of their implementations. The following example
+demonstrates just how much easier implementing a basic upsert of a vertex has gotten:
+
+[source,text]
+----
+// prior to 3.6.0, use fold().coalesce(unfold(), ...)
+gremlin> g.V().has('person','name','vadas').has('age', 27).
+......1> fold().
+......2> coalesce(unfold().property('age',30),
+......3> addV('person').property('name','vadas').property('age',27)).
Review comment:
> I find the example here already quite advanced and not obvious to understand.
well, i think that's somewhat indicative of why these steps needed to be added. i tried to pick an easy one already....you should see a "hard" one. 😄 that said, i've added a more simple example along the lines you described.
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r798482841
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java
##########
@@ -0,0 +1,293 @@
+/*
+ * 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.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import 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.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.ListCallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Implementation for the {@code mergeV()} step covering both the start step version and the one used mid-traversal.
+ */
+public class MergeVertexStep<S> extends FlatMapStep<S, Vertex> implements Mutating<Event>,
+ TraversalOptionParent<Merge, S, Vertex> {
+
+ private final boolean isStart;
+ private boolean first = true;
+ private Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal;
+ private Traversal.Admin<S, Map<Object, Object>> onCreateTraversal = null;
+ private Traversal.Admin<S, Map<String, Object>> onMatchTraversal = null;
+
+ protected CallbackRegistry<Event> callbackRegistry;
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart) {
+ this(traversal, isStart, new IdentityTraversal());
+ }
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart, final Map<Object, Object> searchCreate) {
+ this(traversal, isStart, new ConstantTraversal<>(searchCreate));
+ }
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart, final Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal) {
+ super(traversal);
+ this.isStart = isStart;
+ this.searchCreateTraversal = integrateChild(searchCreateTraversal);
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getSearchCreateTraversal() {
+ return searchCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getOnCreateTraversal() {
+ return onCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<String, Object>> getOnMatchTraversal() {
+ return onMatchTraversal;
+ }
+
+ /**
+ * Determines if this is a start step.
+ */
+ public boolean isStart() {
+ return isStart;
+ }
+
+ public boolean isFirst() {
+ return first;
+ }
+
+ public CallbackRegistry<Event> getCallbackRegistry() {
+ return callbackRegistry;
+ }
+
+ @Override
+ public void addChildOption(final Merge token, final Traversal.Admin<S, Vertex> traversalOption) {
+ if (token == Merge.onCreate) {
+ this.onCreateTraversal = this.integrateChild(traversalOption);
+ } else if (token == Merge.onMatch) {
+ this.onMatchTraversal = this.integrateChild(traversalOption);
+ } else {
+ throw new UnsupportedOperationException(String.format("Option %s for Merge is not supported", token.name()));
+ }
+ }
+
+ @Override
+ public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
+ final List<Traversal.Admin<S, E>> children = new ArrayList<>();
+ if (searchCreateTraversal != null) children.add((Traversal.Admin<S, E>) searchCreateTraversal);
+ if (onMatchTraversal != null) children.add((Traversal.Admin<S, E>) onMatchTraversal);
+ if (onCreateTraversal != null) children.add((Traversal.Admin<S, E>) onCreateTraversal);
+ return children;
+ }
+
+ @Override
+ public void configure(final Object... keyValues) {
+ // this is a Mutating step but property() should not be folded into this step. this exception should not
+ // end up visible to users really
+ }
+
+ @Override
+ public Parameters getParameters() {
+ // merge doesn't take fold ups of property() calls. those need to get treated as regular old PropertyStep
+ // instances. not sure if this should support with() though.....none of the other Mutating steps do.
+ return null;
+ }
+
+ @Override
+ protected Traverser.Admin<Vertex> processNextStart() {
+ // when it's a start step a traverser needs to be created to kick off the traversal.
+ if (isStart && first) {
+ first = false;
+ final TraverserGenerator generator = this.getTraversal().getTraverserGenerator();
+ this.addStart(generator.generate(false, (Step) this, 1L));
+ }
+ return super.processNextStart();
+ }
+
+ /**
+ * Use the {@code Map} of search criteria to most efficiently return a {@code Stream<Vertex>} of matching elements.
+ * Providers might override this method when extending this step to provide their own optimized mechanisms for
+ * matching the list of vertices. This implementation is only optimized for the {@link T#id} so any other usage
+ * will simply be in-memory filtering which could be slow.
+ */
+ protected Stream<Vertex> createSearchStream(final Map<Object,Object> search) {
+ final Graph graph = this.getTraversal().getGraph().get();
+
+ Stream<Vertex> stream;
+ // prioritize lookup by id
+ if (search.containsKey(T.id))
+ stream = IteratorUtils.stream(graph.vertices(search.get(T.id)));
+ else
+ stream = IteratorUtils.stream(graph.vertices());
+
+ // in-memory filter is not going to be nice here. it will be up to graphs to optimize this step as they do
+ // for other Mutation steps
+ stream = stream.filter(v -> {
+ // try to match on all search criteria skipping T.id as it was handled above
+ return search.entrySet().stream().filter(kv -> kv.getKey() != T.id).allMatch(kv -> {
+ if (kv.getKey() == T.label) {
+ return v.label().equals(kv.getValue());
+ } else {
+ final VertexProperty<Object> vp = v.property(kv.getKey().toString());
+ return vp.isPresent() && kv.getValue().equals(vp.value());
+ }
+ });
+ });
+
+ return stream;
+ }
+
+ @Override
+ protected Iterator<Vertex> flatMap(final Traverser.Admin<S> traverser) {
+ final Map<Object,Object> searchCreate = TraversalUtil.apply(traverser, searchCreateTraversal);
+
+ Stream<Vertex> stream = createSearchStream(searchCreate);
+ stream = stream.map(v -> {
+ // if no onMatch is defined then there is no update - return the vertex unchanged
+ if (null == onMatchTraversal) return v;
+
+ // assume good input from GraphTraversal - folks might drop in a T here even though it is immutable
+ final Map<String, Object> onMatchMap = TraversalUtil.apply(traverser, onMatchTraversal);
+ 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()) {
Review comment:
tests here: https://github.com/apache/tinkerpop/pull/1555/files#diff-7f146792ffd876947ef179432d27ec9a862e43d81728898b632c363e3ab3069c
--
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] krlawrence commented on pull request #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
krlawrence commented on pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#issuecomment-1026145737
I have built this branch and run several tests, all of which worked. I think this is a huge win for Gremlin users. The upsert pattern - whether using fold/coalese or injected maps is something most users struggle with and query engines find hard to optimize well.
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r797657666
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java
##########
@@ -0,0 +1,293 @@
+/*
+ * 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.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import 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.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.ListCallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Implementation for the {@code mergeV()} step covering both the start step version and the one used mid-traversal.
+ */
+public class MergeVertexStep<S> extends FlatMapStep<S, Vertex> implements Mutating<Event>,
+ TraversalOptionParent<Merge, S, Vertex> {
+
+ private final boolean isStart;
+ private boolean first = true;
+ private Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal;
+ private Traversal.Admin<S, Map<Object, Object>> onCreateTraversal = null;
+ private Traversal.Admin<S, Map<String, Object>> onMatchTraversal = null;
+
+ protected CallbackRegistry<Event> callbackRegistry;
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart) {
+ this(traversal, isStart, new IdentityTraversal());
+ }
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart, final Map<Object, Object> searchCreate) {
+ this(traversal, isStart, new ConstantTraversal<>(searchCreate));
+ }
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart, final Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal) {
+ super(traversal);
+ this.isStart = isStart;
+ this.searchCreateTraversal = integrateChild(searchCreateTraversal);
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getSearchCreateTraversal() {
+ return searchCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getOnCreateTraversal() {
+ return onCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<String, Object>> getOnMatchTraversal() {
+ return onMatchTraversal;
+ }
+
+ /**
+ * Determines if this is a start step.
+ */
+ public boolean isStart() {
+ return isStart;
+ }
+
+ public boolean isFirst() {
+ return first;
+ }
+
+ public CallbackRegistry<Event> getCallbackRegistry() {
+ return callbackRegistry;
+ }
+
+ @Override
+ public void addChildOption(final Merge token, final Traversal.Admin<S, Vertex> traversalOption) {
+ if (token == Merge.onCreate) {
+ this.onCreateTraversal = this.integrateChild(traversalOption);
+ } else if (token == Merge.onMatch) {
+ this.onMatchTraversal = this.integrateChild(traversalOption);
+ } else {
+ throw new UnsupportedOperationException(String.format("Option %s for Merge is not supported", token.name()));
+ }
+ }
+
+ @Override
+ public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
+ final List<Traversal.Admin<S, E>> children = new ArrayList<>();
+ if (searchCreateTraversal != null) children.add((Traversal.Admin<S, E>) searchCreateTraversal);
+ if (onMatchTraversal != null) children.add((Traversal.Admin<S, E>) onMatchTraversal);
+ if (onCreateTraversal != null) children.add((Traversal.Admin<S, E>) onCreateTraversal);
+ return children;
+ }
+
+ @Override
+ public void configure(final Object... keyValues) {
+ // this is a Mutating step but property() should not be folded into this step. this exception should not
+ // end up visible to users really
+ }
+
+ @Override
+ public Parameters getParameters() {
+ // merge doesn't take fold ups of property() calls. those need to get treated as regular old PropertyStep
+ // instances. not sure if this should support with() though.....none of the other Mutating steps do.
+ return null;
+ }
+
+ @Override
+ protected Traverser.Admin<Vertex> processNextStart() {
+ // when it's a start step a traverser needs to be created to kick off the traversal.
+ if (isStart && first) {
+ first = false;
+ final TraverserGenerator generator = this.getTraversal().getTraverserGenerator();
+ this.addStart(generator.generate(false, (Step) this, 1L));
+ }
+ return super.processNextStart();
+ }
+
+ /**
+ * Use the {@code Map} of search criteria to most efficiently return a {@code Stream<Vertex>} of matching elements.
+ * Providers might override this method when extending this step to provide their own optimized mechanisms for
+ * matching the list of vertices. This implementation is only optimized for the {@link T#id} so any other usage
+ * will simply be in-memory filtering which could be slow.
+ */
+ protected Stream<Vertex> createSearchStream(final Map<Object,Object> search) {
Review comment:
I would consider moving this out of here and maybe make it part of the Graph API - give providers a chance to optimize it. Doesn't need to be this PR though.
##########
File path: gremlin-dotnet/build/generate.groovy
##########
@@ -31,6 +31,16 @@ import java.nio.file.Paths
import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal
+// getting an exception like:
+// > InvocationTargetException: javax.script.ScriptException: groovy.lang.MissingMethodException: No signature of
+// > method: org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal.mergeE() is applicable for
+// > argument types: (String) values: [4ffdea36-4a0e-4681-acba-e76875d1b25b]
+// usually means bindings are not being extracted properly by VarAsBindingASTTransformation which typically happens
+// when a step is taking an argument that cannot properly resolve to the type required by the step itself. there are
+// special cases in that VarAsBindingASTTransformation class which might need to be adjusted. Editing the
+// GremlinGroovyScriptEngineTest#shouldProduceBindingsForVars() with the failing step and argument can typically make
+// this issue relatively easy to debug and enforce.
Review comment:
Why not write a test for this in [GremlinGroovyScriptEngineTest](https://github.com/mikepersonick/tinkerpop/blob/TINKERPOP-2695/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineTest.java#L516)
##########
File path: tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/map/TinkerMergeVertexStep.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.tinkergraph.process.traversal.step.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeVertexStep;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerGraph;
+import org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerHelper;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Optimizes {@code mergeV()} searches by attempting to use an index where possible.
+ */
+public class TinkerMergeVertexStep<S> extends MergeVertexStep<S> {
+ public TinkerMergeVertexStep(final MergeVertexStep step) {
+ super(step.getTraversal(), step.isStart(), step.getSearchCreateTraversal());
+ if (step.getOnMatchTraversal() != null) this.addChildOption(Merge.onMatch, step.getOnMatchTraversal());
+ if (step.getOnCreateTraversal() != null) this.addChildOption(Merge.onCreate, step.getOnCreateTraversal());
+ if (step.getCallbackRegistry() != null) this.callbackRegistry = step.getCallbackRegistry();
+ }
+
+ @Override
+ protected Stream<Vertex> createSearchStream(final Map<Object, Object> search) {
Review comment:
-> Graph API
##########
File path: gremlin-dotnet/build/generate.groovy
##########
@@ -31,6 +31,16 @@ import java.nio.file.Paths
import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal
+// getting an exception like:
+// > InvocationTargetException: javax.script.ScriptException: groovy.lang.MissingMethodException: No signature of
+// > method: org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal.mergeE() is applicable for
+// > argument types: (String) values: [4ffdea36-4a0e-4681-acba-e76875d1b25b]
+// usually means bindings are not being extracted properly by VarAsBindingASTTransformation which typically happens
+// when a step is taking an argument that cannot properly resolve to the type required by the step itself. there are
+// special cases in that VarAsBindingASTTransformation class which might need to be adjusted. Editing the
+// GremlinGroovyScriptEngineTest#shouldProduceBindingsForVars() with the failing step and argument can typically make
+// this issue relatively easy to debug and enforce.
Review comment:
Looks like there is something there. Is it this?
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java
##########
@@ -0,0 +1,340 @@
+/*
+ * 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.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import 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.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.ListCallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Direction;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.Attachable;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Implementation for the {@code mergeE()} step covering both the start step version and the one used mid-traversal.
+ */
+public class MergeEdgeStep<S> extends FlatMapStep<S, Edge> implements Mutating<Event>,
+ TraversalOptionParent<Merge, S, Edge> {
+
+ private final boolean isStart;
+ private boolean first = true;
+ private Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal;
+ private Traversal.Admin<S, Map<Object, Object>> onCreateTraversal = null;
+ private Traversal.Admin<S, Map<String, Object>> onMatchTraversal = null;
+
+ protected CallbackRegistry<Event> callbackRegistry;
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart) {
+ this(traversal, isStart, new IdentityTraversal<>());
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Map<Object, Object> searchCreate) {
+ this(traversal, isStart, new ConstantTraversal<>(searchCreate));
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Traversal.Admin<?,Map<Object, Object>> searchCreateTraversal) {
+ super(traversal);
+ this.isStart = isStart;
+ this.searchCreateTraversal = integrateChild(searchCreateTraversal);
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getSearchCreateTraversal() {
+ return searchCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getOnCreateTraversal() {
+ return onCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<String, Object>> getOnMatchTraversal() {
+ return onMatchTraversal;
+ }
+
+ /**
+ * Determines if this is a start step.
+ */
+ public boolean isStart() {
+ return isStart;
+ }
+
+ public boolean isFirst() {
+ return first;
+ }
+
+ public CallbackRegistry<Event> getCallbackRegistry() {
+ return callbackRegistry;
+ }
+
+ @Override
+ public void addChildOption(final Merge token, final Traversal.Admin<S, Edge> traversalOption) {
+ if (token == Merge.onCreate) {
+ this.onCreateTraversal = this.integrateChild(traversalOption);
+ } else if (token == Merge.onMatch) {
+ this.onMatchTraversal = this.integrateChild(traversalOption);
+ } else {
+ throw new UnsupportedOperationException(String.format("Option %s for Merge is not supported", token.name()));
+ }
+ }
+
+ @Override
+ public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
+ final List<Traversal.Admin<S, E>> children = new ArrayList<>();
+ if (searchCreateTraversal != null) children.add((Traversal.Admin<S, E>) searchCreateTraversal);
+ if (onMatchTraversal != null) children.add((Traversal.Admin<S, E>) onMatchTraversal);
+ if (onCreateTraversal != null) children.add((Traversal.Admin<S, E>) onCreateTraversal);
+ return children;
+ }
+
+ @Override
+ public void configure(final Object... keyValues) {
+ // this is a Mutating step but property() should not be folded into this step. this exception should not
+ // end up visible to users really
+ }
+
+ @Override
+ public Parameters getParameters() {
+ // merge doesn't take fold ups of property() calls. those need to get treated as regular old PropertyStep
+ // instances. not sure if this should support with() though.....none of the other Mutating steps do.
+ return null;
+ }
+
+ @Override
+ protected Traverser.Admin<Edge> processNextStart() {
+ // when it's a start step a traverser needs to be created to kick off the traversal.
+ if (isStart && first) {
+ first = false;
+ final TraverserGenerator generator = this.getTraversal().getTraverserGenerator();
+ this.addStart(generator.generate(false, (Step) this, 1L));
+ }
+ return super.processNextStart();
+ }
+
+ /**
+ * Use the {@code Map} of search criteria to most efficiently return a {@code Stream<Edge>} of matching elements.
+ * Providers might override this method when extending this step to provide their own optimized mechanisms for
+ * matching the list of edges. This implementation is only optimized for the {@link T#id} so any other usage
+ * will simply be in-memory filtering which could be slow.
+ */
+ protected Stream<Edge> createSearchStream(final Map<Object,Object> search) {
Review comment:
Do you think it might make sense to move this out of here and into the Graph API? Give providers a chance to optimize.
##########
File path: tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/step/map/TinkerMergeEdgeStep.java
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.tinkergraph.process.traversal.step.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.MergeEdgeStep;
+import org.apache.tinkerpop.gremlin.structure.Direction;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerGraph;
+import org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerHelper;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Optimizes {@code mergeE()} searches by attempting to use an index where possible.
+ */
+public class TinkerMergeEdgeStep<S> extends MergeEdgeStep<S> {
+
+ public TinkerMergeEdgeStep(final MergeEdgeStep step) {
+ super(step.getTraversal(), step.isStart(), step.getSearchCreateTraversal());
+ if (step.getOnMatchTraversal() != null) this.addChildOption(Merge.onMatch, step.getOnMatchTraversal());
+ if (step.getOnCreateTraversal() != null) this.addChildOption(Merge.onCreate, step.getOnCreateTraversal());
+ if (step.getCallbackRegistry() != null) this.callbackRegistry = step.getCallbackRegistry();
+ }
+
+ @Override
+ protected Stream<Edge> createSearchStream(final Map<Object, Object> search) {
Review comment:
Same as above, perhaps this search stream business should be part of the Graph API with a default impl.
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r797542035
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/reference/ReferenceVertex.java
##########
@@ -38,6 +38,10 @@ private ReferenceVertex() {
}
+ public ReferenceVertex(final Object id) {
Review comment:
good point. a "reference" really just needs to have enough information to identify it so that it is attachable back to the graph. the label has never been required to do that and ends up being a bit superfluous for the case where the user wants to construct a reference independently of the graph as in the case:
```java
g.V(1).addE('knows').to(new ReferenceVertex(2))
```
which I believe is allowed in different language variants currently. Historically speaking, `ReferenceVertex` grew out of a need in OLAP (where the label has more relevance) and then later became the currency of Gremlin Server network serialization. Not sure if this constructor is dangerous for its convenience. I'd be inclined to leave it
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r798486220
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java
##########
@@ -0,0 +1,293 @@
+/*
+ * 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.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import 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.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.ListCallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Implementation for the {@code mergeV()} step covering both the start step version and the one used mid-traversal.
+ */
+public class MergeVertexStep<S> extends FlatMapStep<S, Vertex> implements Mutating<Event>,
+ TraversalOptionParent<Merge, S, Vertex> {
+
+ private final boolean isStart;
+ private boolean first = true;
+ private Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal;
+ private Traversal.Admin<S, Map<Object, Object>> onCreateTraversal = null;
+ private Traversal.Admin<S, Map<String, Object>> onMatchTraversal = null;
+
+ protected CallbackRegistry<Event> callbackRegistry;
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart) {
+ this(traversal, isStart, new IdentityTraversal());
+ }
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart, final Map<Object, Object> searchCreate) {
+ this(traversal, isStart, new ConstantTraversal<>(searchCreate));
+ }
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart, final Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal) {
+ super(traversal);
+ this.isStart = isStart;
+ this.searchCreateTraversal = integrateChild(searchCreateTraversal);
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getSearchCreateTraversal() {
+ return searchCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getOnCreateTraversal() {
+ return onCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<String, Object>> getOnMatchTraversal() {
+ return onMatchTraversal;
+ }
+
+ /**
+ * Determines if this is a start step.
+ */
+ public boolean isStart() {
+ return isStart;
+ }
+
+ public boolean isFirst() {
+ return first;
+ }
+
+ public CallbackRegistry<Event> getCallbackRegistry() {
+ return callbackRegistry;
+ }
+
+ @Override
+ public void addChildOption(final Merge token, final Traversal.Admin<S, Vertex> traversalOption) {
+ if (token == Merge.onCreate) {
+ this.onCreateTraversal = this.integrateChild(traversalOption);
+ } else if (token == Merge.onMatch) {
+ this.onMatchTraversal = this.integrateChild(traversalOption);
+ } else {
+ throw new UnsupportedOperationException(String.format("Option %s for Merge is not supported", token.name()));
+ }
+ }
+
+ @Override
+ public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
+ final List<Traversal.Admin<S, E>> children = new ArrayList<>();
+ if (searchCreateTraversal != null) children.add((Traversal.Admin<S, E>) searchCreateTraversal);
+ if (onMatchTraversal != null) children.add((Traversal.Admin<S, E>) onMatchTraversal);
+ if (onCreateTraversal != null) children.add((Traversal.Admin<S, E>) onCreateTraversal);
+ return children;
+ }
+
+ @Override
+ public void configure(final Object... keyValues) {
+ // this is a Mutating step but property() should not be folded into this step. this exception should not
+ // end up visible to users really
+ }
+
+ @Override
+ public Parameters getParameters() {
+ // merge doesn't take fold ups of property() calls. those need to get treated as regular old PropertyStep
+ // instances. not sure if this should support with() though.....none of the other Mutating steps do.
+ return null;
+ }
+
+ @Override
+ protected Traverser.Admin<Vertex> processNextStart() {
+ // when it's a start step a traverser needs to be created to kick off the traversal.
+ if (isStart && first) {
+ first = false;
+ final TraverserGenerator generator = this.getTraversal().getTraverserGenerator();
+ this.addStart(generator.generate(false, (Step) this, 1L));
+ }
+ return super.processNextStart();
+ }
+
+ /**
+ * Use the {@code Map} of search criteria to most efficiently return a {@code Stream<Vertex>} of matching elements.
+ * Providers might override this method when extending this step to provide their own optimized mechanisms for
+ * matching the list of vertices. This implementation is only optimized for the {@link T#id} so any other usage
+ * will simply be in-memory filtering which could be slow.
+ */
+ protected Stream<Vertex> createSearchStream(final Map<Object,Object> search) {
+ final Graph graph = this.getTraversal().getGraph().get();
+
+ Stream<Vertex> stream;
+ // prioritize lookup by id
+ if (search.containsKey(T.id))
+ stream = IteratorUtils.stream(graph.vertices(search.get(T.id)));
+ else
+ stream = IteratorUtils.stream(graph.vertices());
+
+ // in-memory filter is not going to be nice here. it will be up to graphs to optimize this step as they do
+ // for other Mutation steps
+ stream = stream.filter(v -> {
+ // try to match on all search criteria skipping T.id as it was handled above
+ return search.entrySet().stream().filter(kv -> kv.getKey() != T.id).allMatch(kv -> {
+ if (kv.getKey() == T.label) {
+ return v.label().equals(kv.getValue());
+ } else {
+ final VertexProperty<Object> vp = v.property(kv.getKey().toString());
+ return vp.isPresent() && kv.getValue().equals(vp.value());
+ }
+ });
+ });
+
+ return stream;
+ }
+
+ @Override
+ protected Iterator<Vertex> flatMap(final Traverser.Admin<S> traverser) {
+ final Map<Object,Object> searchCreate = TraversalUtil.apply(traverser, searchCreateTraversal);
+
+ Stream<Vertex> stream = createSearchStream(searchCreate);
+ stream = stream.map(v -> {
+ // if no onMatch is defined then there is no update - return the vertex unchanged
+ if (null == onMatchTraversal) return v;
+
+ // assume good input from GraphTraversal - folks might drop in a T here even though it is immutable
+ final Map<String, Object> onMatchMap = TraversalUtil.apply(traverser, onMatchTraversal);
+ 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));
+ }
+ v.property(key, value);
Review comment:
ok - we can try to detect it with `getCardinality()` off of graph features.
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r798714422
##########
File path: gremlin-test/features/map/MergeEdge.feature
##########
@@ -0,0 +1,201 @@
+# 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.
+
+@StepClassMap @StepMergeE
+Feature: Step - mergeE()
Review comment:
thanks for all the test ideas. this was an area i needed to return to. adding a test index as comments to each test file to make it easier to understand what is being tested as well
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r801741000
##########
File path: tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerGraphWorld.java
##########
@@ -106,6 +106,7 @@ public GraphTraversalSource getGraphTraversalSource(final LoadGraphWith.GraphDat
private static final List<String> TAGS_TO_IGNORE = Arrays.asList(
"@StepDrop",
+ "@StepInject",
Review comment:
Why? Won't this cause a lot of tests (i.e. Comparability/Orderability) to get ignored?
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette merged pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r801695256
##########
File path: docs/src/dev/io/graphson.asciidoc
##########
@@ -2819,6 +2779,16 @@ The following `Bytecode` example represents the traversal of `g.V().hasLabel('pe
}
----
+==== Merge
Review comment:
Why no onCreate too?
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r801784540
##########
File path: tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerGraphWorld.java
##########
@@ -106,6 +106,7 @@ public GraphTraversalSource getGraphTraversalSource(final LoadGraphWith.GraphDat
private static final List<String> TAGS_TO_IGNORE = Arrays.asList(
"@StepDrop",
+ "@StepInject",
Review comment:
Those are for OLAP tests and OLAP doesn't support `inject()` (among other things)
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette merged pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r797687584
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java
##########
@@ -0,0 +1,340 @@
+/*
+ * 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.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import 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.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.ListCallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Direction;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.Attachable;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Implementation for the {@code mergeE()} step covering both the start step version and the one used mid-traversal.
+ */
+public class MergeEdgeStep<S> extends FlatMapStep<S, Edge> implements Mutating<Event>,
+ TraversalOptionParent<Merge, S, Edge> {
+
+ private final boolean isStart;
+ private boolean first = true;
+ private Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal;
+ private Traversal.Admin<S, Map<Object, Object>> onCreateTraversal = null;
+ private Traversal.Admin<S, Map<String, Object>> onMatchTraversal = null;
+
+ protected CallbackRegistry<Event> callbackRegistry;
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart) {
+ this(traversal, isStart, new IdentityTraversal<>());
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Map<Object, Object> searchCreate) {
+ this(traversal, isStart, new ConstantTraversal<>(searchCreate));
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Traversal.Admin<?,Map<Object, Object>> searchCreateTraversal) {
+ super(traversal);
+ this.isStart = isStart;
+ this.searchCreateTraversal = integrateChild(searchCreateTraversal);
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getSearchCreateTraversal() {
+ return searchCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getOnCreateTraversal() {
+ return onCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<String, Object>> getOnMatchTraversal() {
+ return onMatchTraversal;
+ }
+
+ /**
+ * Determines if this is a start step.
+ */
+ public boolean isStart() {
+ return isStart;
+ }
+
+ public boolean isFirst() {
+ return first;
+ }
+
+ public CallbackRegistry<Event> getCallbackRegistry() {
+ return callbackRegistry;
+ }
+
+ @Override
+ public void addChildOption(final Merge token, final Traversal.Admin<S, Edge> traversalOption) {
+ if (token == Merge.onCreate) {
+ this.onCreateTraversal = this.integrateChild(traversalOption);
+ } else if (token == Merge.onMatch) {
+ this.onMatchTraversal = this.integrateChild(traversalOption);
+ } else {
+ throw new UnsupportedOperationException(String.format("Option %s for Merge is not supported", token.name()));
+ }
+ }
+
+ @Override
+ public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
+ final List<Traversal.Admin<S, E>> children = new ArrayList<>();
+ if (searchCreateTraversal != null) children.add((Traversal.Admin<S, E>) searchCreateTraversal);
+ if (onMatchTraversal != null) children.add((Traversal.Admin<S, E>) onMatchTraversal);
+ if (onCreateTraversal != null) children.add((Traversal.Admin<S, E>) onCreateTraversal);
+ return children;
+ }
+
+ @Override
+ public void configure(final Object... keyValues) {
+ // this is a Mutating step but property() should not be folded into this step. this exception should not
+ // end up visible to users really
+ }
+
+ @Override
+ public Parameters getParameters() {
+ // merge doesn't take fold ups of property() calls. those need to get treated as regular old PropertyStep
+ // instances. not sure if this should support with() though.....none of the other Mutating steps do.
+ return null;
+ }
+
+ @Override
+ protected Traverser.Admin<Edge> processNextStart() {
+ // when it's a start step a traverser needs to be created to kick off the traversal.
+ if (isStart && first) {
+ first = false;
+ final TraverserGenerator generator = this.getTraversal().getTraverserGenerator();
+ this.addStart(generator.generate(false, (Step) this, 1L));
+ }
+ return super.processNextStart();
+ }
+
+ /**
+ * Use the {@code Map} of search criteria to most efficiently return a {@code Stream<Edge>} of matching elements.
+ * Providers might override this method when extending this step to provide their own optimized mechanisms for
+ * matching the list of edges. This implementation is only optimized for the {@link T#id} so any other usage
+ * will simply be in-memory filtering which could be slow.
+ */
+ protected Stream<Edge> createSearchStream(final Map<Object,Object> search) {
Review comment:
Might not be a bad idea, though a lot of providers are just extending steps or developing their own implementations wholly. My impression is that the Graph API has become a bit less used in favor of customizations by way of strategies as I did for TinkerGraph. I'd be ok leaving that as an open issue for a future change.
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r797690433
##########
File path: gremlin-dotnet/build/generate.groovy
##########
@@ -31,6 +31,16 @@ import java.nio.file.Paths
import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal
+// getting an exception like:
+// > InvocationTargetException: javax.script.ScriptException: groovy.lang.MissingMethodException: No signature of
+// > method: org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal.mergeE() is applicable for
+// > argument types: (String) values: [4ffdea36-4a0e-4681-acba-e76875d1b25b]
+// usually means bindings are not being extracted properly by VarAsBindingASTTransformation which typically happens
+// when a step is taking an argument that cannot properly resolve to the type required by the step itself. there are
+// special cases in that VarAsBindingASTTransformation class which might need to be adjusted. Editing the
+// GremlinGroovyScriptEngineTest#shouldProduceBindingsForVars() with the failing step and argument can typically make
+// this issue relatively easy to debug and enforce.
Review comment:
yeah there is a test for it there...i just wanted a comment/reminder here in these scripts because i sometimes run afoul of this problem and then forget the reason. others would probably be completely lost without this pointer.
--
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 pull request #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#issuecomment-1026689002
The other +1 seem to be from testing the usage perspective. I am currently in midst of reviewing this PR from code perspective. Please wait for my feedback before merging.
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r796027098
##########
File path: gremlin-test/features/sideEffect/Group.feature
##########
@@ -264,17 +264,17 @@ Feature: Step - group()
| m[{"software":"d[2.0].d", "person":"d[5.0].d"}] |
# The post-ordering really isn't really right but works around TINKERPOP-2600
- Scenario: g_withSideEffectXa__marko_666_noone_blahX_V_groupXaX_byXnameX_byXoutE_label_foldX_capXaX
- Given the modern graph
- And using the parameter xx1 defined as "m[{\"marko\":[\"666\"], \"noone\":[\"blah\"]}]"
- And the traversal of
- """
- g.withSideEffect("a", xx1).V().group("a").by("name").by(__.outE().label().fold()).cap("a").unfold().group().by(keys).by(select(values).order(Scope.local).by(Order.asc))
- """
- When iterated to list
- Then the result should be unordered
- | result |
- | m[{"ripple":[], "peter":["created"], "noone":["blah"], "vadas":[], "josh":["created", "created"], "lop":[], "marko":["666", "created", "knows", "knows"]}] |
+# Scenario: g_withSideEffectXa__marko_666_noone_blahX_V_groupXaX_byXnameX_byXoutE_label_foldX_capXaX
Review comment:
please uncomment
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/reference/ReferenceVertex.java
##########
@@ -38,6 +38,10 @@ private ReferenceVertex() {
}
+ public ReferenceVertex(final Object id) {
Review comment:
An entity used for serialization such as ReferenceVertex should just be a representation for underlying stored data. Such entities should not "assume" or use defaults to fill up missing data entries.
If we are saying that a Reference Vertex cannot exist without both an ID and a label present, then this constructor should not exit.
##########
File path: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsGremlinBinaryRequestDecoder.java
##########
@@ -76,6 +76,7 @@ protected void decode(final ChannelHandlerContext channelHandlerContext, final B
try {
objects.add(serializer.deserializeRequest(messageBytes.discardReadBytes()));
} catch (SerializationException se) {
+ logger.warn(se.getMessage());
Review comment:
Message could contain PII so I am am apprehensive about logging it. It's also not possible to sanitize it since the serialization itself has failed. Why do we want to log the message in case of serialization error? wouldn't stack trace be sufficient to figure out where the serialization failed? If not, I would advocate for better stack trace from serialization instead of logging here.
same comment for other place where we are logging the message 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] divijvaidya commented on a change in pull request #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r797686317
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java
##########
@@ -0,0 +1,339 @@
+/*
+ * 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.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import 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.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.ListCallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Direction;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.util.Attachable;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Implementation for the {@code mergeE()} step covering both the start step version and the one used mid-traversal.
+ */
+public class MergeEdgeStep<S> extends FlatMapStep<S, Edge> implements Mutating<Event>,
+ TraversalOptionParent<Merge, S, Edge> {
+
+ private final boolean isStart;
+ private boolean first = true;
+ private Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal;
+ private Traversal.Admin<S, Map<Object, Object>> onCreateTraversal = null;
+ private Traversal.Admin<S, Map<String, Object>> onMatchTraversal = null;
+
+ protected CallbackRegistry<Event> callbackRegistry;
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart) {
+ this(traversal, isStart, new IdentityTraversal<>());
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Map<Object, Object> searchCreate) {
+ this(traversal, isStart, new ConstantTraversal<>(searchCreate));
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Traversal.Admin<?,Map<Object, Object>> searchCreateTraversal) {
+ super(traversal);
+ this.isStart = isStart;
+ this.searchCreateTraversal = integrateChild(searchCreateTraversal);
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getSearchCreateTraversal() {
+ return searchCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getOnCreateTraversal() {
+ return onCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<String, Object>> getOnMatchTraversal() {
+ return onMatchTraversal;
+ }
+
+ /**
+ * Determines if this is a start step.
+ */
+ public boolean isStart() {
+ return isStart;
+ }
+
+ public boolean isFirst() {
Review comment:
java doc for all public methods please
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java
##########
@@ -0,0 +1,339 @@
+/*
+ * 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.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import 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.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.ListCallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Direction;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.util.Attachable;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Implementation for the {@code mergeE()} step covering both the start step version and the one used mid-traversal.
+ */
+public class MergeEdgeStep<S> extends FlatMapStep<S, Edge> implements Mutating<Event>,
+ TraversalOptionParent<Merge, S, Edge> {
+
+ private final boolean isStart;
+ private boolean first = true;
+ private Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal;
+ private Traversal.Admin<S, Map<Object, Object>> onCreateTraversal = null;
+ private Traversal.Admin<S, Map<String, Object>> onMatchTraversal = null;
+
+ protected CallbackRegistry<Event> callbackRegistry;
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart) {
+ this(traversal, isStart, new IdentityTraversal<>());
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Map<Object, Object> searchCreate) {
+ this(traversal, isStart, new ConstantTraversal<>(searchCreate));
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Traversal.Admin<?,Map<Object, Object>> searchCreateTraversal) {
+ super(traversal);
+ this.isStart = isStart;
+ this.searchCreateTraversal = integrateChild(searchCreateTraversal);
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getSearchCreateTraversal() {
+ return searchCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getOnCreateTraversal() {
+ return onCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<String, Object>> getOnMatchTraversal() {
+ return onMatchTraversal;
+ }
+
+ /**
+ * Determines if this is a start step.
+ */
+ public boolean isStart() {
+ return isStart;
+ }
+
+ public boolean isFirst() {
+ return first;
+ }
+
+ public CallbackRegistry<Event> getCallbackRegistry() {
+ return callbackRegistry;
+ }
+
+ @Override
+ public void addChildOption(final Merge token, final Traversal.Admin<S, Edge> traversalOption) {
+ if (token == Merge.onCreate) {
+ this.onCreateTraversal = this.integrateChild(traversalOption);
+ } else if (token == Merge.onMatch) {
+ this.onMatchTraversal = this.integrateChild(traversalOption);
+ } else {
+ throw new UnsupportedOperationException(String.format("Option %s for Merge is not supported", token.name()));
+ }
+ }
+
+ @Override
+ public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
+ final List<Traversal.Admin<S, E>> children = new ArrayList<>();
+ if (searchCreateTraversal != null) children.add((Traversal.Admin<S, E>) searchCreateTraversal);
+ if (onMatchTraversal != null) children.add((Traversal.Admin<S, E>) onMatchTraversal);
+ if (onCreateTraversal != null) children.add((Traversal.Admin<S, E>) onCreateTraversal);
+ return children;
+ }
+
+ @Override
+ public void configure(final Object... keyValues) {
+ // this is a Mutating step but property() should not be folded into this step. this exception should not
Review comment:
please add a test case to validate that (if not already added)
##########
File path: gremlin-test/features/map/MergeEdge.feature
##########
@@ -0,0 +1,201 @@
+# 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.
+
+@StepClassMap @StepMergeE
+Feature: Step - mergeE()
+
+ Scenario: g_V_mergeEXlabel_self_weight_05X
+ 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]\": \"self\", \"weight\":\"d[0.5].d\"}]"
+ And the traversal of
+ """
+ g.V().mergeE(xx1)
+ """
+ When iterated to list
+ Then the result should have a count of 1
+ And the graph should return 1 for count of "g.E().hasLabel(\"self\").has(\"weight\",0.5)"
Review comment:
please also assert that it is connected to the correct vertex here (marko).
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java
##########
@@ -0,0 +1,293 @@
+/*
+ * 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.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import 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.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.ListCallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Implementation for the {@code mergeV()} step covering both the start step version and the one used mid-traversal.
+ */
+public class MergeVertexStep<S> extends FlatMapStep<S, Vertex> implements Mutating<Event>,
+ TraversalOptionParent<Merge, S, Vertex> {
+
+ private final boolean isStart;
+ private boolean first = true;
+ private Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal;
+ private Traversal.Admin<S, Map<Object, Object>> onCreateTraversal = null;
+ private Traversal.Admin<S, Map<String, Object>> onMatchTraversal = null;
+
+ protected CallbackRegistry<Event> callbackRegistry;
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart) {
+ this(traversal, isStart, new IdentityTraversal());
+ }
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart, final Map<Object, Object> searchCreate) {
+ this(traversal, isStart, new ConstantTraversal<>(searchCreate));
+ }
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart, final Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal) {
+ super(traversal);
+ this.isStart = isStart;
+ this.searchCreateTraversal = integrateChild(searchCreateTraversal);
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getSearchCreateTraversal() {
+ return searchCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getOnCreateTraversal() {
+ return onCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<String, Object>> getOnMatchTraversal() {
+ return onMatchTraversal;
+ }
+
+ /**
+ * Determines if this is a start step.
+ */
+ public boolean isStart() {
+ return isStart;
+ }
+
+ public boolean isFirst() {
+ return first;
+ }
+
+ public CallbackRegistry<Event> getCallbackRegistry() {
+ return callbackRegistry;
+ }
+
+ @Override
+ public void addChildOption(final Merge token, final Traversal.Admin<S, Vertex> traversalOption) {
+ if (token == Merge.onCreate) {
+ this.onCreateTraversal = this.integrateChild(traversalOption);
+ } else if (token == Merge.onMatch) {
+ this.onMatchTraversal = this.integrateChild(traversalOption);
+ } else {
+ throw new UnsupportedOperationException(String.format("Option %s for Merge is not supported", token.name()));
+ }
+ }
+
+ @Override
+ public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
+ final List<Traversal.Admin<S, E>> children = new ArrayList<>();
+ if (searchCreateTraversal != null) children.add((Traversal.Admin<S, E>) searchCreateTraversal);
+ if (onMatchTraversal != null) children.add((Traversal.Admin<S, E>) onMatchTraversal);
+ if (onCreateTraversal != null) children.add((Traversal.Admin<S, E>) onCreateTraversal);
+ return children;
+ }
+
+ @Override
+ public void configure(final Object... keyValues) {
+ // this is a Mutating step but property() should not be folded into this step. this exception should not
+ // end up visible to users really
+ }
+
+ @Override
+ public Parameters getParameters() {
+ // merge doesn't take fold ups of property() calls. those need to get treated as regular old PropertyStep
+ // instances. not sure if this should support with() though.....none of the other Mutating steps do.
+ return null;
+ }
+
+ @Override
+ protected Traverser.Admin<Vertex> processNextStart() {
+ // when it's a start step a traverser needs to be created to kick off the traversal.
+ if (isStart && first) {
+ first = false;
+ final TraverserGenerator generator = this.getTraversal().getTraverserGenerator();
+ this.addStart(generator.generate(false, (Step) this, 1L));
+ }
+ return super.processNextStart();
+ }
+
+ /**
+ * Use the {@code Map} of search criteria to most efficiently return a {@code Stream<Vertex>} of matching elements.
+ * Providers might override this method when extending this step to provide their own optimized mechanisms for
+ * matching the list of vertices. This implementation is only optimized for the {@link T#id} so any other usage
+ * will simply be in-memory filtering which could be slow.
+ */
+ protected Stream<Vertex> createSearchStream(final Map<Object,Object> search) {
+ final Graph graph = this.getTraversal().getGraph().get();
+
+ Stream<Vertex> stream;
+ // prioritize lookup by id
+ if (search.containsKey(T.id))
+ stream = IteratorUtils.stream(graph.vertices(search.get(T.id)));
+ else
+ stream = IteratorUtils.stream(graph.vertices());
+
+ // in-memory filter is not going to be nice here. it will be up to graphs to optimize this step as they do
+ // for other Mutation steps
+ stream = stream.filter(v -> {
+ // try to match on all search criteria skipping T.id as it was handled above
+ return search.entrySet().stream().filter(kv -> kv.getKey() != T.id).allMatch(kv -> {
+ if (kv.getKey() == T.label) {
+ return v.label().equals(kv.getValue());
+ } else {
+ final VertexProperty<Object> vp = v.property(kv.getKey().toString());
+ return vp.isPresent() && kv.getValue().equals(vp.value());
+ }
+ });
+ });
+
+ return stream;
+ }
+
+ @Override
+ protected Iterator<Vertex> flatMap(final Traverser.Admin<S> traverser) {
+ final Map<Object,Object> searchCreate = TraversalUtil.apply(traverser, searchCreateTraversal);
+
+ Stream<Vertex> stream = createSearchStream(searchCreate);
+ stream = stream.map(v -> {
+ // if no onMatch is defined then there is no update - return the vertex unchanged
+ if (null == onMatchTraversal) return v;
+
+ // assume good input from GraphTraversal - folks might drop in a T here even though it is immutable
+ final Map<String, Object> onMatchMap = TraversalUtil.apply(traverser, onMatchTraversal);
+ 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));
+ }
+ v.property(key, value);
Review comment:
This line assumes that the cardinality will be the default one which is incorrect.
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java
##########
@@ -0,0 +1,339 @@
+/*
+ * 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.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import 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.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.ListCallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Direction;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.util.Attachable;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Implementation for the {@code mergeE()} step covering both the start step version and the one used mid-traversal.
+ */
+public class MergeEdgeStep<S> extends FlatMapStep<S, Edge> implements Mutating<Event>,
+ TraversalOptionParent<Merge, S, Edge> {
+
+ private final boolean isStart;
+ private boolean first = true;
+ private Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal;
+ private Traversal.Admin<S, Map<Object, Object>> onCreateTraversal = null;
+ private Traversal.Admin<S, Map<String, Object>> onMatchTraversal = null;
+
+ protected CallbackRegistry<Event> callbackRegistry;
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart) {
+ this(traversal, isStart, new IdentityTraversal<>());
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Map<Object, Object> searchCreate) {
+ this(traversal, isStart, new ConstantTraversal<>(searchCreate));
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Traversal.Admin<?,Map<Object, Object>> searchCreateTraversal) {
+ super(traversal);
+ this.isStart = isStart;
+ this.searchCreateTraversal = integrateChild(searchCreateTraversal);
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getSearchCreateTraversal() {
+ return searchCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getOnCreateTraversal() {
+ return onCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<String, Object>> getOnMatchTraversal() {
+ return onMatchTraversal;
+ }
+
+ /**
+ * Determines if this is a start step.
+ */
+ public boolean isStart() {
+ return isStart;
+ }
+
+ public boolean isFirst() {
+ return first;
+ }
+
+ public CallbackRegistry<Event> getCallbackRegistry() {
+ return callbackRegistry;
+ }
+
+ @Override
+ public void addChildOption(final Merge token, final Traversal.Admin<S, Edge> traversalOption) {
+ if (token == Merge.onCreate) {
+ this.onCreateTraversal = this.integrateChild(traversalOption);
+ } else if (token == Merge.onMatch) {
+ this.onMatchTraversal = this.integrateChild(traversalOption);
+ } else {
+ throw new UnsupportedOperationException(String.format("Option %s for Merge is not supported", token.name()));
+ }
+ }
+
+ @Override
+ public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
+ final List<Traversal.Admin<S, E>> children = new ArrayList<>();
+ if (searchCreateTraversal != null) children.add((Traversal.Admin<S, E>) searchCreateTraversal);
+ if (onMatchTraversal != null) children.add((Traversal.Admin<S, E>) onMatchTraversal);
+ if (onCreateTraversal != null) children.add((Traversal.Admin<S, E>) onCreateTraversal);
+ return children;
+ }
+
+ @Override
+ public void configure(final Object... keyValues) {
+ // this is a Mutating step but property() should not be folded into this step. this exception should not
+ // end up visible to users really
+ }
+
+ @Override
+ public Parameters getParameters() {
+ // merge doesn't take fold ups of property() calls. those need to get treated as regular old PropertyStep
+ // instances. not sure if this should support with() though.....none of the other Mutating steps do.
+ return null;
+ }
+
+ @Override
+ protected Traverser.Admin<Edge> processNextStart() {
+ // when it's a start step a traverser needs to be created to kick off the traversal.
+ if (isStart && first) {
+ first = false;
+ final TraverserGenerator generator = this.getTraversal().getTraverserGenerator();
+ this.addStart(generator.generate(false, (Step) this, 1L));
+ }
+ return super.processNextStart();
+ }
+
+ /**
+ * Use the {@code Map} of search criteria to most efficiently return a {@code Stream<Edge>} of matching elements.
+ * Providers might override this method when extending this step to provide their own optimized mechanisms for
+ * matching the list of edges. This implementation is only optimized for the {@link T#id} so any other usage
+ * will simply be in-memory filtering which could be slow.
+ */
+ protected Stream<Edge> createSearchStream(final Map<Object,Object> search) {
+ final Graph graph = this.getTraversal().getGraph().get();
+
+ final Optional<Direction> directionUsedInLookup;
+ Stream<Edge> stream;
+ // prioritize lookup by id, then use vertices as starting point if possible
+ if (search.containsKey(T.id)) {
+ stream = IteratorUtils.stream(graph.edges(search.get(T.id)));
+ directionUsedInLookup = Optional.empty();
+ } else if (search.containsKey(Direction.OUT)) {
+ stream = IteratorUtils.stream(graph.vertices(search.get(Direction.OUT))).flatMap(v -> IteratorUtils.stream(v.edges(Direction.OUT)));
+ directionUsedInLookup = Optional.of(Direction.OUT);
+ } else if (search.containsKey(Direction.IN)) {
+ stream = IteratorUtils.stream(graph.vertices(search.get(Direction.IN))).flatMap(v -> IteratorUtils.stream(v.edges(Direction.IN)));
+ directionUsedInLookup = Optional.of(Direction.IN);
+ } else {
+ stream = IteratorUtils.stream(graph.edges());
+ directionUsedInLookup = Optional.empty();
+ }
+
+ // in-memory filter is not going to be nice here. it will be up to graphs to optimize this step as they do
+ // for other Mutation steps
+ stream = stream.filter(e -> {
+ // try to match on all search criteria skipping T.id as it was handled above
+ return search.entrySet().stream().filter(
Review comment:
nit
This lambda could be extracted into a `Predicate`
##########
File path: gremlin-test/features/map/MergeEdge.feature
##########
@@ -0,0 +1,201 @@
+# 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.
+
+@StepClassMap @StepMergeE
+Feature: Step - mergeE()
Review comment:
Please add the following test scenarios (similar cases for mergeV as well):
1. Uses g.mergeE() but does not specify OUT in the map
2. Uses g.mergeE() but does not specify IN in the map
3. Uses g.mergeE() but does not specify either OUT or IN in the map
4. Uses g.mergeE() but does not specify LABEL in the map
5. Uses g.V(1).mergeE() but specifies OUT in the map which is different than 1
6. Wrong syntax for the map (this case could go in grammar test)
7. lower case OUT/IN in the map (this case could go in grammar test)
8. test for mergeE(traversal) --> (I am quite curious to see the use case for this scenario)
9. mergeE() at the beginning of a child traversal
10. mergeE() in the middle a child traversal
11. mergeV() with insertion of property having different cardinality (list/set/single)
12. mergeV() when existing vertex has single cardinality but new vertex wants to add same property with set cardinality
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java
##########
@@ -0,0 +1,339 @@
+/*
+ * 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.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import 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.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.ListCallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Direction;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.util.Attachable;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Implementation for the {@code mergeE()} step covering both the start step version and the one used mid-traversal.
+ */
+public class MergeEdgeStep<S> extends FlatMapStep<S, Edge> implements Mutating<Event>,
+ TraversalOptionParent<Merge, S, Edge> {
+
+ private final boolean isStart;
+ private boolean first = true;
+ private Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal;
+ private Traversal.Admin<S, Map<Object, Object>> onCreateTraversal = null;
+ private Traversal.Admin<S, Map<String, Object>> onMatchTraversal = null;
+
+ protected CallbackRegistry<Event> callbackRegistry;
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart) {
+ this(traversal, isStart, new IdentityTraversal<>());
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Map<Object, Object> searchCreate) {
+ this(traversal, isStart, new ConstantTraversal<>(searchCreate));
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Traversal.Admin<?,Map<Object, Object>> searchCreateTraversal) {
+ super(traversal);
+ this.isStart = isStart;
+ this.searchCreateTraversal = integrateChild(searchCreateTraversal);
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getSearchCreateTraversal() {
+ return searchCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getOnCreateTraversal() {
+ return onCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<String, Object>> getOnMatchTraversal() {
+ return onMatchTraversal;
+ }
+
+ /**
+ * Determines if this is a start step.
+ */
+ public boolean isStart() {
+ return isStart;
+ }
+
+ public boolean isFirst() {
+ return first;
+ }
+
+ public CallbackRegistry<Event> getCallbackRegistry() {
+ return callbackRegistry;
+ }
+
+ @Override
+ public void addChildOption(final Merge token, final Traversal.Admin<S, Edge> traversalOption) {
+ if (token == Merge.onCreate) {
+ this.onCreateTraversal = this.integrateChild(traversalOption);
+ } else if (token == Merge.onMatch) {
+ this.onMatchTraversal = this.integrateChild(traversalOption);
+ } else {
+ throw new UnsupportedOperationException(String.format("Option %s for Merge is not supported", token.name()));
+ }
+ }
+
+ @Override
+ public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
+ final List<Traversal.Admin<S, E>> children = new ArrayList<>();
+ if (searchCreateTraversal != null) children.add((Traversal.Admin<S, E>) searchCreateTraversal);
+ if (onMatchTraversal != null) children.add((Traversal.Admin<S, E>) onMatchTraversal);
+ if (onCreateTraversal != null) children.add((Traversal.Admin<S, E>) onCreateTraversal);
+ return children;
+ }
+
+ @Override
+ public void configure(final Object... keyValues) {
+ // this is a Mutating step but property() should not be folded into this step. this exception should not
+ // end up visible to users really
+ }
+
+ @Override
+ public Parameters getParameters() {
+ // merge doesn't take fold ups of property() calls. those need to get treated as regular old PropertyStep
+ // instances. not sure if this should support with() though.....none of the other Mutating steps do.
+ return null;
+ }
+
+ @Override
+ protected Traverser.Admin<Edge> processNextStart() {
+ // when it's a start step a traverser needs to be created to kick off the traversal.
+ if (isStart && first) {
+ first = false;
+ final TraverserGenerator generator = this.getTraversal().getTraverserGenerator();
+ this.addStart(generator.generate(false, (Step) this, 1L));
+ }
+ return super.processNextStart();
+ }
+
+ /**
+ * Use the {@code Map} of search criteria to most efficiently return a {@code Stream<Edge>} of matching elements.
+ * Providers might override this method when extending this step to provide their own optimized mechanisms for
+ * matching the list of edges. This implementation is only optimized for the {@link T#id} so any other usage
+ * will simply be in-memory filtering which could be slow.
+ */
+ protected Stream<Edge> createSearchStream(final Map<Object,Object> search) {
+ final Graph graph = this.getTraversal().getGraph().get();
+
+ final Optional<Direction> directionUsedInLookup;
+ Stream<Edge> stream;
+ // prioritize lookup by id, then use vertices as starting point if possible
+ if (search.containsKey(T.id)) {
+ stream = IteratorUtils.stream(graph.edges(search.get(T.id)));
+ directionUsedInLookup = Optional.empty();
+ } else if (search.containsKey(Direction.OUT)) {
+ stream = IteratorUtils.stream(graph.vertices(search.get(Direction.OUT))).flatMap(v -> IteratorUtils.stream(v.edges(Direction.OUT)));
+ directionUsedInLookup = Optional.of(Direction.OUT);
+ } else if (search.containsKey(Direction.IN)) {
+ stream = IteratorUtils.stream(graph.vertices(search.get(Direction.IN))).flatMap(v -> IteratorUtils.stream(v.edges(Direction.IN)));
+ directionUsedInLookup = Optional.of(Direction.IN);
+ } else {
+ stream = IteratorUtils.stream(graph.edges());
+ directionUsedInLookup = Optional.empty();
+ }
+
+ // in-memory filter is not going to be nice here. it will be up to graphs to optimize this step as they do
+ // for other Mutation steps
+ stream = stream.filter(e -> {
+ // try to match on all search criteria skipping T.id as it was handled above
+ return search.entrySet().stream().filter(
+ kv -> kv.getKey() != T.id && !(directionUsedInLookup.isPresent() && kv.getKey() == directionUsedInLookup.get())).
+ allMatch(kv -> {
+ if (kv.getKey() == T.label) {
+ return e.label().equals(kv.getValue());
+ } else if (kv.getKey() instanceof Direction) {
+ final Direction direction = (Direction) kv.getKey();
+
+ // try to take advantage of string id conversions of the graph by doing a lookup rather
+ // than direct compare on id
+ final Iterator<Vertex> found = graph.vertices(kv.getValue());
+ return found.hasNext() && e.vertices(direction).next().equals(found.next());
Review comment:
Iterator leak here. Please close the iterator once it has been used.
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeEdgeStep.java
##########
@@ -0,0 +1,340 @@
+/*
+ * 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.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import 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.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.ListCallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Direction;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.Attachable;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Implementation for the {@code mergeE()} step covering both the start step version and the one used mid-traversal.
+ */
+public class MergeEdgeStep<S> extends FlatMapStep<S, Edge> implements Mutating<Event>,
+ TraversalOptionParent<Merge, S, Edge> {
+
+ private final boolean isStart;
+ private boolean first = true;
+ private Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal;
+ private Traversal.Admin<S, Map<Object, Object>> onCreateTraversal = null;
+ private Traversal.Admin<S, Map<String, Object>> onMatchTraversal = null;
+
+ protected CallbackRegistry<Event> callbackRegistry;
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart) {
+ this(traversal, isStart, new IdentityTraversal<>());
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Map<Object, Object> searchCreate) {
+ this(traversal, isStart, new ConstantTraversal<>(searchCreate));
+ }
+
+ public MergeEdgeStep(final Traversal.Admin traversal, final boolean isStart, final Traversal.Admin<?,Map<Object, Object>> searchCreateTraversal) {
+ super(traversal);
+ this.isStart = isStart;
+ this.searchCreateTraversal = integrateChild(searchCreateTraversal);
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getSearchCreateTraversal() {
+ return searchCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getOnCreateTraversal() {
+ return onCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<String, Object>> getOnMatchTraversal() {
+ return onMatchTraversal;
+ }
+
+ /**
+ * Determines if this is a start step.
+ */
+ public boolean isStart() {
+ return isStart;
+ }
+
+ public boolean isFirst() {
+ return first;
+ }
+
+ public CallbackRegistry<Event> getCallbackRegistry() {
+ return callbackRegistry;
+ }
+
+ @Override
+ public void addChildOption(final Merge token, final Traversal.Admin<S, Edge> traversalOption) {
+ if (token == Merge.onCreate) {
+ this.onCreateTraversal = this.integrateChild(traversalOption);
+ } else if (token == Merge.onMatch) {
+ this.onMatchTraversal = this.integrateChild(traversalOption);
+ } else {
+ throw new UnsupportedOperationException(String.format("Option %s for Merge is not supported", token.name()));
+ }
+ }
+
+ @Override
+ public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
+ final List<Traversal.Admin<S, E>> children = new ArrayList<>();
+ if (searchCreateTraversal != null) children.add((Traversal.Admin<S, E>) searchCreateTraversal);
+ if (onMatchTraversal != null) children.add((Traversal.Admin<S, E>) onMatchTraversal);
+ if (onCreateTraversal != null) children.add((Traversal.Admin<S, E>) onCreateTraversal);
+ return children;
+ }
+
+ @Override
+ public void configure(final Object... keyValues) {
+ // this is a Mutating step but property() should not be folded into this step. this exception should not
+ // end up visible to users really
+ }
+
+ @Override
+ public Parameters getParameters() {
+ // merge doesn't take fold ups of property() calls. those need to get treated as regular old PropertyStep
+ // instances. not sure if this should support with() though.....none of the other Mutating steps do.
+ return null;
+ }
+
+ @Override
+ protected Traverser.Admin<Edge> processNextStart() {
+ // when it's a start step a traverser needs to be created to kick off the traversal.
+ if (isStart && first) {
+ first = false;
+ final TraverserGenerator generator = this.getTraversal().getTraverserGenerator();
+ this.addStart(generator.generate(false, (Step) this, 1L));
+ }
+ return super.processNextStart();
+ }
+
+ /**
+ * Use the {@code Map} of search criteria to most efficiently return a {@code Stream<Edge>} of matching elements.
+ * Providers might override this method when extending this step to provide their own optimized mechanisms for
+ * matching the list of edges. This implementation is only optimized for the {@link T#id} so any other usage
+ * will simply be in-memory filtering which could be slow.
+ */
+ protected Stream<Edge> createSearchStream(final Map<Object,Object> search) {
Review comment:
+1
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MergeVertexStep.java
##########
@@ -0,0 +1,293 @@
+/*
+ * 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.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Merge;
+import 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.TraverserGenerator;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.ListCallbackRegistry;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.EventStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Graph;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.T;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Stream;
+
+/**
+ * Implementation for the {@code mergeV()} step covering both the start step version and the one used mid-traversal.
+ */
+public class MergeVertexStep<S> extends FlatMapStep<S, Vertex> implements Mutating<Event>,
+ TraversalOptionParent<Merge, S, Vertex> {
+
+ private final boolean isStart;
+ private boolean first = true;
+ private Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal;
+ private Traversal.Admin<S, Map<Object, Object>> onCreateTraversal = null;
+ private Traversal.Admin<S, Map<String, Object>> onMatchTraversal = null;
+
+ protected CallbackRegistry<Event> callbackRegistry;
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart) {
+ this(traversal, isStart, new IdentityTraversal());
+ }
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart, final Map<Object, Object> searchCreate) {
+ this(traversal, isStart, new ConstantTraversal<>(searchCreate));
+ }
+
+ public MergeVertexStep(final Traversal.Admin traversal, final boolean isStart, final Traversal.Admin<S,Map<Object, Object>> searchCreateTraversal) {
+ super(traversal);
+ this.isStart = isStart;
+ this.searchCreateTraversal = integrateChild(searchCreateTraversal);
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getSearchCreateTraversal() {
+ return searchCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<Object, Object>> getOnCreateTraversal() {
+ return onCreateTraversal;
+ }
+
+ public Traversal.Admin<S, Map<String, Object>> getOnMatchTraversal() {
+ return onMatchTraversal;
+ }
+
+ /**
+ * Determines if this is a start step.
+ */
+ public boolean isStart() {
+ return isStart;
+ }
+
+ public boolean isFirst() {
+ return first;
+ }
+
+ public CallbackRegistry<Event> getCallbackRegistry() {
+ return callbackRegistry;
+ }
+
+ @Override
+ public void addChildOption(final Merge token, final Traversal.Admin<S, Vertex> traversalOption) {
+ if (token == Merge.onCreate) {
+ this.onCreateTraversal = this.integrateChild(traversalOption);
+ } else if (token == Merge.onMatch) {
+ this.onMatchTraversal = this.integrateChild(traversalOption);
+ } else {
+ throw new UnsupportedOperationException(String.format("Option %s for Merge is not supported", token.name()));
+ }
+ }
+
+ @Override
+ public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
+ final List<Traversal.Admin<S, E>> children = new ArrayList<>();
+ if (searchCreateTraversal != null) children.add((Traversal.Admin<S, E>) searchCreateTraversal);
+ if (onMatchTraversal != null) children.add((Traversal.Admin<S, E>) onMatchTraversal);
+ if (onCreateTraversal != null) children.add((Traversal.Admin<S, E>) onCreateTraversal);
+ return children;
+ }
+
+ @Override
+ public void configure(final Object... keyValues) {
+ // this is a Mutating step but property() should not be folded into this step. this exception should not
+ // end up visible to users really
+ }
+
+ @Override
+ public Parameters getParameters() {
+ // merge doesn't take fold ups of property() calls. those need to get treated as regular old PropertyStep
+ // instances. not sure if this should support with() though.....none of the other Mutating steps do.
+ return null;
+ }
+
+ @Override
+ protected Traverser.Admin<Vertex> processNextStart() {
+ // when it's a start step a traverser needs to be created to kick off the traversal.
+ if (isStart && first) {
+ first = false;
+ final TraverserGenerator generator = this.getTraversal().getTraverserGenerator();
+ this.addStart(generator.generate(false, (Step) this, 1L));
+ }
+ return super.processNextStart();
+ }
+
+ /**
+ * Use the {@code Map} of search criteria to most efficiently return a {@code Stream<Vertex>} of matching elements.
+ * Providers might override this method when extending this step to provide their own optimized mechanisms for
+ * matching the list of vertices. This implementation is only optimized for the {@link T#id} so any other usage
+ * will simply be in-memory filtering which could be slow.
+ */
+ protected Stream<Vertex> createSearchStream(final Map<Object,Object> search) {
+ final Graph graph = this.getTraversal().getGraph().get();
+
+ Stream<Vertex> stream;
+ // prioritize lookup by id
+ if (search.containsKey(T.id))
+ stream = IteratorUtils.stream(graph.vertices(search.get(T.id)));
+ else
+ stream = IteratorUtils.stream(graph.vertices());
+
+ // in-memory filter is not going to be nice here. it will be up to graphs to optimize this step as they do
+ // for other Mutation steps
+ stream = stream.filter(v -> {
+ // try to match on all search criteria skipping T.id as it was handled above
+ return search.entrySet().stream().filter(kv -> kv.getKey() != T.id).allMatch(kv -> {
+ if (kv.getKey() == T.label) {
+ return v.label().equals(kv.getValue());
+ } else {
+ final VertexProperty<Object> vp = v.property(kv.getKey().toString());
+ return vp.isPresent() && kv.getValue().equals(vp.value());
+ }
+ });
+ });
+
+ return stream;
+ }
+
+ @Override
+ protected Iterator<Vertex> flatMap(final Traverser.Admin<S> traverser) {
+ final Map<Object,Object> searchCreate = TraversalUtil.apply(traverser, searchCreateTraversal);
+
+ Stream<Vertex> stream = createSearchStream(searchCreate);
+ stream = stream.map(v -> {
+ // if no onMatch is defined then there is no update - return the vertex unchanged
+ if (null == onMatchTraversal) return v;
+
+ // assume good input from GraphTraversal - folks might drop in a T here even though it is immutable
+ final Map<String, Object> onMatchMap = TraversalUtil.apply(traverser, onMatchTraversal);
+ 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()) {
Review comment:
I missed any tests in this PR which test the event stream for these cases
##########
File path: gremlin-test/features/map/MergeEdge.feature
##########
@@ -0,0 +1,201 @@
+# 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.
+
+@StepClassMap @StepMergeE
+Feature: Step - mergeE()
+
+ Scenario: g_V_mergeEXlabel_self_weight_05X
+ 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]\": \"self\", \"weight\":\"d[0.5].d\"}]"
+ And the traversal of
+ """
+ g.V().mergeE(xx1)
+ """
+ When iterated to list
+ Then the result should have a count of 1
+ And the graph should return 1 for count of "g.E().hasLabel(\"self\").has(\"weight\",0.5)"
+
+ @UserSuppliedVertexIds
+ Scenario: g_mergeEXlabel_knows_out_marko_in_vadasX
+ Given the empty graph
+ And the graph initializer of
+ """
+ g.addV("person").property(T.id, 100).property("name", "marko").
+ addV("person").property(T.id, 101).property("name", "vadas")
+ """
+ And using the parameter xx1 defined as "m[{\"t[label]\": \"knows\", \"D[OUT]\":\"v[100]\", \"D[IN]\":\"v[101]\"}]"
+ And the traversal of
+ """
+ g.mergeE(xx1)
+ """
+ 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\").out(\"knows\").has(\"person\",\"name\",\"vadas\")"
+
+ @UserSuppliedVertexIds
+ Scenario: g_mergeEXlabel_knows_out_marko_in_vadas_weight_05X_exists
+ Given the empty graph
+ And the graph initializer of
+ """
+ g.addV("person").property(T.id, 100).property("name", "marko").as("a").
+ addV("person").property(T.id, 101).property("name", "vadas").as("b").
+ addE("knows").from("a").to("b")
+ """
+ And using the parameter xx1 defined as "m[{\"t[label]\": \"knows\", \"D[OUT]\":\"v[100]\", \"D[IN]\":\"v[101]\", \"weight\":\"d[0.5].d\"}]"
+ And the traversal of
+ """
+ g.mergeE(xx1)
+ """
+ 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\").outE(\"knows\").has(\"weight\",0.5).inV().has(\"person\",\"name\",\"vadas\")"
+ And the graph should return 2 for count of "g.V().has(\"person\",\"name\",\"marko\").outE(\"knows\").inV().has(\"person\",\"name\",\"vadas\")"
Review comment:
nit
outE().inV() could be replaced with out()
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#issuecomment-1029529040
not sure who else is looking at this but you may want to pump the brakes a bit. i found some holes in `mergeE()` while writing more tests and kelvin, as he was writing documentation, suggested a small refinement to me privately to allow the `Direction` options to take an `T.id` in addition to a `Vertex` which should streamline the syntax a bit. he also suggested a slightly larger adjustment that would allow for `from` and `to` usage rather than `OUT` and `IN` which is sort of appealing as it matches the Gremlin edge creation syntax. still thinking about the impact of that one, but I like how it sounds. I will post back when i have some more code pushed for all this as well as the existing feedback already on this PR. Thanks for all the attention this has been given.
--
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 #1555: TINKERPOP-2681 mergeE() and mergeV()
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#issuecomment-1033295095
I think I've gotten this solidified now. Hopefully there is no more significant change. Thanks for the early reviews. @krlawrence is still planning to follow up with reference documentation and will arrive on a later PR in time for release. Note the addition of the new Gremlin step semantic documentation - hopefully it establishes a nice pattern for future expansion to cover the other steps.
--
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