fix(java/driver/jni): handle "modified UTF-8" encoding - #4423
Conversation
|
THIS IS STACKED ON #4398. When reviewing, manually select the relevant commit. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Java JNI ADBC driver to avoid Java’s “modified UTF-8” string encoding pitfalls by passing explicit UTF-8 byte arrays across the JNI boundary, ensuring correct handling of non-BMP characters (e.g., emoji) when executing SQL and manipulating options.
Changes:
- Switch JNI method signatures from
String/String[]tobyte[]/byte[][]for parameters, SQL queries, and option keys/values. - Add UTF-8 encode/decode helpers in
JniLoaderto keep the public Java-facing API asStringwhile using bytes in JNI. - Add a regression test that queries a non-BMP character to validate correct round-tripping.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| java/driver/jni/src/test/java/org/apache/arrow/adbc/driver/jni/JniDriverTest.java | Adds a regression test for non-BMP UTF-8 query round-trip. |
| java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/NativeAdbc.java | Updates native method signatures to use byte[]/byte[][] for UTF-8 transfer. |
| java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/JniLoader.java | Encodes Java String→UTF-8 bytes before JNI calls and decodes bytes→String on return. |
| java/driver/jni/src/main/cpp/jni_wrapper.cc | Implements byte-array based JNI marshaling and removes GetStringUTFChars usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
just the one question which is likely due to my lack of java/jni knowledge and I just want to understand. But otherwise this LGTM
| auto WithJniString(JNIEnv* env, jstring jni_string, Callable&& callable) { | ||
| JniStringView view(env, jni_string); | ||
| return callable(view.value); | ||
| jbyteArray MakeJniUtf8String(JNIEnv* env, const char* value, size_t length) { |
There was a problem hiding this comment.
jstring isn't utf8 and can't be utf8? Just wondering why we're using jbyteArray instead of jstring for utf8 strings. Why do we need to have the intermediate jbyteArray and then call GetJniUtf8String instead of having this go straight to it? (there's probably some jni thing I'm not aware of which is the reason)
There was a problem hiding this comment.
The whole point of this is that jstring is not UTF-8, yes.
Assisted-by: GPT-5.5 <codex@openai.com>
Assisted-by: GPT-5.5 codex@openai.com