Skip to content
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

Merged
merged 54 commits into from
Oct 16, 2024
Merged

Conversation

criminosis
Copy link
Contributor

@criminosis criminosis commented Jul 13, 2024

This PR does the following:

  • Changed property() step to key by Into, this allows custom vertex ids to be assigned
    • Also updated property_many, property_with_cardinality, and property_many_with_cardinality
  • Added new steps: mergeV, mergeE, option
    • Added support for using option for mergeV & mergeE steps, see integration_traversal.rs for examples
    • Added support for using the to & from options for mergeE as well
  • Implemented T::id into a GKey to support injected map parameters for mergeV & mergeE
    • Also facilitates addV("some_label").properties(T::id, "string_or_integer_id_here")...
  • Added JG to docker compose environment for custom string vertex id tests
    • @wolf4ood the JG container doesn't participate in gremlin version matrix, so the custom vertex ids tests are being ran in each matrix's entry (and JG is ignoring the specified Tinkerpop version). Happy to make changes for how you'd think it'd be better to do. Thought about adding another feature flag for those tests, but figured it'd be good to get your thoughts first.
    • Added a feature to selectively run merge tests for TP versions != 3.5.6
      • Assumes != 3.5.6 will be a higher version. The merge steps don't exist in that family of Tinkerpop.
  • Added mergeV & mergeE to WRITE_OPERATORS in bytecode.rs
    • @wolf4ood this seems like an investigative call upon the bytecode so just stuck them in, lemme know if there's more to do.
  • Support choose step options with literals
  • Support g:Column via by() step
  • Reworked sideEffect() step implementation
  • Permit properties() step in an anonymous traversal
  • Modified Websocket connection layer error handling and propagation
  • Updated to mobc 0.8 family
  • Implemented None step
  • Implemented Tinkerpop-ism of iterator() on returned stream for async client

support converting a TraversalBuilder into GValue via Bytecode
…t literal maps being defined for mergeV steps
@criminosis
Copy link
Contributor Author

@wolf4ood didn't realize this got over 1k lines 😅 , let me know what you think. Happy to make changes 👍 .

This was referenced Jul 13, 2024
@criminosis criminosis changed the title New steps MergeV, MergeE, Option Steps Jul 13, 2024
@criminosis criminosis changed the title MergeV, MergeE, Option Steps MergeV, MergeE, & Option Steps Jul 13, 2024
@wolf4ood
Copy link
Owner

Hey @criminosis

thanks for the hard work let me know when the PR is ready for review i will take a look :)

@criminosis
Copy link
Contributor Author

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 🤔

@wolf4ood
Copy link
Owner

@criminosis

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
Copy link
Contributor Author

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

@criminosis
Copy link
Contributor Author

@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?

Comment on lines +44 to +49
- 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
Copy link
Contributor Author

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.

@criminosis
Copy link
Contributor Author

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:

pub enum GraphSON {
    V2,
    V3,
   GraphBinaryV1
}

Which doesn't make much sense. Tentatively moving them into:

pub enum IoProtocol {
    GraphSONV2,
    GraphSONV3,
    GraphBinaryV1
}

Copy link
Owner

@wolf4ood wolf4ood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

thanks @criminosis

@wolf4ood wolf4ood merged commit d2505d3 into wolf4ood:master Oct 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants