You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@graphar.apache.org by "Thespica (via GitHub)" <gi...@apache.org> on 2025/04/11 05:48:57 UTC
[I] [java-info] Unit test for loading graph [incubator-graphar]
Thespica opened a new issue, #676:
URL: https://github.com/apache/incubator-graphar/issues/676
### Describe the enhancement requested
Write unit test for java-info module, loading graph from [test set](https://github.com/apache/incubator-graphar-testing/tree/955596c325ceba7b607e285738e3dd0ce4ff424e), ensuring that info of:
- [ ] GraphInfo / VertexInfo / EdgeInfo
- [ ] PropertyGroup
- [ ] AdjacentList
- [ ] Property
are loaded as expected.
Can reference [info test of C++](https://github.com/apache/incubator-graphar/blob/main/cpp/test/test_info.cc) and [info test of scala](https://github.com/apache/incubator-graphar/blob/main/maven-projects/spark/graphar/src/test/scala/org/apache/graphar/TestGraphInfo.scala).
### Component(s)
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.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: [I] feat(java, info): Unit test for loading graph [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on issue #676:
URL: https://github.com/apache/incubator-graphar/issues/676#issuecomment-2826645975
I noticed that some interfaces in the Java code are not fully aligned with those in the C++ code. For example, the [Java code](https://github.com/apache/incubator-graphar/blob/main/maven-projects/java/src/main/java/org/apache/graphar/graphinfo/VertexInfo.java) is missing the `vertexInfo::getPropertyGroupByIndex` interface, which is available in the [C++ code](https://github.com/apache/incubator-graphar/blob/main/cpp/src/graphar/graph_info.h) . Also, the current Java project depends on version `v0.10.0` of the C++ code, which is quite outdated. Should we consider updating the Java project first?
--
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: [I] feat(java, info): Unit test for loading graph [incubator-graphar]
Posted by "Thespica (via GitHub)" <gi...@apache.org>.
Thespica commented on issue #676:
URL: https://github.com/apache/incubator-graphar/issues/676#issuecomment-2904023894
> While writing unit tests, I found three issues:
>
> 1. Extra `/` in path concatenation(e.g. `"vertex/person//id/"`) (minor).
> 2. Missing `getVersion()` in GraphInfo, VertexInfo, and EdgeInfo (may affect usage).
> 3. Parameter order mismatch in EdgeInfo constructor vs. EdgeYaml#toEdgeInfo (**this can lead to serious bugs**).
> Specifically, In the constructor of EdgeInfo, the parameter srcChunkSize appears before chunkSize. However, in org.apache.graphar.info.yaml.EdgeYaml#toEdgeInfo, srcChunkSize is passed after chunkSize.
1. Extra / in path concatenation: Thanks for pointing this out (e.g., "vertex/person//id/"). I'm not entirely sure where this redundant / might have been introduced in the path concatenation. If you've pinpointed the exact location in the code, a pull request to fix this would be welcome!
2. Missing getVersion() in GraphInfo, VertexInfo, and EdgeInfo: The getVersion() method wasn't included because the original proto files for these information classes did not define a version field. If having a version is important for your use case, please feel free to open a new, separate issue to propose this enhancement.
3. Parameter order mismatch in EdgeInfo constructor vs. EdgeYaml#toEdgeInfo: You are absolutely right about this! The discrepancy in parameter order (srcChunkSize and chunkSize) between the EdgeInfo constructor and the org.apache.graphar.info.yaml.EdgeYaml#toEdgeInfo method is a serious concern and could indeed lead to bugs. This is a perfect example of why increasing our unit test coverage is so important. Thank you for catching this critical detail!
--
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: [I] feat(java, info): Unit test for loading graph [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on issue #676:
URL: https://github.com/apache/incubator-graphar/issues/676#issuecomment-2893375821
While writing unit tests, I found three issues:
1. Extra `/` in path concatenation(e.g. `"vertex/person//id/"`) (minor).
2. Missing `getVersion()` in GraphInfo, VertexInfo, and EdgeInfo (may affect usage).
3. Parameter order mismatch in EdgeInfo constructor vs. EdgeYaml#toEdgeInfo (**this can lead to serious bugs**).
Specifically, In the constructor of EdgeInfo, the parameter srcChunkSize appears before chunkSize. However, in org.apache.graphar.info.yaml.EdgeYaml#toEdgeInfo, srcChunkSize is passed after chunkSize.
--
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: [I] feat(java, info): Unit test for loading graph [incubator-graphar]
Posted by "Thespica (via GitHub)" <gi...@apache.org>.
Thespica commented on issue #676:
URL: https://github.com/apache/incubator-graphar/issues/676#issuecomment-2829232376
> I noticed that some interfaces in the Java code are not fully aligned with those in the C++ code. For example, the [Java code](https://github.com/apache/incubator-graphar/blob/main/maven-projects/java/src/main/java/org/apache/graphar/graphinfo/VertexInfo.java) is missing the `vertexInfo::getPropertyGroupByIndex` interface, which is available in the [C++ code](https://github.com/apache/incubator-graphar/blob/main/cpp/src/graphar/graph_info.h) . Also, the current Java project depends on version `v0.10.0` of the C++ code, which is quite outdated. Should we consider updating the Java project first?
Hi @yangxk1 , maybe there are some ambiguous expression. The `java-info` here refer to the [info](https://github.com/apache/incubator-graphar/tree/main/maven-projects/info) packages, rather than the [original java](https://github.com/apache/incubator-graphar/tree/main/maven-projects/java) package. You can reach out to us if there is any other problem : )
--
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: [I] feat(java, info): Unit test for loading graph [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on issue #676:
URL: https://github.com/apache/incubator-graphar/issues/676#issuecomment-2893360061
> > I noticed that some interfaces in the Java code are not fully aligned with those in the C++ code. For example, the [Java code](https://github.com/apache/incubator-graphar/blob/main/maven-projects/java/src/main/java/org/apache/graphar/graphinfo/VertexInfo.java) is missing the `vertexInfo::getPropertyGroupByIndex` interface, which is available in the [C++ code](https://github.com/apache/incubator-graphar/blob/main/cpp/src/graphar/graph_info.h) . Also, the current Java project depends on version `v0.10.0` of the C++ code, which is quite outdated. Should we consider updating the Java project first?
>
> Hi [@yangxk1](https://github.com/yangxk1) , maybe there are some ambiguous expression. The `java-info` here refer to the [info](https://github.com/apache/incubator-graphar/tree/main/maven-projects/info) packages, rather than the [original java](https://github.com/apache/incubator-graphar/tree/main/maven-projects/java) package. You can reach out to us if there is any other problem : )
Thanks for your reply! I've opened a PR to improve the [GraphLoaderTest#testLoad](https://github.com/apache/incubator-graphar/blob/main/maven-projects/info/src/test/java/org/apache/graphar/info/GraphLoaderTest.java) method in java-info.
--
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: [I] feat(java, info): Unit test for loading graph [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on issue #676:
URL: https://github.com/apache/incubator-graphar/issues/676#issuecomment-2904077824
(1) and (2). aren’t urgent. I’ll open an issue to fix (3).
--
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: [I] feat(java, info): Unit test for loading graph [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 closed issue #676: feat(java, info): Unit test for loading graph
URL: https://github.com/apache/incubator-graphar/issues/676
--
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