You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@graphar.apache.org by "unical1988 (via GitHub)" <gi...@apache.org> on 2025/09/22 19:08:40 UTC
[PR] add remove vertex edge, TODO: tests [incubator-graphar]
unical1988 opened a new pull request, #773:
URL: https://github.com/apache/incubator-graphar/pull/773
<!--
Thanks for contributing to GraphAr.
If this is your first pull request you can find detailed information on [CONTRIBUTING.md](https://github.com/apache/graphar/blob/main/CONTRIBUTING.md)
The Apache GraphAr (incubating) community has restrictions on the naming of pr title. You can find instructions in
[CONTRIBUTING.md](https://github.com/apache/graphar/blob/main/CONTRIBUTING.md#title) too.
-->
### Reason for this PR
<!--
Why are you proposing this change? If this is already tracked in an issue, please link to the issue here.
Explaining clearly why this change is beneficial is important for the reviewers to understand the context.
-->
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
### Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **BREAKING CHANGE: <description>** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either
(a) a security vulnerability,
(b) a bug that caused incorrect or invalid data to be produced, or
(c) a bug that causes a crash (even when the API contract is upheld).
We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **Critical Fix: <description>** -->
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "sapienza88 (via GitHub)" <gi...@apache.org>.
sapienza88 commented on code in PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#discussion_r2616543678
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +109,19 @@ 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 testingGraphInfo =
+ new GraphInfo("graphTest", new ArrayList<>(), new ArrayList<>(), "", "");
+ // add the created vertex on an empty graph
+ Assert.assertEquals(
+ 1,
+ testingGraphInfo.addVertexAsNew(testingVertexInfo).get().getVertexInfos().size());
+ // remove the newly created vertex and check again the emptied graph
+ Assert.assertEquals(true, testingGraphInfo.removeVertex(testingVertexInfo).isEmpty());
+ // TODO (same tests as vertices for edges)
Review Comment:
done
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "sapienza88 (via GitHub)" <gi...@apache.org>.
sapienza88 commented on PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#issuecomment-3637537072
> > Do we need to consider the adjacency list when deleting a vertex/edge?
>
> Do you mean that delete adjacency list in edgeInfo?
>
@yangxk1 adjacency list can be involved when deleting a node, do you need to add a test for that?
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "unical1988 (via GitHub)" <gi...@apache.org>.
unical1988 commented on PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#issuecomment-3323028591
> > Do we need to consider the adjacency list when deleting a vertex/edge?
>
> Do you mean that delete adjacency list in edgeInfo?
>
> > where do you test addVertex() in GraphInfo.java ?
>
> I don't think we tested `addVertex`, can you add tests for them in `GraphInfoTest`/`EdgeInfoTest`/`VertexInfoTest` by the way?
Ok, where is the yaml file, to load a sample testing graph from which i can add or remove vertex and validate?
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(java,info): add remove vertex edge method [incubator-graphar]
Posted by "sapienza88 (via GitHub)" <gi...@apache.org>.
sapienza88 commented on PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#issuecomment-3665196286
> @sapienza88 , I will help you solve these problems
@yangxk1 Thanks for the help and the merge !
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#issuecomment-3323051152
In [`GraphInfoTest::setup`](https://github.com/apache/incubator-graphar/blob/b7768ad09ece6941a32e65e436f765fdf3c1a0fc/maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java#L51), a GraphInfo has been loaded and parsed, you can test method by it.
https://github.com/apache/incubator-graphar/blob/b7768ad09ece6941a32e65e436f765fdf3c1a0fc/maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java#L39
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "sapienza88 (via GitHub)" <gi...@apache.org>.
sapienza88 commented on PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#issuecomment-3637528134
> @unical1988 , please note that runs ` git submodule update --remote` use the latest test data
@yangxk1 done.
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on code in PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#discussion_r2618396090
##########
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 =
Review Comment:
no need `newCachedEdgeInfoList`, no need to update edge
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on code in PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#discussion_r2626145819
##########
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:
as expected
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#issuecomment-3654333217
@sapienza88 , I will help you solve these problems
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on code in PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#discussion_r2418356497
##########
maven-projects/info/pom.xml:
##########
@@ -92,6 +92,11 @@
<version>${spotless-maven-plugin.version}</version>
<configuration>
<java>
+ <!--<includes>
Review Comment:
please remember to delete this line
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#issuecomment-3639793463
> adjacency list can be involved when deleting a node, do you need to add a test for that?
@sapienza88 Very interesting question!
But I think there is **no need to consider deleting the associated edge(adjacency list) when deleting a node.** First of all, graphar provides an API for the [table view](https://github.com/apache/incubator-graphar/blob/146eb6159dc3df14f15f1ea6f91584d561b4507f/cpp/examples/mid_level_reader_example.cc). In this view, users do not need to care whether the node associated with the edge exists. In addition, GraphAr provides the ability to read on demand. In some algorithms, only edge information is needed without reading node information. For example, in the SSSP algorithm, I only need to read the edge table and obtain its src\dst\weight. I do not need to read the property information of the node.
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "sapienza88 (via GitHub)" <gi...@apache.org>.
sapienza88 commented on PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#issuecomment-3643498066
> > adjacency list can be involved when deleting a node, do you need to add a test for that?
>
> @sapienza88 Very interesting question! But I think there is **no need to consider deleting the associated edge(adjacency list) when deleting a node.** First of all, graphar provides an API for the [table view](https://github.com/apache/incubator-graphar/blob/146eb6159dc3df14f15f1ea6f91584d561b4507f/cpp/examples/mid_level_reader_example.cc). In this view, users do not need to care whether the node associated with the edge exists. In addition, GraphAr provides the ability to read on demand. In some algorithms, only edge information is needed without reading node information. For example, in the SSSP algorithm, I only need to read the edge table and obtain its src\dst\weight. I do not need to read the property information of the node.
@yangxk1 ok, so what remains to code for this PR to get merged?
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "unical1988 (via GitHub)" <gi...@apache.org>.
unical1988 commented on PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#issuecomment-3321053519
@yangxk1 where do you test `addVertex()` in `GraphInfo.java `?
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#issuecomment-3337040440
@unical1988 , please note that runs ` git submodule update --remote` use the latest test data
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on code in PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#discussion_r2612620187
##########
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 =
Review Comment:
As we discussed, we don't need this logic anymore
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +109,19 @@ 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 testingGraphInfo =
+ new GraphInfo("graphTest", new ArrayList<>(), new ArrayList<>(), "", "");
+ // add the created vertex on an empty graph
+ Assert.assertEquals(
+ 1,
+ testingGraphInfo.addVertexAsNew(testingVertexInfo).get().getVertexInfos().size());
+ // remove the newly created vertex and check again the emptied graph
+ Assert.assertEquals(true, testingGraphInfo.removeVertex(testingVertexInfo).isEmpty());
+ // TODO (same tests as vertices for edges)
Review Comment:
Please add a test for edge
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +109,19 @@ 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 testingGraphInfo =
+ new GraphInfo("graphTest", new ArrayList<>(), new ArrayList<>(), "", "");
+ // add the created vertex on an empty graph
+ Assert.assertEquals(
+ 1,
+ testingGraphInfo.addVertexAsNew(testingVertexInfo).get().getVertexInfos().size());
+ // remove the newly created vertex and check again the emptied graph
Review Comment:
Now only the new method is verified without EXCEPTIONS, the result of the method needs to be calibrated. For example verify the count of vertex/edge in graph.
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "sapienza88 (via GitHub)" <gi...@apache.org>.
sapienza88 commented on code in PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#discussion_r2616544167
##########
maven-projects/info/src/test/java/org/apache/graphar/info/GraphInfoTest.java:
##########
@@ -109,6 +109,19 @@ 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 testingGraphInfo =
+ new GraphInfo("graphTest", new ArrayList<>(), new ArrayList<>(), "", "");
+ // add the created vertex on an empty graph
+ Assert.assertEquals(
+ 1,
+ testingGraphInfo.addVertexAsNew(testingVertexInfo).get().getVertexInfos().size());
+ // remove the newly created vertex and check again the emptied graph
Review Comment:
what could be considered as an exception when adding removing vertices?
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on code in PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#discussion_r2626152895
##########
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:
ignore
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#issuecomment-3322183780
> Do we need to consider the adjacency list when deleting a vertex/edge?
Do you mean that delete adjacency list in edgeInfo?
> where do you test addVertex() in GraphInfo.java ?
I don't think we tested addVertex, can you add a test for him in `GraphInfoTest`/`EdgeInfoTest`/`VertexInfoTest` by the way?
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#issuecomment-3320976132
## [Codecov](https://app.codecov.io/gh/apache/incubator-graphar/pull/773?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `39 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 76.14%. Comparing base ([`b7768ad`](https://app.codecov.io/gh/apache/incubator-graphar/commit/b7768ad09ece6941a32e65e436f765fdf3c1a0fc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`cda351e`](https://app.codecov.io/gh/apache/incubator-graphar/commit/cda351ef5a779e32428fe3554ea03f6acb1fa099?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/incubator-graphar/pull/773?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...c/main/java/org/apache/graphar/info/GraphInfo.java](https://app.codecov.io/gh/apache/incubator-graphar/pull/773?src=pr&el=tree&filepath=maven-projects%2Finfo%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fgraphar%2Finfo%2FGraphInfo.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bWF2ZW4tcHJvamVjdHMvaW5mby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ3JhcGhhci9pbmZvL0dyYXBoSW5mby5qYXZh) | 0.00% | [39 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-graphar/pull/773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #773 +/- ##
============================================
- Coverage 76.48% 76.14% -0.35%
Complexity 535 535
============================================
Files 82 82
Lines 8638 8677 +39
Branches 1001 1006 +5
============================================
Hits 6607 6607
- Misses 1815 1854 +39
Partials 216 216
```
| [Flag](https://app.codecov.io/gh/apache/incubator-graphar/pull/773/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [java-info](https://app.codecov.io/gh/apache/incubator-graphar/pull/773/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.92% <0.00%> (-3.08%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-graphar/pull/773?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "sapienza88 (via GitHub)" <gi...@apache.org>.
sapienza88 commented on code in PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#discussion_r2607037703
##########
maven-projects/info/pom.xml:
##########
@@ -92,6 +92,11 @@
<version>${spotless-maven-plugin.version}</version>
<configuration>
<java>
+ <!--<includes>
Review Comment:
done
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on code in PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#discussion_r2618566377
##########
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:
ignore
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] [WIP] add remove vertex edge, TODO: tests [incubator-graphar]
Posted by "sapienza88 (via GitHub)" <gi...@apache.org>.
sapienza88 commented on code in PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773#discussion_r2616543802
##########
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 =
Review Comment:
which one exactly?
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(java,info): add remove vertex edge method [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 merged PR #773:
URL: https://github.com/apache/incubator-graphar/pull/773
--
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: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org