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

Issue#192 Support multiple edge types in GO statement #699

Merged
merged 21 commits into from Sep 10, 2019

Conversation

@monadbobo
Copy link
Contributor

commented Jul 29, 2019

Summary:
Implemented over multiple edge types(include over "*" ) for Support all edge type in Go statement.

  1. Modification of the storage interface:
    1.1 Modify the GetNeighborsRequest structure, pass a list of edge_type to the storage layer.
    1.2 And add an over_all_edges tag to mark "over *".
    Modify the PropDef structure to represent both tag_id and edge_type
    1.3 Added the EdgeData structure to represent the edge data returned to the client.
  2. When "over *", the current implementation was first getting all the types of edges from the meta, and then call the storage interface.
  3. When the yield statement does not exist, delete the Deleted the default rename("dst_" to "id").
  4. Delete the getOutBound and getInBound interfaces,
    because the edges passed to the storage may have both in and out.
  5. Added a map of edge type to edge name (toEdgeName) and a map of spaceid to all edgetype for metaclient.
  6. Returns the default value of the corresponding type when some properties do not exist.
Issue#192 Support multiple edge types in GO statement
Summary:
Implemented over multiple edge types(include over "*" ) for Support all edge type in Go statement.

1. Modification of the storage interface:
   1.1 Modify the GetNeighborsRequest structure, pass a list of edge_type to the storage layer.
   1.2 And add an over_all_edges tag to mark "over *".
       Modify the PropDef structure to represent both tag_id and edge_type
   1.3 Added the EdgeData structure to represent the edge data returned to the client.
2. When the yield statement does not exist, rename the "_dst" attribute of edge to edge_name+"_id"
3. Delete the getOutBound and getInBound interfaces,
   because the edges passed to the storage may have both in and out.
4  Added a map of edge type to edge name for metaclien (toEdgeName)

PS:
There are still two issues that need to be implemented.
1 Since we are not implementing a null type, we cannot represent some non-existing attributes.
2 Due to the existence of 1, there is currently no judgment on the attributes of the edges and the vetex.
So when returning multiple types of vetex/edge,
the same result will be returned if the attribute names are the same("MULTI_EDGES" test in GoTest.cpp).
@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Unit testing passed.

src/graph/GoExecutor.cpp Outdated Show resolved Hide resolved
src/parser/parser.yy Outdated Show resolved Hide resolved
1. add more test.
2. Solved the problem of display when some properties are not present(return the default value to the user).

for example:
type             default
int              0
bool             false
double/float     0.0
@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Unit testing passed.

@monadbobo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

close issue #341 #192 #580 #756

src/interface/storage.thrift Show resolved Hide resolved
src/graph/test/GoTest.cpp Outdated Show resolved Hide resolved
@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Unit testing passed.

@CPWstatic CPWstatic referenced this pull request Aug 12, 2019

@jude-zhu jude-zhu requested review from CPWstatic and wadeliuyi Aug 12, 2019

@laura-ding
Copy link
Contributor

left a comment

Well done

src/common/filter/Expressions.h Show resolved Hide resolved
src/parser/parser.yy Outdated Show resolved Hide resolved
src/parser/parser.yy Outdated Show resolved Hide resolved
src/storage/client/StorageClient.cpp Outdated Show resolved Hide resolved

@jude-zhu jude-zhu requested a review from laura-ding Aug 13, 2019

src/parser/parser.yy Outdated Show resolved Hide resolved
@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

Unit testing failed.

1 similar comment
@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

Unit testing failed.

@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

Unit testing failed.

src/graph/GoExecutor.cpp Outdated Show resolved Hide resolved
src/graph/GoExecutor.cpp Outdated Show resolved Hide resolved
LOG(ERROR) << msg;
return Status::Error(msg);
}
auto res = RowReader::getPropByIndex(vreader.get(), index);

This comment has been minimized.

Copy link
@dangleptr

dangleptr Sep 4, 2019

Contributor

I notice you change the logic from getPropByIndex to getPropByName, the performance will be hurt. Are you OK with it @dutor

src/graph/GoExecutor.cpp Outdated Show resolved Hide resolved
@laura-ding

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

jenkins go

@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

Unit testing failed.

@laura-ding

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

jenkins go

@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Unit testing failed.

@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Unit testing passed.

@dangleptr
Copy link
Contributor

left a comment

Well done. The code looks better now.
@dutor Please recheck the code in GoExecutor. If no other concerns, i will merge it.

@CPWstatic
Copy link
Contributor

left a comment

LGTM.

@dutor

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Generally, this looks good to me, thanks for the big effort.

As @dangleptr said, there truly are some changes that have performance impact. But I think we could improve it later on.

@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Unit testing failed.

@vesoft-inc vesoft-inc deleted a comment from CPWstatic Sep 9, 2019

@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Unit testing failed.

@dangleptr

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

@monadbobo Please fix the conflicts, let's go on.

@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Unit testing failed.

@monadbobo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

Jenkins go!

@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Unit testing failed.

@nebula-community-bot

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Unit testing passed.

@dutor
dutor approved these changes Sep 10, 2019
Copy link
Member

left a comment

naughty

@dangleptr
Copy link
Contributor

left a comment

Well done.

@dangleptr dangleptr merged commit c8bd7cf into vesoft-inc:master Sep 10, 2019

1 check passed

UnitTest All tests passed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.