-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 scan edge/vertex in storage #1381
Conversation
Jenkins go |
Unit testing passed. |
1 similar comment
Unit testing passed. |
Unit testing failed. |
Unit testing passed. |
Unit testing passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, scanVertex
is totally meet my requirements.
Unit testing passed. |
Unit testing passed. |
Unit testing passed. |
src/kvstore/RocksEngine.h
Outdated
|
||
~RocksRangeWithPrefixIter() = default; | ||
|
||
bool valid() const override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why override the method? It looks the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
} | ||
|
||
EdgeType edgeType = NebulaKeyUtils::getEdgeType(key); | ||
if (edgeType < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
|
||
if (returnAllColumns_) { | ||
// return all columns | ||
edgeContexts_.emplace(edgeType, std::move(propContexts)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to set edgeContexts_ and edgeSchema_ when returning all columns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edgeContexts_ and edgeSchema_ are used to store related edgeType, so we can check if we need to scan this edge
Unit testing passed. |
Unit testing passed. |
Unit testing passed. |
Unit testing passed. |
Unit testing passed. |
Unit testing passed. |
@@ -185,6 +185,12 @@ class NebulaKeyUtils final { | |||
return readInt<EdgeRanking>(rawKey.data() + offset, sizeof(EdgeRanking)); | |||
} | |||
|
|||
static int64_t getVersion(const folly::StringPiece& rawKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to use a macro definition instead of int64_t? like VertexID, EdgeRanking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use int64_t as timestamp for now, maybe we can unify them with a macro later in another PR, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I just found out that the macro has already existed.
Using SchemaVer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it later, SchemaVer is used in tag or edge schema to indicate version. Should we define a DataVer to indicate the version of tag or edge data?
std::string cursor = ""; | ||
int32_t rowCount = 0, expectRowCount = 100; | ||
checkResponse(partId, resp, cursor, rowCount, expectRowCount, 0, false, true); | ||
EXPECT_EQ(rowCount, 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For edges, when no output column attributes are specified. What is output?
Does it output the following four fields:_src, _rank, _dst, _type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only return the key, aka, src, type and dst (we don't return rank for now)
Unit testing passed. |
auto f = processor->getFuture(); | ||
processor->process(req); | ||
auto resp = std::move(f).get(); | ||
++batchCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
batchCount
is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, we have count the total rows returned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done. LGTM now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo,brilliant!
* add scan edge/vertex, rebased * refactor scan edge/vertex * address @panda-sheep's comments Co-authored-by: dangleptr <37216992+dangleptr@users.noreply.github.com>
Co-authored-by: jimingquan <mingquan.ji@vesoft.com>
Support scan edge/vertex in storage, later for spark job.
We can retrieve data of a part limited by single vertexId and time range. We could enhance it by adding complicated filter and group of vertex later.