Skip to content
This repository has been archived by the owner on Dec 1, 2022. It is now read-only.

Delete tag #541

Merged
merged 3 commits into from
Aug 5, 2021
Merged

Delete tag #541

merged 3 commits into from
Aug 5, 2021

Conversation

critical27
Copy link
Contributor

for (const auto& entry : delTags) {
const auto& vId = entry.get_id().getStr();
for (const auto& tagId : entry.get_tags()) {
auto key = NebulaKeyUtils::vertexKey(spaceVidLen_, partId, vId, tagId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we delete edge when delete last tag of vertex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could consider later, this involves how we deal with dangle edges.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep same with delete vertex but it's difficult to do. So maybe could let delete tag can't delete the last tag.

Copy link
Contributor Author

@critical27 critical27 Aug 4, 2021

Choose a reason for hiding this comment

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

Why delete tag need to keep same with delete vertex? We need to read once for each tag? Besides, how can we make sure the tag we are deleting is the last one? (We need to lock the vid prefix to make sure no one is doing insert/update)

Delete vertex need to be handled with txn.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could add a special tag with id 0, in this case delete tag won't delete vertex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not just the delete tag's problem. Since we allow dangling edges, we only handle delete last tag -> delete vertex is meaningless.

Copy link
Contributor

@bright-starry-sky bright-starry-sky left a comment

Choose a reason for hiding this comment

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

Good job, the overall logic is fine for me.

src/storage/mutate/DeleteTagsProcessor.cpp Show resolved Hide resolved
src/storage/mutate/DeleteTagsProcessor.cpp Outdated Show resolved Hide resolved
src/storage/mutate/DeleteTagsProcessor.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@bright-starry-sky bright-starry-sky left a comment

Choose a reason for hiding this comment

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

LGTM,nice job.

Copy link
Contributor

@panda-sheep panda-sheep left a comment

Choose a reason for hiding this comment

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

Woohoo!

Copy link
Contributor

@liuyu85cn liuyu85cn left a comment

Choose a reason for hiding this comment

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

LGTM

@critical27 critical27 added the ready-for-testing PR: ready for the CI test label Aug 5, 2021
@critical27 critical27 merged commit 7f5dfbc into vesoft-inc:master Aug 5, 2021
@critical27 critical27 deleted the delete branch August 5, 2021 10:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants