rewrite fetch prop on vertex: 1. support input/variable/multi-vertex-id/multi-tags. 2. keep order of input. #2222

xuguruogu · 2020-07-11T10:40:25Z

rewrite fetch prop on vertex.

support multi vids for all case.
support input/variable ref
support multi tags. if yield is not specified, return as many as columns as posible as long as one of the vids has value.
keep order of input.
compatible with old tests.

dangleptr

Thanks for your contribution. It is really a huge pr, i think we need some time to take care of it.
Great work! Excellent!

dangleptr · 2020-07-13T11:52:13Z

src/graph/FetchVerticesExecutor.cpp

-                    rsWriter = std::make_unique<RowSetWriter>(outputSchema);
+                TagID tagId = std::move(tagIdStatus).value();
+                if (ds.find(tagId) != ds.end()) {
+                    auto vreader = ds[tagId].get();


Use the iterator returned by 'find' directly to avoid looking up the hash map again.

dangleptr · 2020-07-13T11:58:28Z

src/meta/client/MetaClient.cpp

@@ -279,7 +282,8 @@ bool MetaClient::loadSchemas(GraphSpaceID spaceId,
                             SpaceEdgeTypeNameMap &edgeTypeNameMap,
                             SpaceNewestTagVerMap &newestTagVerMap,
                             SpaceNewestEdgeVerMap &newestEdgeVerMap,
-                             SpaceAllEdgeMap &allEdgeMap) {
+                             SpaceAllEdgeMap &allEdgeMap,


The better name are spaceAllEdges/spaceAllTags

dangleptr · 2020-07-13T11:59:38Z

src/meta/client/MetaClient.cpp

@@ -921,6 +933,20 @@ StatusOr<std::vector<std::string>> MetaClient::getAllEdgeFromCache(const GraphSp
    return it->second;
 }

+StatusOr<std::vector<std::string>> MetaClient::getAllTagFromCache(const GraphSpaceID& space) {


The better name is getAllTagsFromCache.

PS. Maybe "getAllEdgeFromCache" should be getAllEdgesFromCache

dangleptr · 2020-07-13T12:02:23Z

src/parser/parser.yy

+        labels->addLabel(new std::string("*"));
+        $$ = new FetchVerticesSentence(labels, $5, $6);
+    }
+    | KW_FETCH KW_PROP KW_ON MUL vid_ref_expression yield_clause {


If you want to support multi props, maybe we need to support "KW_PROP" and "KW_PROPS" both.

just like value/values...

dangleptr · 2020-07-13T12:06:48Z

src/parser/test/ParserTest.cpp

+    }
+    {
+        GQLParser parser;
+        std::string query = "FETCH PROP ON person, another 1, 2, 3";


I want to know what's the response looks like in this case. Person and Another maybe have different props. You join them on vid together?

we use the tag as Wide table, simply speaking: one tag hold one value. In this way, we can support large data adding and modify.

Person and Another tags join together, display default value if one of them is null.

dangleptr · 2020-07-13T12:16:59Z

src/parser/parser.yy

-    | KW_FETCH KW_PROP KW_ON name_label edge_key_ref yield_clause {
-        auto fetch = new FetchEdgesSentence($4, $5, $6);
+    | KW_FETCH KW_PROP KW_ON fetch_labels edge_key_ref yield_clause {
+        auto fetch = new FetchEdgesSentence($4->release(), $5, $6);


Why call release here?

I will remove it.

dangleptr · 2020-07-13T12:18:34Z

src/graph/FetchVerticesExecutor.cpp

+        resp_ = std::make_unique<cpp2::ExecutionResponse>();
+        auto colNames = outputs->getColNames();
+        resp_->set_column_names(std::move(colNames));
+        if (outputs->hasData()) {


If it is the rightest sentence, you could avoid the interim result setting, it will introduce extra encode/decode.
You could take toThriftResponse as your reference inside GoExecutor.

Emm... I simply copy the codes from FetchExecutor::finishExecution. Maybe more attention will needed to fully understand.

Fine. Just leave it here.

dangleptr · 2020-07-13T12:19:40Z

src/graph/FetchVerticesExecutor.cpp

+            auto schema = reader->getSchema().get();
+            Getters getters;
+            getters.getVariableProp = [&] (const std::string &prop) -> OptVariantType {
+                if (prop == "VertexID") {


If you define this behavior, please add notes about it in summary of this pr.

In the previous codes, every record will be attached collumn "VertexID" at beggining. To be compatible with this behavior. I will comment on this.

modify the codes to keep identical with previous codes.

dangleptr · 2020-07-13T12:20:04Z

src/graph/FetchVerticesExecutor.cpp

+            if (dataMap.find(vid) == dataMap.end() && !expCtx_->hasInputProp()) {
+                return Status::OK();
+            }
+            auto& ds = dataMap[vid];


You could use the iterator returned by find directly.

dangleptr · 2020-07-13T12:21:45Z

src/parser/Clauses.h

+
+    std::string toString() const;
+
+    std::string* release() {


Weird about this method. Could you add some comments about it.

CPWstatic

Impressive work!

CPWstatic · 2020-07-15T09:21:59Z

src/graph/FetchVerticesExecutor.cpp

            colname_ = vexpr->prop();
+            bool existing = false;
+            inputs_p_ = ectx()->variableHolder()->get(*varname, &existing);


inputs_p_ is not the code style of nebula repo.

CPWstatic · 2020-07-15T13:54:17Z

src/parser/parser.yy

        auto fetch = new FetchEdgesSentence($4, $5, $6);
        $$ = fetch;
    }
-    | KW_FETCH KW_PROP KW_ON name_label edge_key_ref yield_clause {
+    | KW_FETCH KW_PROP KW_ON fetch_labels edge_key_ref yield_clause {


Emm, if don't implement it in this pr, please forbid this syntax at present.

As the syntax has trouble to shift and reduce before edge_key and vids identifiers, I return the error msg in the executor.

dangleptr · 2020-07-16T02:11:48Z

src/graph/FetchVerticesExecutor.cpp

+        return Status::Error("tags shall never be empty");
+    }
+
+    if (tagNames.size() == 1 && *tagNames[0] == "*") {


Could we combine the logic about "*" and multi tags. It seems there are some duplicate codes here.

make sense.

dangleptr · 2020-07-16T02:13:09Z

src/graph/FetchVerticesExecutor.cpp

+        resp_ = std::make_unique<cpp2::ExecutionResponse>();
+        auto colNames = outputs->getColNames();
+        resp_->set_column_names(std::move(colNames));
+        if (outputs->hasData()) {


Fine. Just leave it here.

dangleptr · 2020-07-16T02:15:31Z

src/graph/test/FetchVerticesTest.cpp

+    {
+        cpp2::ExecutionResponse resp;
+        auto &player = players_["Boris Diaw"];
+        auto *fmt = "GO FROM %ld over like YIELD like._dst as id"


I want to know, this case is really in usage?

Heavily used. Mostly, we get a sub-graph, using filters or other restriction, and then fetch more information of the nodes.

Shylock-Hg · 2020-07-16T03:18:58Z

src/parser/parser.yy

        $$ = new FetchVerticesSentence($4, $5, $6);
    }
-    | KW_FETCH KW_PROP KW_ON MUL vid {
-        $$ = new FetchVerticesSentence($5);
+    | KW_FETCH KW_PROP KW_ON MUL vid_list yield_clause {


This seem be conflict, if you fetch prop on * but specify the tag.prop when yield.

we have many tags in use. it is redundant to write tag-name twice. In actually implement, if yield_clause specified, tag-names may only be used as verification.

dangleptr · 2020-07-16T04:47:21Z

src/graph/FetchVerticesExecutor.cpp

+            if (col->expr()->isInputExpression()) {
+                auto *inputExpr = dynamic_cast<InputPropertyExpression*>(col->expr());
+                auto *colName = inputExpr->prop();
+                if (*colName == "*") {


Excellent. Could we combine the logic about "*" both in InputPropertyExpression and VariablePropertyExpression into one?

may have trouble due to

Expression *expr = new InputPropertyExpression(new std::string(prop)); Expression *expr = new VariablePropertyExpression(alias, new std::string(prop));

dangleptr · 2020-07-16T05:03:19Z

src/graph/FetchVerticesExecutor.cpp

+            pd.owner = storage::cpp2::PropOwner::SOURCE;
+            pd.name = prop;
+            pd.id.set_tag_id(tagId);
+            props_.emplace_back(std::move(pd));


Where did your support allProps on one Tag. Such as tag1.*

tag1.* can not pass syntax check.

Is it compatible with the original behavior?

the behavior is same.

dangleptr

Please address the inline comments.
If you don't want to fix them in this pr, please add one TODO comment.

BTW. Please check the failed UTs.
Thanks

…id/multi-tags. 2. keep order of input.

dangleptr

Totally. the PR looks good to me.
Thanks for your contribution.

amber-moe · 2020-08-03T02:01:46Z

doc done

…id/multi-tags. 2. keep order of input. (vesoft-inc#2222) Co-authored-by: trippli <[email protected]> Co-authored-by: dangleptr <[email protected]>

xuguruogu force-pushed the fetch-prop-on-vertex branch from cfdf2b5 to 40934f6 Compare July 11, 2020 10:43

Shylock-Hg added the feature label Jul 12, 2020

dutor requested review from CPWstatic, monadbobo, Shylock-Hg and critical27 July 13, 2020 01:36

dangleptr reviewed Jul 13, 2020

View reviewed changes

xuguruogu force-pushed the fetch-prop-on-vertex branch 2 times, most recently from 2c97e70 to c35290c Compare July 15, 2020 03:08

jude-zhu requested a review from dangleptr July 15, 2020 03:50

xuguruogu force-pushed the fetch-prop-on-vertex branch from c35290c to 1516bad Compare July 15, 2020 04:57

CPWstatic reviewed Jul 15, 2020

View reviewed changes

xuguruogu force-pushed the fetch-prop-on-vertex branch 2 times, most recently from f83c4eb to ef4f664 Compare July 15, 2020 16:40

dangleptr reviewed Jul 16, 2020

View reviewed changes

Shylock-Hg reviewed Jul 16, 2020

View reviewed changes

dangleptr reviewed Jul 16, 2020

View reviewed changes

xuguruogu force-pushed the fetch-prop-on-vertex branch from ef4f664 to cc34b01 Compare July 17, 2020 14:16

dangleptr added the ready-for-testing PR: ready for the CI test label Jul 20, 2020

jude-zhu requested review from dangleptr, Shylock-Hg and CPWstatic July 20, 2020 03:15

dangleptr reviewed Jul 20, 2020

View reviewed changes

xuguruogu force-pushed the fetch-prop-on-vertex branch from cc34b01 to 44463e6 Compare July 20, 2020 04:53

rewrite fetch prop on vertex: 1. support input/variable/multi-vertex-…

e964bd9

…id/multi-tags. 2. keep order of input.

xuguruogu force-pushed the fetch-prop-on-vertex branch from 7aa223e to e964bd9 Compare July 20, 2020 05:05

Merge branch 'master' into fetch-prop-on-vertex

7b51960

jude-zhu requested a review from dangleptr July 21, 2020 08:00

dangleptr approved these changes Jul 21, 2020

View reviewed changes

CPWstatic approved these changes Jul 22, 2020

View reviewed changes

Merge branch 'master' into fetch-prop-on-vertex

83fb868

dangleptr merged commit 6cc9818 into vesoft-inc:master Jul 22, 2020

xuguruogu deleted the fetch-prop-on-vertex branch July 22, 2020 05:28

amber-moe added the doc affected PR: improvements or additions to documentation label Jul 24, 2020

amber-moe mentioned this pull request Jul 24, 2020

fetch update vesoft-inc/nebula-docs#117

Merged

Shylock-Hg mentioned this pull request Aug 31, 2020

Let fetch vertices accept multi-vertices and input as vids when not specify tag. vesoft-inc/nebula-graph#251

Merged

Shylock-Hg mentioned this pull request Sep 7, 2020

Support the yield clause when fetch vertices on *. vesoft-inc/nebula-graph#264

Merged

Shylock-Hg mentioned this pull request Sep 29, 2020

Feature/fetch vertices on multi tags vesoft-inc/nebula-graph#310

Merged

rewrite fetch prop on vertex: 1. support input/variable/multi-vertex-id/multi-tags. 2. keep order of input. #2222

rewrite fetch prop on vertex: 1. support input/variable/multi-vertex-id/multi-tags. 2. keep order of input. #2222

Conversation

xuguruogu commented Jul 11, 2020 • edited Loading

dangleptr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangleptr Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPWstatic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangleptr Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangleptr left a comment • edited Loading

Choose a reason for hiding this comment

dangleptr left a comment

Choose a reason for hiding this comment

amber-moe commented Aug 3, 2020

xuguruogu commented Jul 11, 2020 •

edited

Loading

dangleptr Jul 13, 2020 •

edited

Loading

dangleptr Jul 17, 2020 •

edited

Loading

dangleptr left a comment •

edited

Loading