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

Insert vertex only #3335

Merged
merged 17 commits into from
Dec 27, 2021
Merged

Insert vertex only #3335

merged 17 commits into from
Dec 27, 2021

Conversation

cangfengzhs
Copy link
Contributor

@cangfengzhs cangfengzhs commented Nov 19, 2021

What type of PR is this?

  • bug
  • feature
  • enhancement

What does this PR do?

add insert vertex only statement
For example:

Insert vertex (1,2,3);

About parentheses in statement.at the beginning, I wanted to implement it in the following format, but there was ambiguity:

Insert vertex t();  // stmt1
Insert vertex t() Values 1:(); // stmt2

In stmt1, t() represents a function, but in stmt2, t() represents a tag

Which issue(s)/PR(s) this PR relates to?

part of issue: #3123
need PR: #3328

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

Additional context:

Checklist:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatible (If it is incompatible, please describe it and add corresponding label.)
  • Need to cherry-pick (If need to cherry-pick to some branches, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to reflect in release notes and how to describe:

                                                            `

@Shylock-Hg
Copy link
Contributor

How about insert vertex values 1:(), 2:() ...?

@cangfengzhs
Copy link
Contributor Author

How about insert vertex values 1:(), 2:() ...?

This problem has been initially solved. insert vertex ifNotExists? VALUES vid_list syntax is used.

add insert vertex only validator

fix grammar

adjust insert vertex parser
@cangfengzhs cangfengzhs added the ready-for-testing PR: ready for the CI test label Dec 15, 2021
@@ -38,6 +38,20 @@ class InsertVerticesValidator final : public Validator {
std::vector<storage::cpp2::NewVertex> vertices_;
};

class InsertVerticesOnlyValidator final : public Validator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't reuse InsertVerticesValidator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The syntax of InsertVerticesOnly and the syntax of InsertVertices are quite different in the parser process. This causes the data structure contained in the two Sentences to be different. If both Sentences use InsertVerticesValidator, two completely different toPlan logics are also required. So simply use a new InsertVerticesOnlyValidator. This also ensures that the impact on InsertVertices is minimal, and other bugs will not be introduced during the modification process.

Copy link
Contributor

@Shylock-Hg Shylock-Hg Dec 15, 2021

Choose a reason for hiding this comment

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

vertex_row_item

I think you just need add a new reduce rule like vid { $$ = new VertexRowIten($1, new ValueList())} in vertex_row_item rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But, InsertVertices can not use vertexID list as "1,2,3".It needs "1:(),2:(),3:()" instead. So if add a new reduce rule in vertex_row_item, the parse process of insertVertices will be very strange.

@@ -52,6 +52,7 @@ class Sentence {
kDropTag,
kDropEdge,
kInsertVertices,
kInsertVerticesOnly,
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@cangfengzhs cangfengzhs marked this pull request as ready for review December 15, 2021 06:58
@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2021

Codecov Report

Merging #3335 (4750d17) into master (53b85dc) will increase coverage by 0.02%.
The diff coverage is 76.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3335      +/-   ##
==========================================
+ Coverage   85.19%   85.22%   +0.02%     
==========================================
  Files        1306     1307       +1     
  Lines      122158   122239      +81     
==========================================
+ Hits       104078   104182     +104     
+ Misses      18080    18057      -23     
Impacted Files Coverage Δ
src/common/expression/VariableExpression.cpp 84.84% <ø> (-0.45%) ⬇️
...rc/graph/executor/query/AppendVerticesExecutor.cpp 98.30% <ø> (ø)
src/graph/executor/query/GetEdgesExecutor.cpp 94.00% <ø> (ø)
src/graph/executor/query/GetVerticesExecutor.cpp 96.77% <ø> (ø)
src/graph/executor/query/IndexScanExecutor.cpp 88.88% <ø> (ø)
src/graph/optimizer/rule/IndexScanRule.h 100.00% <ø> (ø)
...mizer/rule/PushLimitDownEdgeIndexRangeScanRule.cpp 20.00% <0.00%> (-0.69%) ⬇️
...raph/optimizer/rule/PushLimitDownIndexScanRule.cpp 14.81% <0.00%> (-0.57%) ⬇️
...imizer/rule/PushLimitDownTagIndexRangeScanRule.cpp 20.00% <0.00%> (-0.69%) ⬇️
src/graph/planner/plan/Query.h 97.22% <ø> (ø)
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ea4889...4750d17. Read the comment docs.

@Sophie-Xie Sophie-Xie requested review from wenhaocs and removed request for a team December 20, 2021 04:01
INSERT EDGE e() VALUES 1->2:(),2->3:();
"""
Then the execution should be successful
When executing query:
Copy link
Contributor

Choose a reason for hiding this comment

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

Add test case like fetch prop on * 1

Copy link
Contributor

@wenhaocs wenhaocs left a comment

Choose a reason for hiding this comment

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

In previous conversations, there were more files. Just want to make sure this PR contains the files you want. Otherwise, good to go after fixing shylock's comment.

@Shylock-Hg Shylock-Hg added the doc affected PR: improvements or additions to documentation label Dec 21, 2021
@cangfengzhs
Copy link
Contributor Author

In previous conversations, there were more files. Just want to make sure this PR contains the files you want. Otherwise, good to go after fixing shylock's comment.

I adjust the implement according to shylock's suggestion and use another way which minimize the changes of previous code.

@@ -218,11 +218,12 @@ StorageRpcRespFuture<cpp2::GetPropResponse> StorageClient::getProps(
req.set_space_id(param.space);
req.set_parts(std::move(c.second));
req.set_dedup(dedup);
if (vertexProps != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor

Choose a reason for hiding this comment

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

wait fix in #3443

Copy link
Contributor

Choose a reason for hiding this comment

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

The same. Fixing something else?

Copy link
Contributor

@Shylock-Hg Shylock-Hg Dec 23, 2021

Choose a reason for hiding this comment

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

@cangfengzhs Please check why vertexProp equals to null, maybe not fixed in #3443

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FetchVerticesPlanner::buildVertexProps return a nullptr when props is empty

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc affected PR: improvements or additions to documentation ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants