feat(java/driver/jni): add JNI bindings to native driver manager #2401
Conversation
lidavidm
force-pushed
the
gh-2027
branch
4 times, most recently
from
1abdf50 to
7df6826
Compare
java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/JniDriver.java
Outdated
Show resolved
Hide resolved
| import org.apache.arrow.adbc.core.AdbcException; | ||
| import org.apache.arrow.util.Preconditions; | ||
|
|
||
| public enum JniLoader { |
There was a problem hiding this comment.
(design) it's not a common pattern to use an enum class directly for creating a singleton, and not sure if there are benefits outside of it vs using a regular class.
I'm not even sure INSTANCE is needed since things are delegated to NativeAdbc anyway. It looks to me that NativeAdbc could simply have a static initializer invoking some JniLoader#checkInitialized() for example which would cause the library to be loaded or NativeAdbc class loading to fail otherwise
There was a problem hiding this comment.
This pattern is actually recommended by Effective Java. But I can remove the singleton, right.
There was a problem hiding this comment.
Maybe I should check my on copy then :)
There was a problem hiding this comment.
Ah, the problem is then the "nice" wrapper methods, if merged into NativeAdbc, can't use non-standard-library classes anymore, because of the circular dependency issue...
There was a problem hiding this comment.
For now I'm going to keep this as-is (note that Arrow itself uses the singleton pattern to gate access to JNI methods) and we can revisit if needed
java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/JniLoader.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| public NativeQueryResult statementExecuteQuery(NativeHandle statement) throws AdbcException { |
There was a problem hiding this comment.
It says AdbcException is being thrown but not sure it's true in practice...
| * Wrapper for a {@code struct AdbcException} that doesn't depend on other Java code. | ||
| * | ||
| * <p>This breaks a dependency chain in compilation. If we directly used AdbcException, then we | ||
| * would need to compile the Java code before compiling JNI code, but we need the compiled JNI code |
There was a problem hiding this comment.
this is surprising? I thought javah would use the source code, and that javac has no way to check native code? Is the circular dependency actually because of tests?
There was a problem hiding this comment.
The CMake code has to run javac to generate the JNI headers. And CMake doesn't resolve Maven dependencies. So it's easiest if these files can be compiled standalone.
There was a problem hiding this comment.
I can try to configure Maven to do this instead? It'd make the build a bit more complicated (run maven, then cmake, then maven) but maybe that's OK.
There was a problem hiding this comment.
Would invoking Maven from CMake to generate JNI headers address this?
Maybe using native-maven-plugin:javah goal?
There was a problem hiding this comment.
I could also do that. I sorta dislike invoking one build system from another as it makes it a bit more annoying to get in between them to debug or customize things but just generating headers should be OK.
java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/NativeQueryResult.java
Outdated
Show resolved
Hide resolved
Fixes #2027.