Skip to content

Commit

Permalink
Use vertex key only when use_vertex_key is on (vesoft-inc#1405)
Browse files Browse the repository at this point in the history
<!--
Thanks for your contribution!
In order to review PR more efficiently, please add information according to the template.
-->

## What type of PR is this?
- [ ] bug
- [ ] feature
- [X] enhancement

## What problem(s) does this PR solve?
#### Issue(s) number: 

#### Description:
PRD of vertex key changed again... Here is the new version of logic:
1. No matter use_vertex_key is on or off, the query result should be consistent with 3.1.
2. If use_vertex_key is on, we will use the vertex key to detect whether the vertex exists, which is the logic in 3.X version.
3. If use_vertex_key is off, we will prefix scan by vertex to detect whether the vertex exists, **which will bring performance regression**
4. We will insert the vertex only when use_vertex_key is on

> By default, use_vertex_key is off

## How do you solve it?
This PR reverts all related changes before (vesoft-inc#4685 vesoft-inc#4680 vesoft-inc#4675 vesoft-inc#4629), and apply the logic above. 

Related changes:
1. So the upgrade tool is necessary again, so bring it back.
2. The TCK cases keeps same as before vesoft-inc#4629


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

> Maybe review the last commit is enough 


## Checklist:
Tests:
- [X] Unit test(positive and negative cases)
- [ ] Function test
- [ ] Performance test
- [ ] N/A

Affects:
- [X] Documentation affected (Please add the label if documentation needs to be modified.)
- [ ] Incompatibility (If it breaks the compatibility, please describe it and add the label.)
- [ ] If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
- [ ] Performance impacted: Consumes more CPU/Memory


## Release notes:

Please confirm whether to be reflected in release notes and how to describe:
> ex. Fixed the bug .....


Migrated from vesoft-inc#4716

Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com>
  • Loading branch information
nebula-bots and critical27 committed Oct 14, 2022
1 parent 454a3a2 commit eba755c
Show file tree
Hide file tree
Showing 18 changed files with 230 additions and 60 deletions.
2 changes: 1 addition & 1 deletion docker/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@ Following docker images will be ready in production.
- [reg.vesoft-inc.com/vesoft-ent/nebula-graphd](https://reg.vesoft-inc.com/harbor/projects/80/repositories/nebula-graphd): nebula-graphd service built with `Dockerfile.graphd`
- [reg.vesoft-inc.com/vesoft-ent/nebula-metad](https://reg.vesoft-inc.com/harbor/projects/80/repositories/nebula-metad): nebula-metad service built with `Dockerfile.metad`
- [reg.vesoft-inc.com/vesoft-ent/nebula-storaged](https://reg.vesoft-inc.com/harbor/projects/80/repositories/nebula-storaged): nebula-storaged service built with `Dockerfile.storaged`
- [reg.vesoft-inc.com/vesoft-ent/nebula-tools](https://reg.vesoft-inc.com/harbor/projects/80/repositories/nebula-tools): nebula tools built with `Dockerfile.tools`, including db_dump, meta_dump and db_upgrader
- [reg.vesoft-inc.com/vesoft-ent/nebula-tools](https://reg.vesoft-inc.com/harbor/projects/80/repositories/nebula-tools): nebula tools built with `Dockerfile.tools`, including db_dump and meta_dump

1 change: 1 addition & 0 deletions src/common/utils/NebulaKeyUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ std::vector<std::string> NebulaKeyUtils::snapshotPrefix(PartitionID partId) {
if (partId == 0) {
result.emplace_back("");
} else {
result.emplace_back(vertexPrefix(partId));
result.emplace_back(tagPrefix(partId));
result.emplace_back(edgePrefix(partId));
result.emplace_back(IndexKeyUtils::indexPrefix(partId));
Expand Down
2 changes: 0 additions & 2 deletions src/graph/service/GraphFlags.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -180,5 +180,3 @@ DEFINE_uint32(
gc_worker_size,
0,
"Background garbage clean workers, default number is 0 which means using hardware core size.");

DEFINE_bool(graph_use_vertex_key, false, "whether allow insert or query the vertex key");
3 changes: 0 additions & 3 deletions src/graph/service/GraphFlags.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,4 @@ DECLARE_int32(max_job_size);

DECLARE_bool(enable_async_gc);
DECLARE_uint32(gc_worker_size);

DECLARE_bool(graph_use_vertex_key);

#endif // GRAPH_GRAPHFLAGS_H_
3 changes: 0 additions & 3 deletions src/graph/validator/MutateValidator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,6 @@ Status InsertVerticesValidator::check() {
}

auto tagItems = sentence->tagItems();
if (!FLAGS_graph_use_vertex_key && tagItems.empty()) {
return Status::SemanticError("Insert vertex is forbidden, please speicify the tag");
}

schemas_.reserve(tagItems.size());
for (auto &item : tagItems) {
Expand Down
17 changes: 14 additions & 3 deletions src/storage/exec/GetPropNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@ class GetTagPropNode : public QueryNode<VertexID> {
return ret;
}

// If none of the tag node valid, will check vertex key if use_vertex_key is true,
// do not emplace the row if the flag is false
// If none of the tag node valid, will check if vertex exists:
// 1. if use_vertex_key is true, check it by vertex key
// 2. if use_vertex_key is false, check it by scanning vertex prefix
// If vertex does not exists, do not emplace the row.
if (!std::any_of(tagNodes_.begin(), tagNodes_.end(), [](const auto& tagNode) {
return tagNode->valid();
})) {
Expand All @@ -58,7 +60,16 @@ class GetTagPropNode : public QueryNode<VertexID> {
return ret;
}
} else {
return nebula::cpp2::ErrorCode::SUCCEEDED;
// check if vId has any valid tag by prefix scan
std::unique_ptr<kvstore::KVIterator> iter;
auto tagPrefix = NebulaKeyUtils::tagPrefix(context_->vIdLen(), partId, vId);
ret = context_->env()->kvstore_->prefix(context_->spaceId(), partId, tagPrefix, &iter);
if (ret != nebula::cpp2::ErrorCode::SUCCEEDED) {
return ret;
} else if (!iter->valid()) {
return nebula::cpp2::ErrorCode::SUCCEEDED;
}
// if has any tag, will emplace a row with vId
}
}

Expand Down
3 changes: 3 additions & 0 deletions src/storage/exec/UpdateNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,9 @@ class UpdateTagNode : public UpdateNode<VertexID> {
}
}
// step 3, insert new vertex data
if (FLAGS_use_vertex_key) {
batchHolder->put(NebulaKeyUtils::vertexKey(context_->vIdLen(), partId, vId), "");
}
batchHolder->put(std::move(key_), std::move(nVal));
return encodeBatchValue(batchHolder->getBatch());
}
Expand Down
7 changes: 3 additions & 4 deletions src/storage/mutate/AddVerticesProcessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ void AddVerticesProcessor::process(const cpp2::AddVerticesRequest& req) {
void AddVerticesProcessor::doProcess(const cpp2::AddVerticesRequest& req) {
const auto& partVertices = req.get_parts();
const auto& propNamesMap = req.get_prop_names();
bool onlyVertex = propNamesMap.empty();
for (auto& part : partVertices) {
auto partId = part.first;
const auto& vertices = part.second;
Expand All @@ -82,7 +81,7 @@ void AddVerticesProcessor::doProcess(const cpp2::AddVerticesRequest& req) {
code = nebula::cpp2::ErrorCode::E_INVALID_VID;
break;
}
if (onlyVertex && FLAGS_use_vertex_key) {
if (FLAGS_use_vertex_key) {
data.emplace_back(NebulaKeyUtils::vertexKey(spaceVidLen_, partId, vid), "");
}
for (auto& newTag : newTags) {
Expand Down Expand Up @@ -147,7 +146,6 @@ void AddVerticesProcessor::doProcess(const cpp2::AddVerticesRequest& req) {
void AddVerticesProcessor::doProcessWithIndex(const cpp2::AddVerticesRequest& req) {
const auto& partVertices = req.get_parts();
const auto& propNamesMap = req.get_prop_names();
bool onlyVertex = propNamesMap.empty();

for (const auto& part : partVertices) {
auto partId = part.first;
Expand All @@ -170,7 +168,8 @@ void AddVerticesProcessor::doProcessWithIndex(const cpp2::AddVerticesRequest& re
code = nebula::cpp2::ErrorCode::E_INVALID_VID;
break;
}
if (onlyVertex && FLAGS_use_vertex_key) {

if (FLAGS_use_vertex_key) {
verticeData.emplace_back(NebulaKeyUtils::vertexKey(spaceVidLen_, partId, vid));
}
for (const auto& newTag : newTags) {
Expand Down
3 changes: 3 additions & 0 deletions tests/tck/features/delete/DeleteTag.IntVid.feature
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ Feature: Delete int vid of tag
"""
Then the result should be, in any order:
| player.name | player.age |
| EMPTY | EMPTY |
When executing query:
"""
FETCH PROP ON bachelor hash("Tim Duncan") YIELD bachelor.name, bachelor.speciality
Expand Down Expand Up @@ -160,6 +161,7 @@ Feature: Delete int vid of tag
| id |
Then drop the used space

@wtf
Scenario: delete int vid multiple vertex one tag
Given an empty graph
And load "nba_int_vid" csv data to a new space
Expand Down Expand Up @@ -205,6 +207,7 @@ Feature: Delete int vid of tag
"""
Then the result should be, in any order:
| player.name | player.age |
| EMPTY | EMPTY |
When executing query:
"""
FETCH PROP ON player hash("Tony Parker") YIELD player.name, player.age
Expand Down
2 changes: 2 additions & 0 deletions tests/tck/features/delete/DeleteTag.feature
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ Feature: Delete string vid of tag
"""
Then the result should be, in any order:
| player.name | player.age |
| EMPTY | EMPTY |
When executing query:
"""
FETCH PROP ON bachelor "Tim Duncan" YIELD bachelor.name, bachelor.speciality
Expand Down Expand Up @@ -205,6 +206,7 @@ Feature: Delete string vid of tag
"""
Then the result should be, in any order:
| player.name | player.age |
| EMPTY | EMPTY |
When executing query:
"""
FETCH PROP ON player "Tony Parker" YIELD player.name, player.age
Expand Down
24 changes: 12 additions & 12 deletions tests/tck/features/go/GO.IntVid.feature
Original file line number Diff line number Diff line change
Expand Up @@ -1383,14 +1383,14 @@ Feature: IntegerVid Go Sentence
"""
Then the result should be, in any order, with relax comparison, and the columns 0,1 should be hashed:
| serve._dst | like._dst | serve.start_year | like.likeness | $$.player.name |
| "Thunders" | EMPTY | 2008 | EMPTY | NULL |
| "Thunders" | EMPTY | 2008 | EMPTY | EMPTY |
| EMPTY | "Paul George" | EMPTY | 90 | "Paul George" |
| EMPTY | "James Harden" | EMPTY | 90 | "James Harden" |
| "Pacers" | EMPTY | 2010 | EMPTY | NULL |
| "Thunders" | EMPTY | 2017 | EMPTY | NULL |
| "Pacers" | EMPTY | 2010 | EMPTY | EMPTY |
| "Thunders" | EMPTY | 2017 | EMPTY | EMPTY |
| EMPTY | "Russell Westbrook" | EMPTY | 95 | "Russell Westbrook" |
| "Thunders" | EMPTY | 2009 | EMPTY | NULL |
| "Rockets" | EMPTY | 2012 | EMPTY | NULL |
| "Thunders" | EMPTY | 2009 | EMPTY | EMPTY |
| "Rockets" | EMPTY | 2012 | EMPTY | EMPTY |
| EMPTY | "Russell Westbrook" | EMPTY | 80 | "Russell Westbrook" |
When executing query:
"""
Expand All @@ -1399,14 +1399,14 @@ Feature: IntegerVid Go Sentence
"""
Then the result should be, in any order, with relax comparison, and the columns 0,1 should be hashed:
| serve._dst | like._dst | serve.start_year | like.likeness | $$.player.name |
| "Thunders" | EMPTY | 2008 | EMPTY | NULL |
| "Thunders" | EMPTY | 2008 | EMPTY | EMPTY |
| EMPTY | "Paul George" | EMPTY | 90 | "Paul George" |
| EMPTY | "James Harden" | EMPTY | 90 | "James Harden" |
| "Pacers" | EMPTY | 2010 | EMPTY | NULL |
| "Thunders" | EMPTY | 2017 | EMPTY | NULL |
| "Pacers" | EMPTY | 2010 | EMPTY | EMPTY |
| "Thunders" | EMPTY | 2017 | EMPTY | EMPTY |
| EMPTY | "Russell Westbrook" | EMPTY | 95 | "Russell Westbrook" |
| "Thunders" | EMPTY | 2009 | EMPTY | NULL |
| "Rockets" | EMPTY | 2012 | EMPTY | NULL |
| "Thunders" | EMPTY | 2009 | EMPTY | EMPTY |
| "Rockets" | EMPTY | 2012 | EMPTY | EMPTY |
| EMPTY | "Russell Westbrook" | EMPTY | 80 | "Russell Westbrook" |
When executing query:
"""
Expand Down Expand Up @@ -1480,8 +1480,8 @@ Feature: IntegerVid Go Sentence
GO FROM hash('Tim Duncan') OVER serve YIELD $$.player.name as name
"""
Then the result should be, in any order, with relax comparison:
| name |
| NULL |
| name |
| EMPTY |

Scenario: Integer Vid zero step
When executing query:
Expand Down
28 changes: 14 additions & 14 deletions tests/tck/features/go/GO.feature
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ Feature: Go Sentence
When executing query:
"""
GO FROM "Paul Gasol" OVER *
WHERE $$.player.name IS NOT NULL
WHERE $$.player.name IS NOT EMPTY
YIELD like._dst
"""
Then the result should be, in any order, with relax comparison:
Expand All @@ -356,7 +356,7 @@ Feature: Go Sentence
When executing query:
"""
GO FROM "Paul Gasol" OVER *
WHERE $$.player.name IS NULL
WHERE $$.player.name IS EMPTY
YIELD like._dst
"""
Then the result should be, in any order, with relax comparison:
Expand Down Expand Up @@ -1474,14 +1474,14 @@ Feature: Go Sentence
"""
Then the result should be, in any order, with relax comparison:
| serve._dst | like._dst | serve.start_year | like.likeness | $$.player.name |
| "Thunders" | EMPTY | 2008 | EMPTY | NULL |
| "Thunders" | EMPTY | 2008 | EMPTY | EMPTY |
| EMPTY | "Paul George" | EMPTY | 90 | "Paul George" |
| EMPTY | "James Harden" | EMPTY | 90 | "James Harden" |
| "Pacers" | EMPTY | 2010 | EMPTY | NULL |
| "Thunders" | EMPTY | 2017 | EMPTY | NULL |
| "Pacers" | EMPTY | 2010 | EMPTY | EMPTY |
| "Thunders" | EMPTY | 2017 | EMPTY | EMPTY |
| EMPTY | "Russell Westbrook" | EMPTY | 95 | "Russell Westbrook" |
| "Thunders" | EMPTY | 2009 | EMPTY | NULL |
| "Rockets" | EMPTY | 2012 | EMPTY | NULL |
| "Thunders" | EMPTY | 2009 | EMPTY | EMPTY |
| "Rockets" | EMPTY | 2012 | EMPTY | EMPTY |
| EMPTY | "Russell Westbrook" | EMPTY | 80 | "Russell Westbrook" |
When executing query:
"""
Expand All @@ -1490,14 +1490,14 @@ Feature: Go Sentence
"""
Then the result should be, in any order, with relax comparison:
| serve._dst | like._dst | serve.start_year | like.likeness | $$.player.name |
| "Thunders" | EMPTY | 2008 | EMPTY | NULL |
| "Thunders" | EMPTY | 2008 | EMPTY | EMPTY |
| EMPTY | "Paul George" | EMPTY | 90 | "Paul George" |
| EMPTY | "James Harden" | EMPTY | 90 | "James Harden" |
| "Pacers" | EMPTY | 2010 | EMPTY | NULL |
| "Thunders" | EMPTY | 2017 | EMPTY | NULL |
| "Pacers" | EMPTY | 2010 | EMPTY | EMPTY |
| "Thunders" | EMPTY | 2017 | EMPTY | EMPTY |
| EMPTY | "Russell Westbrook" | EMPTY | 95 | "Russell Westbrook" |
| "Thunders" | EMPTY | 2009 | EMPTY | NULL |
| "Rockets" | EMPTY | 2012 | EMPTY | NULL |
| "Thunders" | EMPTY | 2009 | EMPTY | EMPTY |
| "Rockets" | EMPTY | 2012 | EMPTY | EMPTY |
| EMPTY | "Russell Westbrook" | EMPTY | 80 | "Russell Westbrook" |
When executing query:
"""
Expand Down Expand Up @@ -1571,8 +1571,8 @@ Feature: Go Sentence
GO FROM 'Tim Duncan' OVER serve YIELD $$.player.name as name
"""
Then the result should be, in any order, with relax comparison:
| name |
| NULL |
| name |
| EMPTY |

Scenario: zero step
When executing query:
Expand Down
14 changes: 7 additions & 7 deletions tests/tck/features/go/GoYieldVertexEdge.feature
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ Feature: Go Yield Vertex And Edge Sentence
When executing query:
"""
GO FROM "Paul Gasol" OVER *
WHERE $$.player.name IS NOT NULL
WHERE $$.player.name IS NOT EMPTY
YIELD edge as e
"""
Then the result should be, in any order, with relax comparison:
Expand All @@ -355,7 +355,7 @@ Feature: Go Yield Vertex And Edge Sentence
When executing query:
"""
GO FROM "Paul Gasol" OVER *
WHERE $$.player.name IS NULL
WHERE $$.player.name IS EMPTY
YIELD type(edge) as type
"""
Then the result should be, in any order, with relax comparison:
Expand Down Expand Up @@ -1383,13 +1383,13 @@ Feature: Go Yield Vertex And Edge Sentence
| dst | serve.start_year | like.likeness | $$.player.name |
| "James Harden" | EMPTY | 90 | "James Harden" |
| "Paul George" | EMPTY | 90 | "Paul George" |
| "Thunders" | 2008 | EMPTY | NULL |
| "Thunders" | 2008 | EMPTY | EMPTY |
| "Russell Westbrook" | EMPTY | 80 | "Russell Westbrook" |
| "Rockets" | 2012 | EMPTY | NULL |
| "Thunders" | 2009 | EMPTY | NULL |
| "Rockets" | 2012 | EMPTY | EMPTY |
| "Thunders" | 2009 | EMPTY | EMPTY |
| "Russell Westbrook" | EMPTY | 95 | "Russell Westbrook" |
| "Pacers" | 2010 | EMPTY | NULL |
| "Thunders" | 2017 | EMPTY | NULL |
| "Pacers" | 2010 | EMPTY | EMPTY |
| "Thunders" | 2017 | EMPTY | EMPTY |
When executing query:
"""
GO 1 TO 2 STEPS FROM 'Russell Westbrook' OVER * REVERSELY YIELD edge as e
Expand Down
4 changes: 0 additions & 4 deletions tests/tck/features/insert/insertVertexOnly.feature
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,6 @@ Feature: insert vertex without tag
When executing query and retrying it on failure every 6 seconds for 3 times:
"""
INSERT VERTEX VALUES 1:(),2:(),3:();
"""
Then a SemanticError should be raised at runtime: Insert vertex is forbidden, please speicify the tag
When executing query:
"""
INSERT EDGE e() VALUES 1->2:(),2->3:();
"""
Then the execution should be successful
Expand Down
2 changes: 2 additions & 0 deletions tests/tck/features/match/MatchById.IntVid.feature
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ Feature: Integer Vid Match By Id
"""
Then the result should be, in any order, with relax comparison:
| Type | Name |
| 'like' | NULL |
| 'serve' | 'Cavaliers' |
| 'serve' | 'Heat' |
| 'serve' | 'Cavaliers' |
Expand All @@ -113,6 +114,7 @@ Feature: Integer Vid Match By Id
"""
Then the result should be, in any order, with relax comparison:
| Type | Name |
| 'like' | NULL |
| 'serve' | 'Cavaliers' |
| 'serve' | 'Heat' |
| 'serve' | 'Cavaliers' |
Expand Down
Loading

0 comments on commit eba755c

Please sign in to comment.