Remove GenericArgument from from() and to()#3252
Conversation
xiazcy
force-pushed
the
from-to-id-fix
branch
from
c419963 to
fbb9f29
Compare
Cole-Greer
force-pushed
the
from-to-id-fix
branch
from
fbb9f29 to
fceb8d9
Compare
|
VOTE +1 |
|
VOTE +1 |
| gremlin> String.format("g.addE('knows').from(__.V(%s)).to(__.constant(%s))", v1.id(), v6.id()) | ||
| ==>g.addE('knows').from(__.V(1)).to(__.constant(6)) | ||
| gremlin> client.submit(script).all().get().get(0).getEdge() |
There was a problem hiding this comment.
These commands do not work when executed in the console
There was a problem hiding this comment.
Those examples are assuming a pre-existing client to submit remote scripts to a server. It's not really intended to be executable, the purpose is more to demonstrate how to properly pass from() and to() arguments in a script instead of embedding a ReferenceVertex.
| And the traversal of | ||
| """ | ||
| g.addE("knows").from(vid1).to(vid6).property("weight", xx1) | ||
| g.addE("knows").from(V(vid1)).to(V(vid6)).property("weight", xx1) |
There was a problem hiding this comment.
The scenario names for the 2 modified tests should be updated (they seem out of date even prior to these changes).
| And the traversal of | ||
| """ | ||
| g.addE(xx1).from(vid1).to(vid6).property("weight", xx2) | ||
| g.addE(xx1).from(V(vid1)).to(V(vid6)).property("weight", xx2) |
There was a problem hiding this comment.
Would be nice to change one of the tests to use constant instead of V.
There was a problem hiding this comment.
Unfortunately use of constant() here doesn't quite work as must be passed in as query parameters (to support providers with differing ID management systems), and constant does not take parameters. I will leave this test using V().
|
Nit: missing CHANGELOG |
|
I don't understand the need for this PR still after reviewing it. Can you maybe expand a bit about it in the documentation? What problem exactly is it trying to fix? |
This PR is really a followup to an unintended consequence stemming from #3133. The issue with directly allowing ID's to be passed into from/to, it creates a weird asymmetry for graphs which use Strings as their ids. For example (Adding this to the PR description as well) |
I'm not considering a new changelog entry as necessary as this is really a refinement of a previous unreleased change in 3.8-dev. |
from() and to() will now only accept Strings (alias for __.select()) and Traversals. If users wish to pass a vertex id, they may embed it in __.V() or __.constant(). The primary motivation here is to ensure consistent behaviour accross providers regardless of if their id types are numerics, strings, or other.
Co-authored-by: andreachild <andrea.child@improving.com>
Co-authored-by: andreachild <andrea.child@improving.com>
Cole-Greer
force-pushed
the
from-to-id-fix
branch
from
49d5e48 to
b924695
Compare
|
VOTE +1 |
) Fixes edge creation in gremlin-python by automatically wrapping Vertex objects with __.V(vertex.id) in to() and from_() methods. Key Changes: • Wrap Vertex objects with __.V() in to() and from_() steps using isinstance() check • Pass through strings and traversals unchanged • Update test expectations to verify Vertex wrapping functionality • Revert manual __.V() wrapping from basic examples now that driver handles it automatically Relates to #3252
4 participants
from() and to() will now only accept Strings (alias for __.select()) and Traversals. If users wish to pass a vertex id, they may embed it in __.V() or __.constant().
The primary motivation here is to ensure consistent behaviour accross providers regardless of if their id types are numerics, strings, or other.
This PR is really a followup to an unintended consequence stemming from #3133. The issue with directly allowing ID's to be passed into from/to, it creates a weird asymmetry for graphs which use Strings as their ids. For example g.addE().from(1).to(2) would work fine and create an edge from V[1] to V[2], however if the graph used String ids, the equivalent traversal (g.addE().from("a").to("b")) does not work, as the Strings are interpreted as path labels (converted to __.select("a")) instead of vertex ids. There are some ugly workarounds which can partially mitigate this issue, but overall there is no generalized fix which allows all traversals to behave the same regardless of the vertex id type.