Copilot commented on code in PR #783:
URL: https://github.com/apache/incubator-graphar/pull/783#discussion_r2409445324
##########
cpp/src/graphar/graph_info.cc:
##########
@@ -1293,6 +1387,22 @@ Result<std::shared_ptr<GraphInfo>> GraphInfo::AddEdge(
impl_->prefix_, impl_->version_);
}
+Result<std::shared_ptr<GraphInfo>> GraphInfo::RemoveEdge(
+ std::shared_ptr<EdgeInfo> edge_info) const {
+ if (edge_info == nullptr) {
+ return Status::Invalid("edge info is nullptr");
+ }
+ int idx=GetEdgeInfoIndex(edge_info->GetSrcType(), edge_info->GetEdgeType(),
+ edge_info->GetDstType());
+ if(idx==-1){
Review Comment:
[nitpick] Missing spaces around operators and after control flow keywords.
Should be `int idx = GetEdgeInfoIndex(...)` and `if (idx == -1)`.
```suggestion
int idx = GetEdgeInfoIndex(edge_info->GetSrcType(),
edge_info->GetEdgeType(),
edge_info->GetDstType());
if (idx == -1) {
```
##########
cpp/src/graphar/graph_info.cc:
##########
@@ -845,6 +881,28 @@ Result<std::shared_ptr<EdgeInfo>>
EdgeInfo::AddAdjacentList(
impl_->property_groups_, impl_->prefix_, impl_->version_);
}
+Result<std::shared_ptr<EdgeInfo>> EdgeInfo::RemoveAdjacentList(
+ std::shared_ptr<AdjacentList> adj_list) const {
+ if(adj_list==nullptr){
+ return Status::Invalid("adj list is nullptr");
+ }
+ int idx=-1;
+ for (size_t i=0;i<impl_->adjacent_lists_.size();i++){
+ if(impl_->adjacent_lists_[i]->GetType()==adj_list->GetType()){
+ idx=i;
+ break;
+ }
+ }
+ if(idx==-1){
+ return Status::Invalid("adj list not found");
+ }
+ return std::make_shared<EdgeInfo>(
+ impl_->src_type_, impl_->edge_type_, impl_->dst_type_,
impl_->chunk_size_,
+ impl_->src_chunk_size_, impl_->dst_chunk_size_, impl_->directed_,
+ RemoveVectorElement(impl_->adjacent_lists_, static_cast<size_t>(idx)),
+ impl_->property_groups_, impl_->prefix_, impl_->version_);
+ }
Review Comment:
[nitpick] Missing spaces around operators and after control flow keywords.
Should follow consistent formatting: `int idx = -1;`, `for (size_t i = 0; i <
impl_->adjacent_lists_.size(); i++)`, `if (impl_->adjacent_lists_[i]->GetType()
== adj_list->GetType())`, and `if (idx == -1)`.
```suggestion
if (adj_list == nullptr) {
return Status::Invalid("adj list is nullptr");
}
int idx = -1;
for (size_t i = 0; i < impl_->adjacent_lists_.size(); i++) {
if (impl_->adjacent_lists_[i]->GetType() == adj_list->GetType()) {
idx = i;
break;
}
}
if (idx == -1) {
return Status::Invalid("adj list not found");
}
return std::make_shared<EdgeInfo>(
impl_->src_type_, impl_->edge_type_, impl_->dst_type_,
impl_->chunk_size_,
impl_->src_chunk_size_, impl_->dst_chunk_size_, impl_->directed_,
RemoveVectorElement(impl_->adjacent_lists_, static_cast<size_t>(idx)),
impl_->property_groups_, impl_->prefix_, impl_->version_);
}
```
##########
cpp/src/graphar/graph_info.cc:
##########
@@ -398,6 +413,27 @@ Result<std::shared_ptr<VertexInfo>>
VertexInfo::AddPropertyGroup(
impl_->prefix_, impl_->version_);
}
+Result<std::shared_ptr<VertexInfo>> VertexInfo::RemovePropertyGroup(
+ std::shared_ptr<PropertyGroup> property_group) const {
+ if (property_group == nullptr) {
+ return Status::Invalid("property group is nullptr");
+ }
+ int idx=-1;
+ for (size_t i=0;i<impl_->property_groups_.size();i++){
+ if(*(impl_->property_groups_[i])==*property_group){
+ idx=i;
+ break;
+ }
+ }
+ if(idx==-1){
Review Comment:
[nitpick] Missing spaces around operators and after control flow keywords.
Should follow consistent formatting: `int idx = -1;`, `for (size_t i = 0; i <
impl_->property_groups_.size(); i++)`, `if (*(impl_->property_groups_[i]) ==
*property_group)`, and `if (idx == -1)`.
```suggestion
if (idx == -1) {
```
##########
cpp/test/test_info.cc:
##########
@@ -550,6 +581,23 @@ version: gar/v1
auto extend_info2 = extend_info->AddPropertyGroup(pg2);
REQUIRE(!extend_info2.status().ok());
}
+
+ SECTION("RemovePropertyGroup"){
Review Comment:
[nitpick] Missing space before opening brace. Should be
`SECTION(\"RemovePropertyGroup\") {` for consistency with other test sections.
```suggestion
SECTION("RemovePropertyGroup") {
```
##########
cpp/src/graphar/graph_info.cc:
##########
@@ -864,6 +922,28 @@ Result<std::shared_ptr<EdgeInfo>>
EdgeInfo::AddPropertyGroup(
impl_->version_);
}
+Result<std::shared_ptr<EdgeInfo>> EdgeInfo::RemovePropertyGroup(
+ std::shared_ptr<PropertyGroup> property_group) const {
+ if(property_group==nullptr){
+ return Status::Invalid("property group is nullptr");
+ }
+ int idx=-1;
+ for (size_t i=0;i<impl_->property_groups_.size();i++){
+ if(*(impl_->property_groups_[i])==*property_group){
+ idx=i;
+ }
+ }
+ if(idx==-1){
+ return Status::Invalid("property group not found");
+ }
+ return std::make_shared<EdgeInfo>(
+ impl_->src_type_, impl_->edge_type_, impl_->dst_type_,
impl_->chunk_size_,
+ impl_->src_chunk_size_, impl_->dst_chunk_size_, impl_->directed_,
+ impl_->adjacent_lists_,
+ RemoveVectorElement(impl_->property_groups_, static_cast<size_t>(idx)),
impl_->prefix_,
+ impl_->version_);
+ }
Review Comment:
[nitpick] Missing spaces around operators and after control flow keywords.
Should follow consistent formatting: `int idx = -1;`, `for (size_t i = 0; i <
impl_->property_groups_.size(); i++)`, `if (*(impl_->property_groups_[i]) ==
*property_group)`, and `if (idx == -1)`. Also missing `break;` statement after
`idx=i;` for consistency with other implementations.
```suggestion
std::shared_ptr<PropertyGroup> property_group) const {
if (property_group == nullptr) {
return Status::Invalid("property group is nullptr");
}
int idx = -1;
for (size_t i = 0; i < impl_->property_groups_.size(); i++) {
if (*(impl_->property_groups_[i]) == *property_group) {
idx = i;
break;
}
}
if (idx == -1) {
return Status::Invalid("property group not found");
}
return std::make_shared<EdgeInfo>(
impl_->src_type_, impl_->edge_type_, impl_->dst_type_,
impl_->chunk_size_,
impl_->src_chunk_size_, impl_->dst_chunk_size_, impl_->directed_,
impl_->adjacent_lists_,
RemoveVectorElement(impl_->property_groups_,
static_cast<size_t>(idx)), impl_->prefix_,
impl_->version_);
}
```
##########
cpp/test/test_info.cc:
##########
@@ -715,6 +780,28 @@ version: gar/v1
auto extend_info2 = extend_info->AddEdge(edge_info2);
REQUIRE(!extend_info2.status().ok());
}
+
+ SECTION("RemoveEdge"){
Review Comment:
[nitpick] Missing space before opening brace. Should be
`SECTION(\"RemoveEdge\") {` for consistency with other test sections.
```suggestion
SECTION("RemoveEdge") {
```
##########
cpp/src/graphar/graph_info.cc:
##########
@@ -71,6 +71,21 @@ std::vector<T> AddVectorElement(const std::vector<T>&
values, T new_element) {
return out;
}
+template <typename T>
+std::vector<T> RemoveVectorElement(const std::vector<T>& values, size_t index)
{
+ if (index >= values.size()) {
+ return values;
+ }
+ std::vector<T> out;
+ out.reserve(values.size()-1);
Review Comment:
[nitpick] Missing space around the minus operator. Should be `values.size()
- 1` for consistency with project style.
```suggestion
out.reserve(values.size() - 1);
```
##########
cpp/test/test_info.cc:
##########
@@ -693,6 +741,23 @@ version: gar/v1
REQUIRE(!extend_info2.status().ok());
}
+ SECTION("RemoveVertex") {
+ auto vertex_info2 = CreateVertexInfo("test_vertex2", 100, {pg}, {},
+ "test_vertex2/", version);
+ auto maybe_extend_info = graph_info->AddVertex(vertex_info2);
+ REQUIRE(maybe_extend_info.status().ok());
+ auto extend_info = maybe_extend_info.value();
+ auto maybe_remove_info = extend_info->RemoveVertex(vertex_info2);
+ REQUIRE(maybe_remove_info.status().ok());
+ auto remove_info = maybe_remove_info.value();
+ REQUIRE(remove_info->GetVertexInfos().size() == 1);
+ REQUIRE(remove_info->GetVertexInfoByIndex(1) == nullptr);
+ REQUIRE(remove_info->GetVertexInfo("test_vertex2") == nullptr);
+ REQUIRE(remove_info->GetVertexInfoByIndex(1) == nullptr);
+ auto remove_info2 = remove_info->RemoveVertex(vertex_info2);
+ REQUIRE(!remove_info2.status().ok());
+ }
Review Comment:
[nitpick] Inconsistent indentation. The closing brace should align with the
SECTION statement above it.
```suggestion
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
