[
https://issues.apache.org/jira/browse/TINKERPOP-3023?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17974011#comment-17974011
]
ASF GitHub Bot commented on TINKERPOP-3023:
-------------------------------------------
Cole-Greer commented on code in PR #3133:
URL: https://github.com/apache/tinkerpop/pull/3133#discussion_r2145529496
##########
docs/src/recipes/between-vertices.asciidoc:
##########
@@ -22,14 +22,14 @@ traversal on the paths found between them. Consider the
following examples using
[gremlin-groovy,modern]
----
-g.V(1).bothE() <1>
-g.V(1).bothE().where(otherV().hasId(2)) <2>
+g.V(1).bothE() <1>
+g.V(1).bothE().where(otherV().hasId(2)) <2>
v1 = g.V(1).next();[]
v2 = g.V(2).next();[]
-g.V(v1).bothE().where(otherV().is(v2)) <3>
-g.V(v1).outE().where(inV().is(v2)) <4>
-g.V(1).outE().where(inV().has(id, within(2,3))) <5>
-g.V(1).out().where(__.in().hasId(6)) <6>
+g.V(v1.id()).bothE().where(otherV().id().is(v2.id())) <3>
Review Comment:
I'm not sure if we want to change these examples in the docs. My expectation
is that most users will continue to use the sugar syntax of directly passing
vertices in both the GLVs and console. It's simpler to read and understand if
we leave it with the sugar syntax.
In order to keep it with the sugared syntax, we will need to update the
[DocumentationReader](https://github.com/apache/tinkerpop/blob/3.8-dev/gremlin-language/src/main/java/org/apache/tinkerpop/gremlin/language/corpus/DocumentationReader.java)
to ignore these cases. I think we can either add these examples to the
[`throwAway` ignore set at the top of the
class](https://github.com/apache/tinkerpop/blob/6c0459863758f5f4966631786985306ce8478532/gremlin-language/src/main/java/org/apache/tinkerpop/gremlin/language/corpus/DocumentationReader.java#L42),
or we can change the [replace
patterns](https://github.com/apache/tinkerpop/blob/6c0459863758f5f4966631786985306ce8478532/gremlin-language/src/main/java/org/apache/tinkerpop/gremlin/language/corpus/DocumentationReader.java#L113-L114)
to translate v1 and v2 to something safe like `"1"` and `"2"` similarly to how
`vBob` is handled above.
Thoughts?
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
@@ -30,6 +30,41 @@ complete list of all the modifications that are part of this
release.
=== Upgrading for Users
+==== Removal of Vertex/ReferenceVertex from gtammar
+
+Previously the use of Vertex/ReferenceVertex existed in the grammar, these
have now been removed. Going forward, vertex ID should
Review Comment:
I think it's important to make clear that the removal is only in the
grammar, and thus only applies to users directly writing gremlin-lang scripts.
Vertex as an argument to these steps has been retained as sugared syntax in
`GraphTraversal` for all of the GLVs, as well as in gremlin-groovy. This means
that this change should not be apparent when for GLV or console users.
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
@@ -30,6 +30,41 @@ complete list of all the modifications that are part of this
release.
=== Upgrading for Users
+==== Removal of Vertex/ReferenceVertex from gtammar
+
+Previously the use of Vertex/ReferenceVertex existed in the grammar, these
have now been removed. Going forward, vertex ID should
+be used in traversals.
+
+// 3.7.3
+gremlin> graph = TinkerFactory.createModern()
+==>tinkergraph[vertices:6 edges:6]
+gremlin> g = graph.traversal()
+==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
+gremlin> v1 = g.V(1).next();
+==>v[1]
+gremlin> v2 = g.V(2).next();
+==>v[2]
+gremlin> g.V(v1).outE().where(inV().is(v2))
+==>e[7][1-knows->2]
+
+// 3.8.0
+gremlin> graph = TinkerFactory.createModern()
+==>tinkergraph[vertices:6 edges:6]
+gremlin> g = graph.traversal()
+==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
Review Comment:
Nit:
```suggestion
gremlin> g = TinkerFactory.createModern().traversal()
==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
```
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -862,6 +862,22 @@ public GraphTraversal<TStart, string> Format(string format)
return Wrap<TStart, string>(this);
}
+ /// <summary>
+ /// Adds the from step to this <see cref="GraphTraversal{SType,
EType}" />.
+ /// </summary>
+ public GraphTraversal<TStart, TEnd> From (object? fromStepLabel)
+ {
+ if (fromStepLabel is string fromStepLabelString)
+ {
+ return From(fromStepLabelString);
+ }
+ if (fromStepLabel is int fromStepLabelSInteger)
+ {
+ return From(new Vertex(fromStepLabelSInteger));
+ }
+ throw new ArgumentException($"{fromStepLabel} is not a valid step
label");
+ }
Review Comment:
This same sort of "pop the id out of vertices" approach should apply to the
equivalent GraphTraversal methods in all of the GLVs.
##########
gremlin-language/src/test/resources/incorrect-traversals.txt:
##########
@@ -23,7 +23,6 @@ g.V().horder().by(id)
g.mergeE(["weight": 0.5, Direction.out: 1])
g.mergeE(["weight": 0.5, Direction.in: 1])
g.addE('knows').from(g.V(1))
-g.addE('knows').from(g.V(1).next())
Review Comment:
This is interesting. from() in the grammar previously only allowed a
nestedTraversal, but not a terminatedTraversal. By putting `genericArgument` in
there to accept any arbitrary ID type, we've now allowed terminatedTraversal to
leak in. I don't think this is a big concern.
@spmallette any thoughts on this?
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStartStep.java:
##########
@@ -110,27 +111,45 @@ protected Traverser.Admin<Edge> processNextStart() {
final String edgeLabel = (String) this.parameters.get(traverser,
T.label, () -> Edge.DEFAULT_LABEL).get(0);
// FROM/TO must be set and must be vertices
- final Object theTo = this.parameters.get(traverser, TO, () ->
null).get(0);
- if (!(theTo instanceof Vertex))
+ Object theTo = this.parameters.get(traverser, TO, () ->
null).get(0);
+ if (theTo != null && !(theTo instanceof Vertex)) {
+ theTo = new ReferenceVertex(theTo);
+ }
+
+ if (theTo == null)
throw new IllegalStateException(String.format(
- "The value given to addE(%s).to() must resolve to a
Vertex but %s was specified instead", edgeLabel,
- null == theTo ? "null" :
theTo.getClass().getSimpleName()));
+ "The value given to addE(%s).to() must resolve to a
Vertex but null was specified instead", edgeLabel));
Review Comment:
```suggestion
"The value given to addE(%s).to() must resolve to a
Vertex or the ID of a Vertex present in the graph, but null was specified
instead", edgeLabel));
```
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/MergeEdge.feature:
##########
@@ -151,7 +151,7 @@ Feature: Step - mergeE()
g.addV("person").property("name", "marko").
addV("person").property("name", "vadas")
"""
- And using the parameter xx1 defined as "m[{\"t[label]\": \"knows\",
\"D[OUT]\":\"v[marko]\", \"D[IN]\":\"v[vadas]\"}]"
+ And using the parameter xx1 defined as "m[{\"t[label]\": \"knows\",
\"D[OUT]\":\"v[marko].id\", \"D[IN]\":\"v[vadas].id\"}]"
Review Comment:
These mergeE cases worry me. Once this gets merged up to TinkerPop 4 and
GraphTraversal starts generating GremlinLang instead of Bytecode, Vertex will
no longer be allowed in the mergeE merge map. This would be an unintended
breaking change.
It's a bit messy but I wonder if we need to add that same sort of `Vertex ->
Id` syntax sugar in the mergeE map at the GraphTraversal level in each GLV
similar to what has been done in from() and to().
Thoughts?
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStartStep.java:
##########
@@ -110,27 +111,45 @@ protected Traverser.Admin<Edge> processNextStart() {
final String edgeLabel = (String) this.parameters.get(traverser,
T.label, () -> Edge.DEFAULT_LABEL).get(0);
// FROM/TO must be set and must be vertices
- final Object theTo = this.parameters.get(traverser, TO, () ->
null).get(0);
- if (!(theTo instanceof Vertex))
+ Object theTo = this.parameters.get(traverser, TO, () ->
null).get(0);
+ if (theTo != null && !(theTo instanceof Vertex)) {
+ theTo = new ReferenceVertex(theTo);
+ }
+
+ if (theTo == null)
throw new IllegalStateException(String.format(
- "The value given to addE(%s).to() must resolve to a
Vertex but %s was specified instead", edgeLabel,
- null == theTo ? "null" :
theTo.getClass().getSimpleName()));
+ "The value given to addE(%s).to() must resolve to a
Vertex but null was specified instead", edgeLabel));
- final Object theFrom = this.parameters.get(traverser, FROM, () ->
null).get(0);
- if (!(theFrom instanceof Vertex))
+ Object theFrom = this.parameters.get(traverser, FROM, () ->
null).get(0);
+ if (theFrom != null && !(theFrom instanceof Vertex)) {
+ theFrom = new ReferenceVertex(theFrom);
+ }
+ if (theFrom == null)
throw new IllegalStateException(String.format(
- "The value given to addE(%s).from() must resolve to a
Vertex but %s was specified instead", edgeLabel,
- null == theFrom ? "null" :
theFrom.getClass().getSimpleName()));
+ "The value given to addE(%s).from() must resolve to a
Vertex but null was specified instead", edgeLabel));
Review Comment:
```suggestion
"The value given to addE(%s).from() must resolve to
a Vertex or the ID of a Vertex present in the graph, but null was specified
instead", edgeLabel));
```
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
@@ -30,6 +30,41 @@ complete list of all the modifications that are part of this
release.
=== Upgrading for Users
+==== Removal of Vertex/ReferenceVertex from gtammar
Review Comment:
```suggestion
==== Removal of Vertex/ReferenceVertex from grammar
```
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
@@ -30,6 +30,41 @@ complete list of all the modifications that are part of this
release.
=== Upgrading for Users
+==== Removal of Vertex/ReferenceVertex from gtammar
+
+Previously the use of Vertex/ReferenceVertex existed in the grammar, these
have now been removed. Going forward, vertex ID should
+be used in traversals.
+
+// 3.7.3
+gremlin> graph = TinkerFactory.createModern()
+==>tinkergraph[vertices:6 edges:6]
+gremlin> g = graph.traversal()
+==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
Review Comment:
Nit: For brevity
```suggestion
gremlin> g = TinkerFactory.createModern().traversal()
==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
```
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java:
##########
@@ -98,43 +99,61 @@ public void addFrom(final Traversal.Admin<?, ?> fromObject)
{
protected Edge map(final Traverser.Admin<S> traverser) {
final String edgeLabel = this.parameters.get(traverser, T.label, () ->
Edge.DEFAULT_LABEL).get(0);
- final Object theTo;
+ Object theTo;
try {
theTo = this.parameters.get(traverser, TO, traverser::get).get(0);
+ if (theTo != null && !(theTo instanceof Vertex)) {
+ theTo = new ReferenceVertex(theTo);
+ }
} catch (IllegalArgumentException e) { // as thrown by
TraversalUtil.apply()
throw new IllegalStateException(String.format(
"addE(%s) failed because the to() traversal (which should
give a Vertex) failed with: %s",
edgeLabel, e.getMessage()));
}
- if (!(theTo instanceof Vertex))
+ if (theTo == null)
throw new IllegalStateException(String.format(
- "The value given to addE(%s).to() must resolve to a Vertex
but %s was specified instead", edgeLabel,
- null == theTo ? "null" :
theTo.getClass().getSimpleName()));
+ "The value given to addE(%s).to() must resolve to a Vertex
but null was specified instead", edgeLabel));
Review Comment:
```suggestion
"The value given to addE(%s).to() must resolve to a
Vertex or the ID of a Vertex present in the graph, but null was specified
instead", edgeLabel));
```
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -2193,6 +2209,22 @@ public GraphTraversal<TStart, Vertex> To (Direction?
direction, params string?[]
Bytecode.AddStep("to", args.ToArray());
return Wrap<TStart, Vertex>(this);
}
+
+ /// <summary>
+ /// Adds the to step to this <see cref="GraphTraversal{SType,
EType}" />.
+ /// </summary>
+ public GraphTraversal<TStart, TEnd> To (object? toStepLabel)
+ {
Review Comment:
See above suggestion for From()
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java:
##########
@@ -98,43 +99,61 @@ public void addFrom(final Traversal.Admin<?, ?> fromObject)
{
protected Edge map(final Traverser.Admin<S> traverser) {
final String edgeLabel = this.parameters.get(traverser, T.label, () ->
Edge.DEFAULT_LABEL).get(0);
- final Object theTo;
+ Object theTo;
try {
theTo = this.parameters.get(traverser, TO, traverser::get).get(0);
+ if (theTo != null && !(theTo instanceof Vertex)) {
+ theTo = new ReferenceVertex(theTo);
+ }
} catch (IllegalArgumentException e) { // as thrown by
TraversalUtil.apply()
throw new IllegalStateException(String.format(
"addE(%s) failed because the to() traversal (which should
give a Vertex) failed with: %s",
edgeLabel, e.getMessage()));
}
- if (!(theTo instanceof Vertex))
+ if (theTo == null)
throw new IllegalStateException(String.format(
- "The value given to addE(%s).to() must resolve to a Vertex
but %s was specified instead", edgeLabel,
- null == theTo ? "null" :
theTo.getClass().getSimpleName()));
+ "The value given to addE(%s).to() must resolve to a Vertex
but null was specified instead", edgeLabel));
- final Object theFrom;
+ Object theFrom;
try {
theFrom = this.parameters.get(traverser, FROM,
traverser::get).get(0);
+ if (theFrom != null && !(theFrom instanceof Vertex)) {
+ theFrom = new ReferenceVertex(theFrom);
+ }
} catch (IllegalArgumentException e) { // as thrown by
TraversalUtil.apply()
throw new IllegalStateException(String.format(
"addE(%s) failed because the from() traversal (which
should give a Vertex) failed with: %s",
edgeLabel, e.getMessage()));
}
- if (!(theFrom instanceof Vertex))
+ if (theFrom == null)
throw new IllegalStateException(String.format(
- "The value given to addE(%s).to() must resolve to a Vertex
but %s was specified instead", edgeLabel,
- null == theFrom ? "null" :
theFrom.getClass().getSimpleName()));
+ "The value given to addE(%s).to() must resolve to a Vertex
but null was specified instead", edgeLabel));
Review Comment:
```suggestion
"The value given to addE(%s).from() must resolve to a
Vertex or the ID of a Vertex present in the graph, but null was specified
instead", edgeLabel));
```
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -862,6 +862,22 @@ public GraphTraversal<TStart, string> Format(string format)
return Wrap<TStart, string>(this);
}
+ /// <summary>
+ /// Adds the from step to this <see cref="GraphTraversal{SType,
EType}" />.
+ /// </summary>
+ public GraphTraversal<TStart, TEnd> From (object? fromStepLabel)
+ {
+ if (fromStepLabel is string fromStepLabelString)
+ {
+ return From(fromStepLabelString);
+ }
+ if (fromStepLabel is int fromStepLabelSInteger)
+ {
+ return From(new Vertex(fromStepLabelSInteger));
+ }
+ throw new ArgumentException($"{fromStepLabel} is not a valid step
label");
+ }
Review Comment:
I don't believe it's necessary to split into the more specific overloads
here, and I don't think the `ArgumentException` is right. `object?
fromStepLabel` could be any arbitrary vertex id, which could be of any type.
The one special case which should be handled is that if `object? fromStepLabel`
is a `vertex`, the GLV should handle that pop out the ID and pass that along
instead. Other than that one case, it should be fine to simply pass the id as
an `object` into the bytecode for this step.
```suggestion
public GraphTraversal<TStart, TEnd> From (object?
fromStepVertexIdOrLabel)
{
if (fromStepVertexIdOrLabel is Vertex fromStepVertex)
{
fromStepVertexIdOrLabel = fromStepVertex.Id;
}
Bytecode.AddStep("from", fromStepVertexIdOrLabel);
return Wrap<TStart, TEnd>(this);
}
```
Similar should apply for the To() step below
> Expand type syntax in grammar in 3.8
> ------------------------------------
>
> Key: TINKERPOP-3023
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3023
> Project: TinkerPop
> Issue Type: Improvement
> Components: language
> Affects Versions: 3.8.0
> Reporter: Stephen Mallette
> Priority: Major
>
> There are certain types commonly used in Gremlin that should have some native
> support in the grammar:
> * UUID
> ** {{UUID()}} - random
> ** {{UUID('1e077e63-e45a-4f8e-aa00-9b6ffd91f20e')}}
> * Edge
> ** {{Edge(11)}} - different than current syntax for Vertex, adjust Vertex to
> match - drop grammar support for {{{}ReferenceVertex{}}}.
> * Binary (ByteBuffer)
> ** {{Binary( '/9j/4AAQSkZJRgABAQEAAAAAAAD/==')}}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
