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 verteKey #3328

Merged
merged 8 commits into from
Dec 8, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/common/utils/NebulaKeyUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,20 @@ std::string NebulaKeyUtils::edgeKey(size_t vIdLen,
.append(1, ev);
return key;
}

// static
std::string NebulaKeyUtils::vertexKey(size_t vIdLen,
PartitionID partId,
const VertexID& vId,
char pad) {
CHECK_GE(vIdLen, vId.size());
int32_t item = (partId << kPartitionOffset) | static_cast<uint32_t>(NebulaKeyType::kVertex);
std::string key;
key.reserve(kTagLen + vIdLen);
key.append(reinterpret_cast<const char*>(&item), sizeof(int32_t))
.append(vId.data(), vId.size())
.append(vIdLen - vId.size(), pad);
return key;
}
// static
std::string NebulaKeyUtils::systemCommitKey(PartitionID partId) {
int32_t item = (partId << kPartitionOffset) | static_cast<uint32_t>(NebulaKeyType::kSystem);
Expand Down
9 changes: 6 additions & 3 deletions src/common/utils/NebulaKeyUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class NebulaKeyUtils final {
static std::string lastKey(const std::string& prefix, size_t count);

/**
* Generate vertex key for kv store
* Generate tag key for kv store
* */
static std::string tagKey(
size_t vIdLen, PartitionID partId, const VertexID& vId, TagID tagId, char pad = '\0');
Expand All @@ -65,15 +65,18 @@ class NebulaKeyUtils final {
EdgeRanking rank,
const VertexID& dstId,
EdgeVerPlaceHolder ev = 1);

static std::string vertexKey(size_t vIdLen,
PartitionID partId,
const VertexID& vId,
char pad = '\0');
static std::string systemCommitKey(PartitionID partId);

static std::string systemPartKey(PartitionID partId);

static std::string kvKey(PartitionID partId, const folly::StringPiece& name);

/**
* Prefix for vertex
* Prefix for tag
* */
static std::string tagPrefix(size_t vIdLen, PartitionID partId, const VertexID& vId, TagID tagId);

Expand Down
6 changes: 3 additions & 3 deletions src/common/utils/Types.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ enum class NebulaKeyType : uint32_t {
kSystem = 0x00000004,
kOperation = 0x00000005,
kKeyValue = 0x00000006,
// kVertex = 0x00000007,
kVertex = 0x00000007,
Copy link
Contributor

Choose a reason for hiding this comment

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

Will storing tag/vertex in separate places decrease performance of getting properties 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.

There will be a decline, but it is negligible for the following reasons:

  1. Only when the tag to be read does not exist, will the vertex be judged based on the vertexKey. The probability of this situation is very low.
  2. Even if the vertexKey is read, it is only a get() request, and the overhead is very small.
  3. Someone previously proposed a solution to put vertexKey and tagKey together, and use the method of prefix for query. But the performance of rocksdb's seek() and iterate() is very poor, which may be dozens of times that of get(). This approach will bring performance degradation instead. Of course this is just a theoretical analysis, I will do a simple experiment if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

};

enum class NebulaSystemKeyType : uint32_t {
Expand All @@ -41,10 +41,10 @@ static typename std::enable_if<std::is_integral<T>::value, T>::type readInt(cons
return *reinterpret_cast<const T*>(data);
}

// size of vertex key except vertexId
// size of tag key except vertexId
static constexpr int32_t kTagLen = sizeof(PartitionID) + sizeof(TagID);

// size of vertex key except srcId and dstId
// size of tag key except srcId and dstId
static constexpr int32_t kEdgeLen =
sizeof(PartitionID) + sizeof(EdgeType) + sizeof(EdgeRanking) + sizeof(EdgeVerPlaceHolder);

Expand Down
8 changes: 4 additions & 4 deletions src/storage/CompactionFilter.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class StorageCompactionFilter final : public kvstore::KVFilter {
}

if (NebulaKeyUtils::isTag(vIdLen_, key)) {
return !vertexValid(spaceId, key, val);
return !tagValid(spaceId, key, val);
} else if (NebulaKeyUtils::isEdge(vIdLen_, key)) {
return !edgeValid(spaceId, key, val);
} else if (IndexKeyUtils::isIndexKey(key)) {
Expand All @@ -53,9 +53,9 @@ class StorageCompactionFilter final : public kvstore::KVFilter {
}

private:
bool vertexValid(GraphSpaceID spaceId,
const folly::StringPiece& key,
const folly::StringPiece& val) const {
bool tagValid(GraphSpaceID spaceId,
const folly::StringPiece& key,
const folly::StringPiece& val) const {
auto tagId = NebulaKeyUtils::getTagId(vIdLen_, key);
auto schema = schemaMan_->getTagSchema(spaceId, tagId);
if (!schema) {
Expand Down
12 changes: 10 additions & 2 deletions src/storage/exec/GetPropNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,19 @@ class GetTagPropNode : public QueryNode<VertexID> {
return ret;
}

// if none of the tag node valid, do not emplace the row
// if none of the tag node and vertex valid, do not emplace the row
if (!std::any_of(tagNodes_.begin(), tagNodes_.end(), [](const auto& tagNode) {
return tagNode->valid();
})) {
return nebula::cpp2::ErrorCode::SUCCEEDED;
auto kvstore = context_->env()->kvstore_;
auto vertexKey = NebulaKeyUtils::vertexKey(context_->vIdLen(), partId, vId);
std::string value;
ret = kvstore->get(context_->spaceId(), partId, vertexKey, &value);
if (ret == nebula::cpp2::ErrorCode::E_KEY_NOT_FOUND) {
return nebula::cpp2::ErrorCode::SUCCEEDED;
} else if (ret != nebula::cpp2::ErrorCode::SUCCEEDED) {
return ret;
}
}

List row;
Expand Down
1 change: 1 addition & 0 deletions src/storage/exec/UpdateNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ class UpdateTagNode : public UpdateNode<VertexID> {
}
}
// step 3, insert new vertex data
batchHolder->put(NebulaKeyUtils::vertexKey(context_->vIdLen(), partId, vId), "");
batchHolder->put(std::move(key_), std::move(nVal));
return encodeBatchValue(batchHolder->getBatch());
}
Expand Down
4 changes: 2 additions & 2 deletions src/storage/mutate/AddVerticesProcessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ void AddVerticesProcessor::doProcess(const cpp2::AddVerticesRequest& req) {
code = nebula::cpp2::ErrorCode::E_INVALID_VID;
break;
}

data.emplace_back(NebulaKeyUtils::vertexKey(spaceVidLen_, partId, vid), "");
for (auto& newTag : newTags) {
auto tagId = newTag.get_tag_id();
VLOG(3) << "PartitionID: " << partId << ", VertexID: " << vid << ", TagID: " << tagId;
Expand Down Expand Up @@ -155,7 +155,7 @@ void AddVerticesProcessor::doProcessWithIndex(const cpp2::AddVerticesRequest& re
code = nebula::cpp2::ErrorCode::E_INVALID_VID;
break;
}

batchHolder->put(NebulaKeyUtils::vertexKey(spaceVidLen_, partId, vid), "");
for (auto& newTag : newTags) {
auto tagId = newTag.get_tag_id();
auto l = std::make_tuple(spaceId_, partId, tagId, vid);
Expand Down
3 changes: 2 additions & 1 deletion src/storage/mutate/DeleteVerticesProcessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ void DeleteVerticesProcessor::process(const cpp2::DeleteVerticesRequest& req) {
code = nebula::cpp2::ErrorCode::E_INVALID_VID;
break;
}

keys.emplace_back(NebulaKeyUtils::vertexKey(spaceVidLen_, partId, vid.getStr()));
auto prefix = NebulaKeyUtils::tagPrefix(spaceVidLen_, partId, vid.getStr());
std::unique_ptr<kvstore::KVIterator> iter;
code = env_->kvstore_->prefix(spaceId_, partId, prefix, &iter);
Expand Down Expand Up @@ -112,6 +112,7 @@ ErrorOr<nebula::cpp2::ErrorCode, std::string> DeleteVerticesProcessor::deleteVer
target.reserve(vertices.size());
std::unique_ptr<kvstore::BatchHolder> batchHolder = std::make_unique<kvstore::BatchHolder>();
for (auto& vertex : vertices) {
batchHolder->remove(NebulaKeyUtils::vertexKey(spaceVidLen_, partId, vertex.getStr()));
auto prefix = NebulaKeyUtils::tagPrefix(spaceVidLen_, partId, vertex.getStr());
std::unique_ptr<kvstore::KVIterator> iter;
auto ret = env_->kvstore_->prefix(spaceId_, partId, prefix, &iter);
Expand Down
9 changes: 9 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 |
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't return a result with all properties empty value when the vertex doesn't exist. The empty response is what we expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we add a vertexKey, Delete Tag will not delete vertex at same time. Vertex is still existed

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to support fetch tags on hash("Tim Duncan") later?
This seems like a natural user demand scenario bcz TagKey and VertexKey are abstracted out.

When executing query:
"""
FETCH PROP ON bachelor hash("Tim Duncan") YIELD bachelor.name, bachelor.speciality
Expand Down Expand Up @@ -94,12 +95,14 @@ 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
"""
Then the result should be, in any order:
| bachelor.name | bachelor.speciality |
| EMPTY | EMPTY |
When executing query:
"""
LOOKUP ON player WHERE player.name == "Tim Duncan"
Expand Down Expand Up @@ -146,12 +149,14 @@ 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
"""
Then the result should be, in any order:
| bachelor.name | bachelor.speciality |
| EMPTY | EMPTY |
When executing query:
"""
LOOKUP ON player WHERE player.name == "Tim Duncan"
Expand Down Expand Up @@ -205,12 +210,14 @@ 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
"""
Then the result should be, in any order:
| player.name | player.age |
| EMPTY | EMPTY |
When executing query:
"""
LOOKUP ON player WHERE player.name == "Tim Duncan"
Expand Down Expand Up @@ -256,6 +263,7 @@ Feature: Delete int vid of tag
"""
Then the result should be, in any order:
| team.name |
| EMPTY |
# delete tag from pipe and normal
When executing query:
"""
Expand Down Expand Up @@ -295,6 +303,7 @@ Feature: Delete int vid of tag
"""
Then the result should be, in any order:
| team.name |
| EMPTY |
# delete one tag from var and normal
When executing query:
"""
Expand Down
9 changes: 9 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 @@ -94,12 +95,14 @@ 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
"""
Then the result should be, in any order:
| bachelor.name | bachelor.speciality |
| EMPTY | EMPTY |
When executing query:
"""
LOOKUP ON player WHERE player.name == "Tim Duncan"
Expand Down Expand Up @@ -146,12 +149,14 @@ 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
"""
Then the result should be, in any order:
| bachelor.name | bachelor.speciality |
| EMPTY | EMPTY |
When executing query:
"""
LOOKUP ON player WHERE player.name == "Tim Duncan"
Expand Down Expand Up @@ -205,12 +210,14 @@ 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
"""
Then the result should be, in any order:
| player.name | player.age |
| EMPTY | EMPTY |
When executing query:
"""
LOOKUP ON player WHERE player.name == "Tim Duncan"
Expand Down Expand Up @@ -256,6 +263,7 @@ Feature: Delete string vid of tag
"""
Then the result should be, in any order:
| team.name |
| EMPTY |
# delete tag from pipe and normal
When executing query:
"""
Expand Down Expand Up @@ -295,6 +303,7 @@ Feature: Delete string vid of tag
"""
Then the result should be, in any order:
| team.name |
| EMPTY |
# delete one tag from var and normal
When executing query:
"""
Expand Down
14 changes: 10 additions & 4 deletions tests/tck/features/ttl/TTL.feature
Original file line number Diff line number Diff line change
Expand Up @@ -393,31 +393,36 @@ Feature: TTLTest
FETCH PROP ON person "1" YIELD vertex as node;
"""
Then the result should be, in any order, with relax comparison:
| node |
| node |
| ("1") |
When executing query:
"""
FETCH PROP ON person "1" YIELD person.id as id
"""
Then the result should be, in any order:
| id |
| id |
| EMPTY |
When executing query:
"""
FETCH PROP ON * "1" YIELD person.id, career.id
"""
Then the result should be, in any order:
| person.id | career.id |
| EMPTY | EMPTY |
When executing query:
"""
FETCH PROP ON person "2" YIELD person.id
"""
Then the result should be, in any order:
| person.id |
| EMPTY |
When executing query:
"""
FETCH PROP ON person "2" YIELD person.id as id
"""
Then the result should be, in any order:
| id |
| id |
| EMPTY |
When executing query:
"""
FETCH PROP ON career "2" YIELD career.id;
Expand Down Expand Up @@ -486,5 +491,6 @@ Feature: TTLTest
FETCH PROP ON person "1" YIELD person.age as age;
"""
Then the result should be, in any order:
| age |
| age |
| EMPTY |
And drop the used space