[FEAT][JAVA] Implement READERS and WRITERS for Java #233
Conversation
Thespica
changed the title
.github/workflows/java.yml
Outdated
| @@ -43,7 +43,7 @@ jobs: | |||
|
|
|||
| - name: Code Format Check | |||
| run: | | |||
| export JAVA_HOME=${JAVA_HOME_11_X64} | |||
| export JAVA_HOME=${JAVA_HOME_8_X64} | |||
There was a problem hiding this comment.
I haven't used features that higher than Java 8, and Java 8 have better compatibility with others graph systems which rely on Java 8. So maybe we should assume that environment is Java 8, and Java higher is compatible with lower version.
There was a problem hiding this comment.
the spark rely on java 11, so for CI unification and avoid confusion， I think 11 is better.
| find_package(gar REQUIRED) | ||
|
|
||
| add_library(${LIBNAME} SHARED ${SOURCES}) | ||
| target_link_libraries(${LIBNAME} ${CMAKE_JNI_LINKER_FLAGS} gar) | ||
| target_link_libraries(${LIBNAME} ${CMAKE_JNI_LINKER_FLAGS} Arrow::arrow_static) |
There was a problem hiding this comment.
maybe link to shared library is better? otherwise it would take a long time to compile
There was a problem hiding this comment.
On one head, linking arrow as dynamic will leads error about libprotobuf: https://github.com/Thespica/GraphAr/actions/runs/6206365474/job/16850543098#step:6:9100
On the other head, I consider linking arrow as static just copy arrow lib once, rather than recompile, which only cost little time.
There was a problem hiding this comment.
OK, this can remain and open as a issue to fix in the future.
| @@ -16,6 +16,7 @@ | |||
| <maven.compiler.target>8</maven.compiler.target> | |||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | |||
| <fastffi.revision>0.1.2</fastffi.revision> | |||
| <arrow.version>13.0.0</arrow.version> | |||
There was a problem hiding this comment.
Does this have to be set? If user install other version of arrow, the program still work as expect?
There was a problem hiding this comment.
Maven will automatically download needed jar with specific version and manage them, so it will works.
But maybe relying arrow 12 will be better?
There was a problem hiding this comment.
if work as expected, 13.0.0 is ok
| * @param propertyGroup The property group that describes the property group. | ||
| * @param prefix The absolute prefix. | ||
| */ | ||
| // VertexPropertyArrowChunkReader create( |
There was a problem hiding this comment.
Can you add some comment here if the code has to remain?
Proposed changes
As titile.
Next step: Add related documents and library referance.
Types of changes
What types of changes does your code introduce to GraphAr?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
Related issue: #72