ARROW-17297: [Java][Doc] Adding documentation to interact between C++ to Java via C Data Interface #13788

davisusanibar · 2022-08-03T15:19:27Z

Adding documentation to interact between C++ to Java via C Data Interface for:

ImportArray
ImportRecordBatch

…face

lidavidm · 2022-08-03T17:56:30Z

Can you create a JIRA? This isn't minor

lidavidm · 2022-08-03T18:56:48Z

docs/source/java/cdata.rst

@@ -33,7 +33,10 @@ Python communication using the C Data Interface.
 Java to C++
 -----------

-Example: Share an Int64 array from C++ to Java:
+Share an Int64 array from C++ to Java
+=====================================


This is the wrong level of header

docs/source/java/cdata.rst

lidavidm · 2022-08-03T18:58:30Z

docs/source/java/cdata.rst

+            return root;
+        }
+
+        public static void main(String[] args) {


We don't need a main method, do we?

docs/source/java/cdata.rst

github-actions · 2022-08-03T19:29:58Z

https://issues.apache.org/jira/browse/ARROW-17297

github-actions · 2022-08-03T19:29:59Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

Co-authored-by: David Li <[email protected]>

…face

lidavidm · 2022-08-03T21:22:20Z

docs/source/java/cdata.rst

+
+.. code-block:: java
+
+    import org.apache.arrow.c.ArrowArray;


nit, but everything is indented one space too many here

that's actually true of all the code blocks

lidavidm · 2022-08-03T21:23:12Z

docs/source/java/cdata.rst

+        int status = JNI_CreateJavaVM(jvm, (void **) &env, &vm_args);
+        if (status < 0) {
+            std::cout << "\n<<<<< Unable to Launch JVM >>>>>\n" << std::endl;
+            std::abort();


Requires #include <cstdlib>

Actually I suppose the abort isn't necessary, maybe just explicitly return nullptr and it'll exit in main()

lidavidm · 2022-08-03T21:23:35Z

docs/source/java/cdata.rst

+        jclass javaClassToBeCalledByCpp = NULL;
+        javaClassToBeCalledByCpp = env->FindClass("ToBeCalledByCpp");
+        if (javaClassToBeCalledByCpp != NULL) {
+            jmethodID fillVector = NULL;


Please update the other uses of NULL to be nullptr

Though, why separate the declaration and assignment? Just do it in one line

lidavidm · 2022-08-03T21:23:59Z

docs/source/java/cdata.rst

+    find_package(JNI REQUIRED)
+    find_package(Arrow REQUIRED)
+    message(STATUS "Arrow version: ${ARROW_VERSION}")
+    message(${ARROW_FULL_SO_VERSION})


This needs to be updated like the line above

Message not needed, deleted

lidavidm · 2022-08-03T21:24:17Z

docs/source/java/cdata.rst

+        JNIEnv *env;
+        JavaVM *jvm;
+        env = CreateVM(&jvm);
+        if (env == nullptr) return 1;


use EXIT_FAILURE here too as well

lidavidm · 2022-08-03T21:26:28Z

docs/source/java/cdata.rst

+                struct ArrowSchema arrowSchema;
+                struct ArrowArray arrowArray;
+                std::cout << "\n<<<<< C++ to Java for Arrays >>>>>\n" << std::endl;
+                env->CallStaticVoidMethod(javaClassToBeCalledByCpp, fillVector, reinterpret_cast<uintptr_t>(&arrowSchema),


I would wrap lines to be a little shorter here if possible to make sure it's readable in Sphinx

docs/source/java/cdata.rst

Co-authored-by: David Li <[email protected]>

lidavidm · 2022-08-04T14:56:17Z

docs/source/java/cdata.rst

+           <maven.compiler.source>11</maven.compiler.source>
+           <maven.compiler.target>11</maven.compiler.target>


Any reason to use Java 11 here?

lidavidm · 2022-08-04T14:56:22Z

docs/source/java/cdata.rst

+       <properties>
+           <maven.compiler.source>11</maven.compiler.source>
+           <maven.compiler.target>11</maven.compiler.target>
+           <arrow.version>8.0.0</arrow.version>


Update to 9.0.0?

lidavidm · 2022-08-04T14:56:30Z

docs/source/java/cdata.rst

+       <properties>
+           <maven.compiler.source>8</maven.compiler.source>
+           <maven.compiler.target>8</maven.compiler.target>
+           <arrow.version>8.0.0</arrow.version>


Might as well update to 9.0.0 now

lidavidm · 2022-08-04T14:56:44Z

docs/source/java/cdata.rst

+       <repositories>
+           <repository>
+               <id>arrow-nightly</id>
+               <url>https://nightlies.apache.org/arrow/java</url>


No need to use nightlies right?

lidavidm · 2022-08-04T14:57:00Z

docs/source/java/cdata.rst

+                               <archive>
+                                   <manifest>
+                                       <mainClass>
+                                           ToBeCalledByCpp


Do we need the mainClass config?

lidavidm · 2022-08-04T14:58:25Z

docs/source/java/cdata.rst

+               struct ArrowArray arrowArray;
+               std::cout << "\n<<<<< C++ to Java for RecordBatch >>>>>\n" << std::endl;
+               env->CallStaticVoidMethod(javaClassToBeCalledByCpp, fillVectorSchemaRoot,
+                                         static_cast<long>(reinterpret_cast<uintptr_t>(&arrowSchema)),


Don't we need the static_cast<long> above as well then?

(Also, shouldn't it be jlong?)

All of these kind of declaration work well (I don't know what of this should I use?)

env->CallStaticVoidMethod(javaClassToBeCalledByCpp, fillVector, static_cast<long>(reinterpret_cast<uintptr_t>(&arrowSchema)), static_cast<long>(reinterpret_cast<uintptr_t>(&arrowArray))); env->CallStaticVoidMethod(javaClassToBeCalledByCpp, fillVector, reinterpret_cast<uintptr_t>(&arrowSchema), reinterpret_cast<uintptr_t>(&arrowArray)); env->CallStaticVoidMethod(javaClassToBeCalledByCpp, fillVector, (long)(&arrowSchema), (long)(&arrowArray)); env->CallStaticVoidMethod(javaClassToBeCalledByCpp, fillVector, (jlong)(&arrowSchema), (jlong)(&arrowArray)); env->CallStaticVoidMethod(javaClassToBeCalledByCpp, fillVector, (uintptr_t)(&arrowSchema), (uintptr_t)(&arrowArray));

To be the most compliant, I think it should be:

env->CallStaticVoidMethod(javaClassToBeCalledByCpp, fillVector, static_cast<jlong>(reinterpret_cast<uintptr_t>(&arrowSchema)), static_cast<jlong>(reinterpret_cast<uintptr_t>(&arrowArray)));

(note jlong not long)

docs/source/java/cdata.rst

pitrou · 2022-08-04T15:59:13Z

docs/source/java/cdata.rst

+               struct ArrowArray arrowArray;
+               std::cout << "\n<<<<< C++ to Java for RecordBatch >>>>>\n" << std::endl;
+               env->CallStaticVoidMethod(javaClassToBeCalledByCpp, fillVectorSchemaRoot,
+                                         static_cast<long>(reinterpret_cast<uintptr_t>(&arrowSchema)),


To be the most compliant, I think it should be:

env->CallStaticVoidMethod(javaClassToBeCalledByCpp, fillVector, static_cast<jlong>(reinterpret_cast<uintptr_t>(&arrowSchema)), static_cast<jlong>(reinterpret_cast<uintptr_t>(&arrowArray)));

(note jlong not long)

docs/source/java/cdata.rst

pitrou · 2022-08-04T16:08:21Z

docs/source/java/cdata.rst

+           }
+       }
+
+       public static FieldVector populateFieldVectorToExport(){


Shouldn't you make this private?

pitrou · 2022-08-04T16:08:27Z

docs/source/java/cdata.rst

+           }
+       }
+
+       public static VectorSchemaRoot populateVectorSchemaRootToExport(){


private as well?

pitrou · 2022-08-04T16:08:40Z

docs/source/java/cdata.rst

+   public class ToBeCalledByCpp {
+       final static BufferAllocator allocator = new RootAllocator();
+
+       public static void fillVector(long schemaAddress, long arrayAddress){


Can you add small docstring describing the two public methods?

Co-authored-by: Antoine Pitrou <[email protected]>

…itory and Java 11 version references

docs/source/java/cdata.rst

Co-authored-by: David Li <[email protected]>

lidavidm · 2022-08-05T18:25:46Z

Lint issues are addressed by #13809

ursabot · 2022-08-06T03:22:12Z

Benchmark runs are scheduled for baseline = bc7de40 and contender = d26489c. d26489c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.65% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.27% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.43% ⬆️0.07%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d26489c3 ec2-t3-xlarge-us-east-2
[Finished] d26489c3 test-mac-arm
[Finished] d26489c3 ursa-i9-9960x
[Finished] d26489c3 ursa-thinkcentre-m75q
[Finished] bc7de406 ec2-t3-xlarge-us-east-2
[Finished] bc7de406 test-mac-arm
[Finished] bc7de406 ursa-i9-9960x
[Finished] bc7de406 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Adding documentation to interact between C++ to Java via C Data Inter…

a3c7035

…face

github-actions bot added the Component: Documentation label Aug 3, 2022

lidavidm reviewed Aug 3, 2022

View reviewed changes

davisusanibar changed the title ~~MINOR: [Java] Adding documentation to interact between C++ to Java via C Data Interface~~ ARROW-17297: [Java][Doc] Adding documentation to interact between C++ to Java via C Data Interface Aug 3, 2022

davisusanibar and others added 2 commits August 3, 2022 14:47

Apply suggestions from code review

dda742a

Co-authored-by: David Li <[email protected]>

Adding documentation to interact between C++ to Java via C Data Inter…

c4fe1b2

…face

lidavidm reviewed Aug 3, 2022

View reviewed changes

lidavidm mentioned this pull request Aug 4, 2022

[Java][C++] Java VectorSchemaRoot to C++ RecordBatchreader #13703

Closed

davisusanibar and others added 2 commits August 4, 2022 07:50

Apply suggestions from code review

f9ea73e

Co-authored-by: David Li <[email protected]>

Format rst code + C++ use of semantic error codes

d475042

lidavidm reviewed Aug 4, 2022

View reviewed changes

pitrou reviewed Aug 4, 2022

View reviewed changes

davisusanibar and others added 2 commits August 4, 2022 11:55

Apply suggestions from code review

d68ac3f

Co-authored-by: Antoine Pitrou <[email protected]>

Use Arrow 9.0.0, compliant parameters methods, delete nightlies repos…

0c21eae

…itory and Java 11 version references

lidavidm approved these changes Aug 5, 2022

View reviewed changes

docs/source/java/cdata.rst Outdated Show resolved Hide resolved

docs/source/java/cdata.rst Outdated Show resolved Hide resolved

Apply suggestions from code review

5ff8893

Co-authored-by: David Li <[email protected]>

lidavidm merged commit d26489c into apache:master Aug 5, 2022

		<maven.compiler.source>11</maven.compiler.source>
		<maven.compiler.target>11</maven.compiler.target>

ARROW-17297: [Java][Doc] Adding documentation to interact between C++ to Java via C Data Interface #13788

ARROW-17297: [Java][Doc] Adding documentation to interact between C++ to Java via C Data Interface #13788

Conversation

davisusanibar commented Aug 3, 2022

lidavidm commented Aug 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 3, 2022

github-actions bot commented Aug 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented Aug 5, 2022

ursabot commented Aug 6, 2022