Copilot commented on code in PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#discussion_r2617746540
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +109,43 @@ public void testGraphInfoBasics() {
illegalArgumentException.getMessage());
// test version gar/v1
Assert.assertEquals(1, graphInfo.getVersion().getVersion());
+ // basic tests for addVertex and removeVertex (more advanced ones
should include adjancency
Review Comment:
Spelling error in comment: "adjancency" should be "adjacency".
```suggestion
// basic tests for addVertex and removeVertex (more advanced ones
should include adjacency
```
##########
maven-projects/info/src/main/java/org/apache/graphar/info/GraphInfo.java:
##########
@@ -136,6 +136,45 @@ public Optional<GraphInfo> addVertexAsNew(VertexInfo
vertexInfo) {
edgeConcat2EdgeInfo));
}
+ public Optional<GraphInfo> removeVertex(VertexInfo vertexInfo) {
+
+ if (vertexInfo == null || !hasVertexInfo(vertexInfo.getType())) {
+ return Optional.empty();
+ }
+ List<EdgeInfo> newCachedEdgeInfoList =
+ edgeInfos.stream()
+ .filter(
+ currEdge ->
+
!currEdge.getSrcType().equals(vertexInfo.getType())
+ && !currEdge.getDstType()
+
.equals(vertexInfo.getType()))
+ .collect(Collectors.toList());
+
+ final List<VertexInfo> newVertexInfoList =
+ vertexInfos.stream()
+ .filter(
+ v ->
+
!v.getType().equals(vertexInfo.getType())
+ &&
!v.getPrefix().equals(vertexInfo.getPrefix()))
+ .collect(Collectors.toList());
+
+ final Map<String, VertexInfo> newVertexInfoMap =
+ vertexType2VertexInfo.entrySet().stream()
+ .filter(v ->
!v.getValue().getType().equals(vertexInfo.getType()))
+ .collect(
+ Collectors.toUnmodifiableMap(
+ Map.Entry::getKey,
Map.Entry::getValue));
+ return Optional.of(
+ new GraphInfo(
+ name,
+ newVertexInfoList,
+ newCachedEdgeInfoList,
+ baseUri,
+ version,
+ newVertexInfoMap,
+ edgeConcat2EdgeInfo));
Review Comment:
The edgeConcat2EdgeInfo map is passed unchanged when creating the new
GraphInfo, but it should be updated to remove edges that reference the removed
vertex. When a vertex is removed, all edges connected to that vertex are
filtered out from newCachedEdgeInfoList, so the edgeConcat2EdgeInfo map should
also be filtered to remove those same edges for consistency.
##########
maven-projects/info/src/main/java/org/apache/graphar/info/GraphInfo.java:
##########
@@ -162,6 +201,39 @@ public Optional<GraphInfo> addEdgeAsNew(EdgeInfo edgeInfo)
{
newEdgeConcat2EdgeInfo));
}
+ public Optional<GraphInfo> removeEdge(EdgeInfo edgeInfo) {
+ if (edgeInfo == null
+ || !hasEdgeInfo(
+ edgeInfo.getSrcType(), edgeInfo.getEdgeType(),
edgeInfo.getDstType())) {
+ return Optional.empty();
+ }
+ final List<EdgeInfo> newEdgeInfos =
+ edgeInfos.stream()
+ .filter(
+ e ->
+
!(e.getSrcType().equals(edgeInfo.getSrcType())
+ &&
e.getDstType().equals(edgeInfo.getDstType())
+ &&
e.getEdgeType().equals(edgeInfo.getEdgeType())))
+ .collect(Collectors.toList());
+
+ final Map<String, EdgeInfo> newEdgeConcat2EdgeInfo =
+ edgeConcat2EdgeInfo.entrySet().stream()
+ .filter(e -> !e.getKey().equals(edgeInfo.getConcat()))
+ .filter(e ->
!e.getValue().getConcat().equals(edgeInfo.getConcat()))
Review Comment:
The second filter on line 222 is redundant. Both filters check if the concat
matches edgeInfo.getConcat(), but one checks the map key (e.getKey()) and the
other checks the map value's concat (e.getValue().getConcat()). Since
edgeConcat2EdgeInfo maps concat strings to EdgeInfo objects, and each
EdgeInfo's getConcat() should match its key in the map, these filters are
logically equivalent. Only one filter is needed.
```suggestion
```
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +109,43 @@ public void testGraphInfoBasics() {
illegalArgumentException.getMessage());
// test version gar/v1
Assert.assertEquals(1, graphInfo.getVersion().getVersion());
+ // basic tests for addVertex and removeVertex (more advanced ones
should include adjancency
+ // list checks)
+ VertexInfo testingVertexInfo =
+ new VertexInfo("", 100, Arrays.asList(TestUtil.pg1),
"vertex/person/", "gar/v1");
+ GraphInfo testingVertexGraphInfo =
+ new GraphInfo("graphVertexTest", new ArrayList<>(), new
ArrayList<>(), "", "");
+ // add the created vertex on an empty graph
+ Assert.assertEquals(
+ 1,
+ testingVertexGraphInfo
+ .addVertexAsNew(testingVertexInfo)
+ .get()
+ .getVertexInfos()
+ .size());
+ // remove the newly created vertex and check again the emptied graph
+ Assert.assertEquals(true,
testingVertexGraphInfo.removeVertex(testingVertexInfo).isEmpty());
+ // same tests as vertices for edges
+ GraphInfo testingEdgeGraphInfo =
+ new GraphInfo("graphEdgeTest", new ArrayList<>(), new
ArrayList<>(), "", "");
+ EdgeInfo testingEdgeInfo =
+ new EdgeInfo(
+ "person",
+ "knows",
+ "person",
+ 1024,
+ 100,
+ 100,
+ false,
+ URI.create("edge/person_knows_person/"),
+ "gar/v1",
+ List.of(TestUtil.orderedBySource),
+ List.of(TestUtil.pg3));
+ // add the created edge to an empty graph
+ Assert.assertEquals(
+ 1,
testingEdgeGraphInfo.addEdgeAsNew(testingEdgeInfo).get().getEdgeInfos().size());
+ // remove the newly created edge and check again the emptied graph
+ Assert.assertEquals(true,
testingEdgeGraphInfo.removeEdge(testingEdgeInfo).isEmpty());
Review Comment:
This test is not correctly testing the removeEdge functionality. The
addEdgeAsNew call returns a new GraphInfo with the edge added, but
testingEdgeGraphInfo remains empty. When removeEdge is called on the empty
testingEdgeGraphInfo, it returns Optional.empty() because the edge doesn't
exist (not because removal resulted in an empty graph). The test should store
the result of addEdgeAsNew and then call removeEdge on that updated graph to
properly test the removal functionality.
```suggestion
GraphInfo graphWithEdge =
testingEdgeGraphInfo.addEdgeAsNew(testingEdgeInfo).get();
Assert.assertEquals(1, graphWithEdge.getEdgeInfos().size());
// remove the newly created edge and check again the emptied graph
Assert.assertEquals(true,
graphWithEdge.removeEdge(testingEdgeInfo).isEmpty());
```
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +109,43 @@ public void testGraphInfoBasics() {
illegalArgumentException.getMessage());
// test version gar/v1
Assert.assertEquals(1, graphInfo.getVersion().getVersion());
+ // basic tests for addVertex and removeVertex (more advanced ones
should include adjancency
+ // list checks)
+ VertexInfo testingVertexInfo =
+ new VertexInfo("", 100, Arrays.asList(TestUtil.pg1),
"vertex/person/", "gar/v1");
+ GraphInfo testingVertexGraphInfo =
+ new GraphInfo("graphVertexTest", new ArrayList<>(), new
ArrayList<>(), "", "");
+ // add the created vertex on an empty graph
+ Assert.assertEquals(
+ 1,
+ testingVertexGraphInfo
+ .addVertexAsNew(testingVertexInfo)
+ .get()
+ .getVertexInfos()
+ .size());
+ // remove the newly created vertex and check again the emptied graph
+ Assert.assertEquals(true,
testingVertexGraphInfo.removeVertex(testingVertexInfo).isEmpty());
Review Comment:
This test is not correctly testing the removeVertex functionality. The
addVertexAsNew call returns a new GraphInfo with the vertex added, but
testingVertexGraphInfo remains empty. When removeVertex is called on the empty
testingVertexGraphInfo, it returns Optional.empty() because the vertex doesn't
exist (not because removal resulted in an empty graph). The test should store
the result of addVertexAsNew and then call removeVertex on that updated graph
to properly test the removal functionality.
```suggestion
GraphInfo graphWithVertex =
testingVertexGraphInfo.addVertexAsNew(testingVertexInfo).get();
Assert.assertEquals(
1,
graphWithVertex
.getVertexInfos()
.size());
// remove the newly created vertex and check again the emptied graph
Assert.assertEquals(true,
graphWithVertex.removeVertex(testingVertexInfo).isEmpty());
```
##########
maven-projects/info/src/main/java/org/apache/graphar/info/GraphInfo.java:
##########
@@ -136,6 +136,45 @@ public Optional<GraphInfo> addVertexAsNew(VertexInfo
vertexInfo) {
edgeConcat2EdgeInfo));
}
+ public Optional<GraphInfo> removeVertex(VertexInfo vertexInfo) {
+
+ if (vertexInfo == null || !hasVertexInfo(vertexInfo.getType())) {
+ return Optional.empty();
+ }
+ List<EdgeInfo> newCachedEdgeInfoList =
+ edgeInfos.stream()
+ .filter(
+ currEdge ->
+
!currEdge.getSrcType().equals(vertexInfo.getType())
+ && !currEdge.getDstType()
+
.equals(vertexInfo.getType()))
+ .collect(Collectors.toList());
+
+ final List<VertexInfo> newVertexInfoList =
+ vertexInfos.stream()
+ .filter(
+ v ->
+
!v.getType().equals(vertexInfo.getType())
+ &&
!v.getPrefix().equals(vertexInfo.getPrefix()))
Review Comment:
The filter condition uses AND (&&) but should use OR (||). A vertex should
be excluded if EITHER its type matches OR its prefix matches the vertex being
removed. With the current AND logic, only vertices that match both type and
prefix would be filtered out, which would fail to remove vertices with the same
type but different prefixes, or vice versa. This should be a single condition
checking only the type, as that's the unique identifier for vertices in the
vertexType2VertexInfo map.
```suggestion
!v.getType().equals(vertexInfo.getType()))
```
--
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]
