-
Notifications
You must be signed in to change notification settings - Fork 30
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
MergeV, MergeE, & Option Steps #214
Conversation
support converting a TraversalBuilder into GValue via Bytecode
…t literal maps being defined for mergeV steps
…n order to leverage v2's wait flag
@wolf4ood didn't realize this got over 1k lines 😅 , let me know what you think. Happy to make changes 👍 . |
…ream for only Null terminated traversals
Hey @criminosis thanks for the hard work let me know when the PR is ready for review i will take a look :) |
Yeah I started tinkering with other stuff 😅 . Lemme back out the exploratory logging & and the experimental connection muxing. Are you okay with the rest of the stuff going in as a single PR? Might take a while to splice everything else out at this point 🤔 |
if it's ready for review as it is i would keep it in a single PR, I might have some time in the next days to go through it |
@@ -23,7 +23,7 @@ jobs: | |||
- uses: actions/checkout@v2 | |||
- name: Starting Gremlin Servers | |||
run: | | |||
docker-compose -f ./docker-compose/docker-compose.yaml up -d |
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.
Seems docker-compose
(note the middle -
is no longer a known command on the latest action runners, so changed it here and in the test workflow files
@wolf4ood think I've got it tuned back to being in a ready to merge state 👍 . I updated the OP with I think the additional items since I stopped updating it. Whenever you're able to review it and it gets merged mind cutting a release please? |
- name: Run cargo test with blocking client | ||
if: matrix.gremlin-server == '3.5.7' | ||
uses: actions-rs/cargo@v1 | ||
with: | ||
command: test | ||
args: --manifest-path gremlin-client/Cargo.toml |
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.
By chance I bumped into a compilation error for the blocking I/O side fixed in d56179d while working on a follow-up branch for this PR. Surprised me since this PR had been building successfully, but that's when I noticed there didn't seem to be a cargo test call without either async flag. If there was the build should have broken, so I added one.
Hey @wolf4ood do you think you'd still be able to review this in the near future? I'm working on implementing the GraphBinary protocol into this library (the follow-up PR I mentioned previously). It's going to involve moving "load bearing walls" to make space for it though. A handful of places I've noticed seem to assume JSON based serialization & deserialization, so it's still gonna take a few days but it's eventually gonna be stuck behind this one merging. It may end up being best to do a PR that tries to make that space first, and then put up the PR that actually adds the GraphBinary protocol. I'm striving to hide everything behind the serializer/deserializer options configured in the client, but I think it's going to be unavoidable as a breaking change if people were explicitly setting their serializer/deserializer. I'll leave the default of GraphSONV3 though. Because at a bare minimum I'd like to rename that enum to a more generic sounding name. Otherwise it looks like:
Which doesn't make much sense. Tentatively moving them into:
|
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.
LGTM
thanks @criminosis
This PR does the following:
property_many
,property_with_cardinality
, andproperty_many_with_cardinality
mergeV
,mergeE
,option
option
formergeV
&mergeE
steps, seeintegration_traversal.rs
for examplesto
&from
options formergeE
as wellmergeV
&mergeE
!= 3.5.6
will be a higher version. The merge steps don't exist in that family of Tinkerpop.mergeV & mergeE
to WRITE_OPERATORS inbytecode.rs
g:Column
viaby()
stepsideEffect()
step implementationproperties()
step in an anonymous traversalNone
stepiterator()
on returned stream for async client