Copilot commented on code in PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#discussion_r2618832095
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +110,56 @@ 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 adjacency
+ // list checks)
+ VertexInfo testingVertexInfo =
+ new VertexInfo("", 100, Arrays.asList(TestUtil.pg1),
"vertex/person/", "gar/v1");
+ GraphInfo testingVertexGraphInfo =
+ new GraphInfo("graphVertexTest", new ArrayList<>(), new
ArrayList<>(), "", "");
+ // remove unexisting vertex from an empty graph
+
Assert.assertTrue(testingVertexGraphInfo.removeVertex(testingVertexInfo).isEmpty());
+ // add the created vertex on an empty graph
+ Optional<GraphInfo> addVertexAsNewGraph =
+ testingVertexGraphInfo.addVertexAsNew(testingVertexInfo);
+ Assert.assertTrue(addVertexAsNewGraph.isPresent());
+ testingVertexGraphInfo = addVertexAsNewGraph.get();
+ Assert.assertEquals(1,
addVertexAsNewGraph.get().getVertexInfos().size());
+ // try to add the same vertex again
+
Assert.assertTrue(testingVertexGraphInfo.addVertexAsNew(testingVertexInfo).isEmpty());
+ // test remove vertex
+ addVertexAsNewGraph =
testingVertexGraphInfo.removeVertex(testingVertexInfo);
+ Assert.assertTrue(addVertexAsNewGraph.isPresent());
+ Assert.assertEquals(0,
addVertexAsNewGraph.get().getVertexInfos().size());
+
+ // 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));
+ // remove unexisting edge from an empty graph
+
Assert.assertTrue(testingEdgeGraphInfo.removeEdge(testingEdgeInfo).isEmpty());
+ // add the created edge on an empty graph
+ Optional<GraphInfo> addEdgeAsNewGraph =
testingEdgeGraphInfo.addEdgeAsNew(testingEdgeInfo);
+ Assert.assertTrue(addEdgeAsNewGraph.isPresent());
+ testingEdgeGraphInfo = addEdgeAsNewGraph.get();
+ Assert.assertEquals(1, addEdgeAsNewGraph.get().getEdgeInfos().size());
+ // try to add the same Edge again
+
Assert.assertTrue(testingEdgeGraphInfo.addEdgeAsNew(testingEdgeInfo).isEmpty());
+ // test remove Edge
Review Comment:
Inconsistent capitalization: "Edge" is capitalized here but "vertex" is
lowercase in line 129. For consistency and Java naming conventions, use "edge"
(lowercase) instead of "Edge".
```suggestion
// test remove edge
```
##########
maven-projects/info/src/main/java/org/apache/graphar/info/GraphInfo.java:
##########
@@ -136,6 +136,34 @@ public Optional<GraphInfo> addVertexAsNew(VertexInfo
vertexInfo) {
edgeConcat2EdgeInfo));
}
+ public Optional<GraphInfo> removeVertex(VertexInfo vertexInfo) {
+
+ if (vertexInfo == null || !hasVertexInfo(vertexInfo.getType())) {
+ return Optional.empty();
+ }
+
+ final List<VertexInfo> newVertexInfoList =
+ vertexInfos.stream()
+ .filter(v -> !v.getType().equals(vertexInfo.getType()))
+ .collect(Collectors.toList());
+
+ final Map<String, VertexInfo> newVertexInfoMap =
+ vertexType2VertexInfo.entrySet().stream()
+ .filter(v -> !v.getKey().equals(vertexInfo.getType()))
+ .collect(
+ Collectors.toUnmodifiableMap(
+ Map.Entry::getKey,
Map.Entry::getValue));
+ return Optional.of(
+ new GraphInfo(
+ name,
+ newVertexInfoList,
+ edgeInfos,
+ baseUri,
+ version,
+ newVertexInfoMap,
+ edgeConcat2EdgeInfo));
Review Comment:
The removeVertex method does not remove edges that reference the vertex
being removed. When a vertex is removed, any edges that have this vertex as
source (srcType) or destination (dstType) should also be removed to maintain
graph consistency. This can lead to dangling edge references pointing to
non-existent vertices.
```suggestion
// Remove edges that reference the removed vertex as src or dst
final List<EdgeInfo> newEdgeInfos =
edgeInfos.stream()
.filter(e ->
!e.getSrcType().equals(vertexInfo.getType())
&&
!e.getDstType().equals(vertexInfo.getType()))
.collect(Collectors.toList());
final Map<String, EdgeInfo> newEdgeConcat2EdgeInfo =
edgeConcat2EdgeInfo.entrySet().stream()
.filter(e ->
!e.getValue().getSrcType().equals(vertexInfo.getType()) &&
!e.getValue().getDstType().equals(vertexInfo.getType()))
.collect(
Collectors.toUnmodifiableMap(
Map.Entry::getKey,
Map.Entry::getValue));
return Optional.of(
new GraphInfo(
name,
newVertexInfoList,
newEdgeInfos,
baseUri,
version,
newVertexInfoMap,
newEdgeConcat2EdgeInfo));
```
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +110,56 @@ 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 adjacency
+ // list checks)
+ VertexInfo testingVertexInfo =
+ new VertexInfo("", 100, Arrays.asList(TestUtil.pg1),
"vertex/person/", "gar/v1");
Review Comment:
The VertexInfo is created with an empty string for the type parameter, which
appears to be invalid. The type should be a meaningful identifier like "person"
to properly identify the vertex. An empty type string may cause issues with the
graph structure and lookups.
```suggestion
new VertexInfo("test_vertex", 100,
Arrays.asList(TestUtil.pg1), "vertex/person/", "gar/v1");
```
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +110,56 @@ 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 adjacency
+ // list checks)
+ VertexInfo testingVertexInfo =
+ new VertexInfo("", 100, Arrays.asList(TestUtil.pg1),
"vertex/person/", "gar/v1");
+ GraphInfo testingVertexGraphInfo =
+ new GraphInfo("graphVertexTest", new ArrayList<>(), new
ArrayList<>(), "", "");
+ // remove unexisting vertex from an empty graph
Review Comment:
The word "unexisting" should be "non-existent" or "nonexistent".
```suggestion
// remove non-existent vertex from an empty graph
```
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +110,56 @@ 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 adjacency
+ // list checks)
+ VertexInfo testingVertexInfo =
+ new VertexInfo("", 100, Arrays.asList(TestUtil.pg1),
"vertex/person/", "gar/v1");
+ GraphInfo testingVertexGraphInfo =
+ new GraphInfo("graphVertexTest", new ArrayList<>(), new
ArrayList<>(), "", "");
+ // remove unexisting vertex from an empty graph
+
Assert.assertTrue(testingVertexGraphInfo.removeVertex(testingVertexInfo).isEmpty());
+ // add the created vertex on an empty graph
+ Optional<GraphInfo> addVertexAsNewGraph =
+ testingVertexGraphInfo.addVertexAsNew(testingVertexInfo);
+ Assert.assertTrue(addVertexAsNewGraph.isPresent());
+ testingVertexGraphInfo = addVertexAsNewGraph.get();
+ Assert.assertEquals(1,
addVertexAsNewGraph.get().getVertexInfos().size());
+ // try to add the same vertex again
+
Assert.assertTrue(testingVertexGraphInfo.addVertexAsNew(testingVertexInfo).isEmpty());
+ // test remove vertex
+ addVertexAsNewGraph =
testingVertexGraphInfo.removeVertex(testingVertexInfo);
+ Assert.assertTrue(addVertexAsNewGraph.isPresent());
+ Assert.assertEquals(0,
addVertexAsNewGraph.get().getVertexInfos().size());
Review Comment:
The tests for removeVertex should include test cases where the vertex being
removed has associated edges. The comment on line 113-114 acknowledges this
("more advanced ones should include adjacency list checks"), but these tests
are missing. This is critical to ensure removeVertex handles edge removal
correctly or fails appropriately when edges reference the vertex.
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +110,56 @@ 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 adjacency
+ // list checks)
+ VertexInfo testingVertexInfo =
+ new VertexInfo("", 100, Arrays.asList(TestUtil.pg1),
"vertex/person/", "gar/v1");
+ GraphInfo testingVertexGraphInfo =
+ new GraphInfo("graphVertexTest", new ArrayList<>(), new
ArrayList<>(), "", "");
+ // remove unexisting vertex from an empty graph
+
Assert.assertTrue(testingVertexGraphInfo.removeVertex(testingVertexInfo).isEmpty());
+ // add the created vertex on an empty graph
+ Optional<GraphInfo> addVertexAsNewGraph =
+ testingVertexGraphInfo.addVertexAsNew(testingVertexInfo);
+ Assert.assertTrue(addVertexAsNewGraph.isPresent());
+ testingVertexGraphInfo = addVertexAsNewGraph.get();
+ Assert.assertEquals(1,
addVertexAsNewGraph.get().getVertexInfos().size());
+ // try to add the same vertex again
+
Assert.assertTrue(testingVertexGraphInfo.addVertexAsNew(testingVertexInfo).isEmpty());
+ // test remove vertex
+ addVertexAsNewGraph =
testingVertexGraphInfo.removeVertex(testingVertexInfo);
+ Assert.assertTrue(addVertexAsNewGraph.isPresent());
+ Assert.assertEquals(0,
addVertexAsNewGraph.get().getVertexInfos().size());
+
+ // 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));
+ // remove unexisting edge from an empty graph
+
Assert.assertTrue(testingEdgeGraphInfo.removeEdge(testingEdgeInfo).isEmpty());
+ // add the created edge on an empty graph
+ Optional<GraphInfo> addEdgeAsNewGraph =
testingEdgeGraphInfo.addEdgeAsNew(testingEdgeInfo);
+ Assert.assertTrue(addEdgeAsNewGraph.isPresent());
+ testingEdgeGraphInfo = addEdgeAsNewGraph.get();
+ Assert.assertEquals(1, addEdgeAsNewGraph.get().getEdgeInfos().size());
+ // try to add the same Edge again
Review Comment:
Inconsistent capitalization: "Edge" is capitalized here but "vertex" is
lowercase in line 128. For consistency with line 128 and Java naming
conventions, use "edge" (lowercase) instead of "Edge".
```suggestion
// try to add the same edge again
```
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +110,56 @@ 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 adjacency
+ // list checks)
+ VertexInfo testingVertexInfo =
+ new VertexInfo("", 100, Arrays.asList(TestUtil.pg1),
"vertex/person/", "gar/v1");
+ GraphInfo testingVertexGraphInfo =
+ new GraphInfo("graphVertexTest", new ArrayList<>(), new
ArrayList<>(), "", "");
+ // remove unexisting vertex from an empty graph
+
Assert.assertTrue(testingVertexGraphInfo.removeVertex(testingVertexInfo).isEmpty());
+ // add the created vertex on an empty graph
+ Optional<GraphInfo> addVertexAsNewGraph =
+ testingVertexGraphInfo.addVertexAsNew(testingVertexInfo);
+ Assert.assertTrue(addVertexAsNewGraph.isPresent());
+ testingVertexGraphInfo = addVertexAsNewGraph.get();
+ Assert.assertEquals(1,
addVertexAsNewGraph.get().getVertexInfos().size());
+ // try to add the same vertex again
+
Assert.assertTrue(testingVertexGraphInfo.addVertexAsNew(testingVertexInfo).isEmpty());
+ // test remove vertex
+ addVertexAsNewGraph =
testingVertexGraphInfo.removeVertex(testingVertexInfo);
+ Assert.assertTrue(addVertexAsNewGraph.isPresent());
+ Assert.assertEquals(0,
addVertexAsNewGraph.get().getVertexInfos().size());
+
+ // 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));
+ // remove unexisting edge from an empty graph
Review Comment:
The word "unexisting" should be "non-existent" or "nonexistent".
```suggestion
// remove non-existent edge from an empty graph
```
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +110,56 @@ 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 adjacency
+ // list checks)
+ VertexInfo testingVertexInfo =
+ new VertexInfo("", 100, Arrays.asList(TestUtil.pg1),
"vertex/person/", "gar/v1");
+ GraphInfo testingVertexGraphInfo =
+ new GraphInfo("graphVertexTest", new ArrayList<>(), new
ArrayList<>(), "", "");
+ // remove unexisting vertex from an empty graph
+
Assert.assertTrue(testingVertexGraphInfo.removeVertex(testingVertexInfo).isEmpty());
+ // add the created vertex on an empty graph
+ Optional<GraphInfo> addVertexAsNewGraph =
+ testingVertexGraphInfo.addVertexAsNew(testingVertexInfo);
+ Assert.assertTrue(addVertexAsNewGraph.isPresent());
+ testingVertexGraphInfo = addVertexAsNewGraph.get();
+ Assert.assertEquals(1,
addVertexAsNewGraph.get().getVertexInfos().size());
+ // try to add the same vertex again
+
Assert.assertTrue(testingVertexGraphInfo.addVertexAsNew(testingVertexInfo).isEmpty());
+ // test remove vertex
+ addVertexAsNewGraph =
testingVertexGraphInfo.removeVertex(testingVertexInfo);
+ Assert.assertTrue(addVertexAsNewGraph.isPresent());
+ Assert.assertEquals(0,
addVertexAsNewGraph.get().getVertexInfos().size());
+
+ // 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));
+ // remove unexisting edge from an empty graph
+
Assert.assertTrue(testingEdgeGraphInfo.removeEdge(testingEdgeInfo).isEmpty());
+ // add the created edge on an empty graph
+ Optional<GraphInfo> addEdgeAsNewGraph =
testingEdgeGraphInfo.addEdgeAsNew(testingEdgeInfo);
+ Assert.assertTrue(addEdgeAsNewGraph.isPresent());
+ testingEdgeGraphInfo = addEdgeAsNewGraph.get();
+ Assert.assertEquals(1, addEdgeAsNewGraph.get().getEdgeInfos().size());
+ // try to add the same Edge again
+
Assert.assertTrue(testingEdgeGraphInfo.addEdgeAsNew(testingEdgeInfo).isEmpty());
+ // test remove Edge
+ addEdgeAsNewGraph = testingEdgeGraphInfo.removeEdge(testingEdgeInfo);
+ Assert.assertTrue(addEdgeAsNewGraph.isPresent());
+ Assert.assertEquals(0, addEdgeAsNewGraph.get().getEdgeInfos().size());
Review Comment:
Similar to the vertex test, the variable name "addEdgeAsNewGraph" is
misleading. Consider renaming to "graphWithEdge" or "updatedGraphInfo" for
clarity.
```suggestion
Optional<GraphInfo> graphWithEdge =
testingEdgeGraphInfo.addEdgeAsNew(testingEdgeInfo);
Assert.assertTrue(graphWithEdge.isPresent());
testingEdgeGraphInfo = graphWithEdge.get();
Assert.assertEquals(1, graphWithEdge.get().getEdgeInfos().size());
// try to add the same Edge again
Assert.assertTrue(testingEdgeGraphInfo.addEdgeAsNew(testingEdgeInfo).isEmpty());
// test remove Edge
graphWithEdge = testingEdgeGraphInfo.removeEdge(testingEdgeInfo);
Assert.assertTrue(graphWithEdge.isPresent());
Assert.assertEquals(0, graphWithEdge.get().getEdgeInfos().size());
```
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +110,56 @@ 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 adjacency
+ // list checks)
+ VertexInfo testingVertexInfo =
+ new VertexInfo("", 100, Arrays.asList(TestUtil.pg1),
"vertex/person/", "gar/v1");
+ GraphInfo testingVertexGraphInfo =
+ new GraphInfo("graphVertexTest", new ArrayList<>(), new
ArrayList<>(), "", "");
+ // remove unexisting vertex from an empty graph
+
Assert.assertTrue(testingVertexGraphInfo.removeVertex(testingVertexInfo).isEmpty());
+ // add the created vertex on an empty graph
+ Optional<GraphInfo> addVertexAsNewGraph =
+ testingVertexGraphInfo.addVertexAsNew(testingVertexInfo);
+ Assert.assertTrue(addVertexAsNewGraph.isPresent());
+ testingVertexGraphInfo = addVertexAsNewGraph.get();
+ Assert.assertEquals(1,
addVertexAsNewGraph.get().getVertexInfos().size());
+ // try to add the same vertex again
+
Assert.assertTrue(testingVertexGraphInfo.addVertexAsNew(testingVertexInfo).isEmpty());
+ // test remove vertex
+ addVertexAsNewGraph =
testingVertexGraphInfo.removeVertex(testingVertexInfo);
+ Assert.assertTrue(addVertexAsNewGraph.isPresent());
+ Assert.assertEquals(0,
addVertexAsNewGraph.get().getVertexInfos().size());
Review Comment:
The variable name "addVertexAsNewGraph" is misleading. It suggests the
entire graph is new, when actually it's a new GraphInfo instance with the
vertex added. A better name would be "graphWithVertex" or "updatedGraphInfo" to
clarify that it's a modified version of the original graph.
```suggestion
Optional<GraphInfo> graphWithVertex =
testingVertexGraphInfo.addVertexAsNew(testingVertexInfo);
Assert.assertTrue(graphWithVertex.isPresent());
testingVertexGraphInfo = graphWithVertex.get();
Assert.assertEquals(1,
graphWithVertex.get().getVertexInfos().size());
// try to add the same vertex again
Assert.assertTrue(testingVertexGraphInfo.addVertexAsNew(testingVertexInfo).isEmpty());
// test remove vertex
graphWithVertex =
testingVertexGraphInfo.removeVertex(testingVertexInfo);
Assert.assertTrue(graphWithVertex.isPresent());
Assert.assertEquals(0,
graphWithVertex.get().getVertexInfos().size());
```
--
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]
