-
Notifications
You must be signed in to change notification settings - Fork 829
TINKERPOP-3023 Expand type syntax in grammar in 3.8 - Remove Vertex #3133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.8-dev
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.8-dev #3133 +/- ##
==========================================
Coverage ? 76.52%
Complexity ? 13829
==========================================
Files ? 1109
Lines ? 69787
Branches ? 7545
==========================================
Hits ? 53404
Misses ? 13617
Partials ? 2766 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this now a valid traversal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously was invalid but now works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
.../src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStartStep.java
Outdated
Show resolved
Hide resolved
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs
Outdated
Show resolved
Hide resolved
@@ -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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
...lin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/MergeEdge.feature
Show resolved
Hide resolved
I've pushed some changes into the branch which updates the docs a bit and strips out the vertex -> id sugar in GraphTraversal as it's not required for bytecode. (to be added to master in a separate PR) My vote is VOTE +1 |
Note: Followup PR for master: #3138 |
Feat: Remove Vertex support from grammar, and for places where a vertex is required, pass in the vertex id instead at traversal construction.
The grammar currently includes inconsistencies, particularly around "type constructors" like ReferenceVertex(), which are being re-evaluated.
ReferenceVertex should be renamed simply to Vertex, as the distinction (e.g., between ReferenceVertex and DetachedVertex) is no longer meaningful or guaranteed.
The keyword new should be removed from the grammar rule for structureVertex, as it's syntactically unnecessary.
The Vertex constructor should only require an id, not a label. Labels are not utilized in practice (e.g., in g.V(Vertex)), and can default to an empty string.
Resolves: 3023,
For reference...
https://lists.apache.org/thread/ptq81160z4y4vddg4lxz2nv244x2yzhq