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

Add new method ApplyTag #317

Merged
merged 13 commits into from Feb 21, 2024
Merged

Conversation

haoxins
Copy link
Collaborator

@haoxins haoxins commented Feb 15, 2024

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

ApplyTag applies the given tag to the graph.

1. If the tag does not exist, it will be created.
2. If the tag exists, it will be checked if the fields are the same.
2.1 If not, the new fields will be added.
2.2 If the field type is different, it will return an error.
2.3 If a field exists in the graph but not in the given tag,
it will be removed.

Notice:
We won't change the field type because it has
unexpected behavior for the data.

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2024

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (b220b12) 63.70% compared to head (747dec2) 64.32%.

Files Patch % Lines
schema_manager.go 75.00% 10 Missing and 4 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #317      +/-   ##
==========================================
+ Coverage   63.70%   64.32%   +0.61%     
==========================================
  Files          10       11       +1     
  Lines        2579     2643      +64     
==========================================
+ Hits         1643     1700      +57     
- Misses        797      802       +5     
- Partials      139      141       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@haoxins haoxins force-pushed the apply-edge-vertex branch 4 times, most recently from fb1aa2d to 503af42 Compare February 15, 2024 12:46
@haoxins haoxins changed the title WIP - ApplyTag Add new method ApplyTag Feb 15, 2024
@haoxins haoxins marked this pull request as ready for review February 15, 2024 12:50
@wey-gu
Copy link

wey-gu commented Feb 19, 2024

This sugar on Client Side is so very inspiring.

We should consider do similar things on all Clients.

@wey-gu wey-gu requested a review from Nicole00 February 19, 2024 03:46
@Nicole00
Copy link
Contributor

This is a very convenient interface that can provide users with an encapsulated and convenient interface. Thanks for your idea.
I'm thinking whether it's appropriate to put such syntactic sugar inside the client? Are their application scenarios general enough? After all, the client serves as an SDK rather than an ORM framework. Maybe we can add these sugar in another go file to differentiate from session or session_pool. What's your idea? @haoxin @wey-gu

@haoxins
Copy link
Collaborator Author

haoxins commented Feb 20, 2024

Maybe we can add these sugar in another go file to differentiate from session or session_pool.

The idea of applying xxx comes from the cloud-native community (or K8s), I think it is the basic requirement of schema management.

The benefit is that we don't need to pass session_pool if we add this into the session_pool struct.

But I agree that it does not belong to session_pool/session.
Do you have an idea for the name of this?

@haoxins
Copy link
Collaborator Author

haoxins commented Feb 20, 2024

SchemaManager?

@Nicole00
Copy link
Contributor

SchemaManager?

LGTM. cc @wey-gu

@wey-gu
Copy link

wey-gu commented Feb 20, 2024

Agree with @Nicole00 that it's strange to be placed under the session_pool tent, SchemaManager sounds great to me, too :D

@haoxins
Copy link
Collaborator Author

haoxins commented Feb 20, 2024

done cc @Nicole00 @wey-gu

@wey-gu wey-gu added the doc affected PR: improvements or additions to documentation label Feb 20, 2024
wey-gu
wey-gu previously approved these changes Feb 20, 2024
@wey-gu
Copy link

wey-gu commented Feb 20, 2024

We could later have some example/docs to highlight this sweet change :D

@haoxins
Copy link
Collaborator Author

haoxins commented Feb 20, 2024

We could later have some example/docs to highlight this sweet change :D

I have another PR for ApplyEdge on the way.
BTW, I don't see the docs for Nebula client or SDKs :)

schema_manager.go Outdated Show resolved Hide resolved
schema_manager.go Outdated Show resolved Hide resolved
@Nicole00
Copy link
Contributor

We could later have some example/docs to highlight this sweet change :D

I have another PR for ApplyEdge on the way. BTW, I don't see the docs for Nebula client or SDKs :)

Great! Looking forward to your pr~ we have sdk api doc: https://pkg.go.dev/github.com/vesoft-inc/nebula-go/v3

Copy link
Contributor

@Nicole00 Nicole00 left a comment

Choose a reason for hiding this comment

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

LGTM, Great sugar!

@Nicole00 Nicole00 merged commit 2fd8560 into vesoft-inc:master Feb 21, 2024
23 checks passed
@haoxins haoxins deleted the apply-edge-vertex branch February 21, 2024 09:17
@haoxins haoxins mentioned this pull request Feb 21, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc affected PR: improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants