Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Doc] Provide Java's reference library, documentation for users and developers #242

Merged
merged 10 commits into from
Oct 30, 2023

Conversation

Thespica
Copy link
Contributor

@Thespica Thespica commented Sep 28, 2023

Proposed changes

As titile.

  • API reference
  • Document for users
  • Document for Java developers

Besides, using GOOGLE Java style rather than AOSP style.

Types of changes

What types of changes does your code introduce to GraphAr?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

Put an x in 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.

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Related issue #72 #240

@Thespica Thespica marked this pull request as ready for review October 29, 2023 14:21
@Thespica Thespica changed the title [WIP][Doc] Provide Java's documents for users and developers [Doc] Provide Java's reference library, documentation for users and developers Oct 29, 2023
By writing CMakeLists.txt, all C++ dependents(e.g. JNI code, GraphAr C++
library and other C++ library) will been integrated into a bridge
dynamic library called gar-jni which can be called by native methods
directly.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here can add a link to CMakeLists.txt as example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have written CMakeLists.txt, developers only need know the architecture, not need to write new CMakeLists.

I think add a link to CMakeLists is enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If developer do not need to know the CMakeLists, that we should talk about CMakeList here

Building GraphAr Java
---------------------

Please refer to user guide.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a link to the user guide


Tips:

- From apt-get:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should change to Use Ubuntu as example:


$ mvn clean install -DskipTests

Then set GraphAr as a dependency in maven project:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the maven project has not set, so maybe we should not include this part. We can add this part back if maven is ready

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User needs to add this part in pom.xml so the project can import the gar-java library

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean after building the project, the user can use the project directly?

java/README.md Outdated


Tips:
- From apt-get:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like above comment.

java/pom.xml Outdated
@@ -258,7 +289,7 @@
<java>
<googleJavaFormat>
<version>1.7</version>
<style>AOSP</style>
<style>GOOGLE</style>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think AOSP is find for JAVA, DO we have any necessary reason to change the format configuration?

to build in Java. Instead, the GraphAr Java library provide a static
method to convert VectorSchemaRoot into arrow::Table. Warning: There are
some problems concerning this method which lead to memory leaks. We will
fix it or rewritre writer with Apache arrow Java.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: rewritre -> rewrite

@@ -0,0 +1,63 @@
Java Devolopment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: Development

Copy link
Contributor

@acezen acezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@acezen acezen merged commit c752915 into apache:main Oct 30, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants