[GitHub] [arrow] pitrou commented on a diff in pull request #13465: ARROW-16913: [Java] Implement ArrowArrayStream

[GitBox <gi...@apache.org>]

pitrou commented on code in PR #13465:
URL: https://github.com/apache/arrow/pull/13465#discussion_r910142254


##########
java/c/src/main/java/org/apache/arrow/c/ArrayStreamExporter.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.c;
+
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.nio.charset.StandardCharsets;
+
+import org.apache.arrow.c.jni.JniWrapper;
+import org.apache.arrow.c.jni.PrivateData;
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.vector.ipc.ArrowReader;
+import org.apache.arrow.vector.types.pojo.Schema;
+
+/**
+ * Utility to export an {@link ArrowReader} as an ArrowArrayStream.
+ */
+final class ArrayStreamExporter {
+  private final BufferAllocator allocator;
+
+  ArrayStreamExporter(BufferAllocator allocator) {
+    this.allocator = allocator;
+  }
+
+  /**
+   * Java-side state for the exported stream.
+   */
+  static class ExportedArrayStreamPrivateData implements PrivateData {
+    final BufferAllocator allocator;
+    final ArrowReader reader;
+    int nextDictionary;

Review Comment:
   This member doesn't seem used anymore, or am I missing something?



##########
java/c/src/main/cpp/jni_wrapper.cc:
##########
@@ -148,16 +176,99 @@ void release_exported(T* base) {
   // Mark released
   base->release = nullptr;
 }
+
+int ArrowArrayStreamGetSchema(ArrowArrayStream* stream, ArrowSchema* out) {
+  assert(stream->private_data != nullptr);
+  InnerPrivateData* private_data =
+      reinterpret_cast<InnerPrivateData*>(stream->private_data);
+  JNIEnvGuard guard(private_data->vm_);
+  JNIEnv* env = guard.env();
+
+  const long out_addr = static_cast<long>(reinterpret_cast<uintptr_t>(out));

Review Comment:
   Also, according to the [JNI spec](https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/types.html), a `jlong` is always 64 bits, so perhaps we should use `jlong` or simply `int64_t` here?



##########
java/c/src/main/cpp/jni_wrapper.cc:
##########
@@ -148,16 +176,99 @@ void release_exported(T* base) {
   // Mark released
   base->release = nullptr;
 }
+
+int ArrowArrayStreamGetSchema(ArrowArrayStream* stream, ArrowSchema* out) {
+  assert(stream->private_data != nullptr);
+  InnerPrivateData* private_data =
+      reinterpret_cast<InnerPrivateData*>(stream->private_data);
+  JNIEnvGuard guard(private_data->vm_);
+  JNIEnv* env = guard.env();
+
+  const long out_addr = static_cast<long>(reinterpret_cast<uintptr_t>(out));

Review Comment:
   I suppose this doesn't work on 64-bit Windows? `long` is 32 bits there...



##########
java/c/src/main/java/org/apache/arrow/c/Data.java:
##########
@@ -314,4 +325,14 @@ public static VectorSchemaRoot importVectorSchemaRoot(BufferAllocator allocator,
     }
     return vsr;
   }
+
+  /**
+   * Import an ArrowArrayStream as an {@link ArrowReader}.
+   * @param allocator Buffer allocator for allocating the output data.
+   * @param stream C stream interface struct to import.
+   * @return Imported reader
+   */
+  public static ArrowReader importStream(BufferAllocator allocator, ArrowArrayStream stream) {

Review Comment:
   Is there a reason for the naming discrepancy (`importStream` vs. `exportArrayStream`)?



##########
java/c/src/main/cpp/jni_wrapper.cc:
##########
@@ -54,23 +53,52 @@ void ThrowPendingException(const std::string& message) {
 
 void JniThrow(std::string message) { ThrowPendingException(message); }
 
+jclass CreateGlobalClassReference(JNIEnv* env, const char* class_name) {
+  jclass local_class = env->FindClass(class_name);
+  if (!local_class) {
+    std::string message = "Could not find class ";
+    message += class_name;
+    ThrowPendingException(message);
+  }
+  jclass global_class = (jclass)env->NewGlobalRef(local_class);
+  if (!local_class) {

Review Comment:
   Is this a mistake?
   ```suggestion
     if (!global_class) {
   ```



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org