[refactor](jni)refactor jni util for safe jni call #56763
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
run buildall |
hubgeter
force-pushed
the
jni_util_factor
branch
2 times, most recently
from
39b5627 to
0f97a59
Compare
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
ClickBench: Total hot run time: 30.94 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the JNI utility code in Apache Doris, introducing a new type-safe C++ wrapper API around JNI operations. The changes replace raw JNI API calls with a templated, RAII-based approach using Jni::Object, Jni::Class, Jni::String, and Jni::Array template classes.
Key changes:
- New JNI wrapper framework with automatic reference management (Local vs Global refs)
- Replaces raw
jobject,jclass,jmethodIDwith type-safe wrappers - Adds comprehensive test suite for JNI utility functions
- Updates all existing JNI call sites across the codebase to use new API
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| be/src/util/jni-util.h | Major refactor introducing templated JNI wrapper classes with RAII semantics |
| be/src/util/jni-util.cpp | Implementation of new JNI utilities and namespace reorganization |
| be/test/util/jni_util_test.cpp | New comprehensive test suite for JNI operations |
| be/src/vec/exec/vjdbc_connector.cpp | Migration from raw JNI to new wrapper API |
| be/src/vec/exec/jni_connector.cpp | Migration from raw JNI to new wrapper API |
| be/src/vec/functions/function_java_udf.cpp | Migration from raw JNI to new wrapper API |
| be/src/util/jvm_metrics.cpp | Migration from raw JNI to new wrapper API |
| be/src/service/doris_main.cpp | Update to new namespace Jni::Util::Init() |
Comments suppressed due to low confidence (1)
be/src/vec/exec/vjdbc_connector.cpp:1
- Commented-out old code at lines 110, 275 should be removed.
// Licensed to the Apache Software Foundation (ASF) under one
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
be/test/util/jni_util_test.cpp
Outdated
| ASSERT_STREQ(buf2.get(), "hello"); | ||
| } | ||
|
|
||
| // 6. substring() 测试 |
There was a problem hiding this comment.
Chinese comment in English codebase. Should be: '// 6. substring() test'
Suggested change
| // 6. substring() 测试 | |
| // 6. substring() test |
be/src/vec/exec/vjdbc_connector.cpp
Outdated
Comment on lines
83
to
95
| // RETURN_IF_ERROR | ||
| RETURN_IF_ERROR( | ||
| _executor_obj.call_nonvirtual_void_method(env, _executor_clazz, _executor_close_id) | ||
| .call()); | ||
|
|
||
| // env->CallNonvirtualVoidMethod(_executor_obj, _executor_clazz, _executor_close_id); todo | ||
| // RETURN_ERROR_IF_EXC(env); | ||
| // env->DeleteGlobalRef(_executor_factory_clazz); | ||
| // RETURN_ERROR_IF_EXC(env); | ||
| // env->DeleteGlobalRef(_executor_clazz); | ||
| // RETURN_IF_ERROR(JniUtil::GetJniExceptionMsg(env)); | ||
| // env->DeleteGlobalRef(_executor_obj); | ||
| // RETURN_ERROR_IF_EXC(env); |
There was a problem hiding this comment.
Commented-out old code should be removed. These lines are unnecessary and reduce code clarity.
Suggested change
| // RETURN_IF_ERROR | |
| RETURN_IF_ERROR( | |
| _executor_obj.call_nonvirtual_void_method(env, _executor_clazz, _executor_close_id) | |
| .call()); | |
| // env->CallNonvirtualVoidMethod(_executor_obj, _executor_clazz, _executor_close_id); todo | |
| // RETURN_ERROR_IF_EXC(env); | |
| // env->DeleteGlobalRef(_executor_factory_clazz); | |
| // RETURN_ERROR_IF_EXC(env); | |
| // env->DeleteGlobalRef(_executor_clazz); | |
| // RETURN_IF_ERROR(JniUtil::GetJniExceptionMsg(env)); | |
| // env->DeleteGlobalRef(_executor_obj); | |
| // RETURN_ERROR_IF_EXC(env); | |
| RETURN_IF_ERROR( | |
| _executor_obj.call_nonvirtual_void_method(env, _executor_clazz, _executor_close_id) | |
| .call()); |
be/src/vec/exec/vjdbc_connector.cpp
Outdated
| , _executor_factory_ctor_id, env, | ||
| GetStaticMethodID(_executor_factory_clazz, "getExecutorClass", | ||
| "(Lorg/apache/doris/thrift/TOdbcTableType;)Ljava/lang/String;")); | ||
| // todo xxx |
There was a problem hiding this comment.
Incomplete TODO comment. Either complete the TODO or remove it if not needed.
Suggested change
| // todo xxx |
| &_executor_factory_clazz)); | ||
|
|
||
| jobject jtable_type = nullptr; | ||
| RETURN_IF_ERROR(_get_java_table_type(env, _conn_param.table_type, &jtable_type)); | ||
| RETURN_IF_ERROR(_executor_factory_clazz.get_static_method( |
There was a problem hiding this comment.
Commented-out old code at lines 187-189 and 203-205 should be removed to improve code readability.
be/src/vec/exec/vjdbc_connector.cpp
Outdated
| env->DeleteLocalRef(java_enum_local_obj); | ||
| return Status::OK(); | ||
| Jni::LocalObject* java_enum_obj) { | ||
| Jni::LocalClass enum_class; // todo global |
There was a problem hiding this comment.
Incomplete TODO comment. If enum_class should be Global instead of Local, either fix it or provide more context about why it remains Local.
Suggested change
| Jni::LocalClass enum_class; // todo global | |
| Jni::LocalClass enum_class; // Local reference is sufficient as enum_class is only used within this method. |
|
|
||
| jobject input_map = nullptr; | ||
| RETURN_IF_ERROR(JniUtil::convert_to_java_map(env, input_params, &input_map)); | ||
| // jobject input_map = nullptr; |
There was a problem hiding this comment.
Commented-out old code should be removed for cleaner codebase.
Suggested change
| // jobject input_map = nullptr; |
be/src/util/jvm_metrics.cpp
Outdated
Comment on lines
401
to
402
| // auto = (jlongArray)threadIdsObject; | ||
| // Jni::LocalArray threadIds; |
There was a problem hiding this comment.
Commented-out old code at lines 401-402, 414-416 should be removed.
be/src/util/jvm_metrics.cpp
Outdated
|
|
||
| //IsSameObject not throw exception | ||
| if (env->IsSameObject(threadState, _newThreadStateObj)) { | ||
| //IsSameObject not throw exception todo ?? |
There was a problem hiding this comment.
Unclear TODO comment. Clarify what needs to be verified about IsSameObject exception behavior or remove if confirmed.
Suggested change
| //IsSameObject not throw exception todo ?? |
be/src/util/jvm_metrics.cpp
Outdated
| const char* gcNameStr = env->GetStringUTFChars((jstring)gcName, NULL); | ||
| if (gcNameStr != nullptr) { | ||
| if (strcmp(gcNameStr, "G1 Young Generation") == 0) { | ||
| if (gcNameStr.get() != nullptr) { // todo nulllptr in get_string_chars ?? |
There was a problem hiding this comment.
Typo in comment: 'nulllptr' should be 'nullptr'. Also, clarify the TODO about when get_string_chars might return nullptr.
Suggested change
| if (gcNameStr.get() != nullptr) { // todo nulllptr in get_string_chars ?? | |
| if (gcNameStr.get() != nullptr) { // TODO: get_string_chars may return nullptr if the Java string is null or if a JNI error occurs. |
zhangstar333
reviewed
Nov 6, 2025
hubgeter
force-pushed
the
jni_util_factor
branch
from
8d02507 to
c33c840
Compare
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 34345 ms |
TPC-DS: Total hot run time: 188583 ms |
ClickBench: Total hot run time: 27.81 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
be/src/util/jni-util.h
Outdated
| JNIEnv* env = nullptr; | ||
| RETURN_IF_ERROR(Jni::Env::Get(&env)); | ||
|
|
||
| if (env == nullptr) { |
Contributor
There was a problem hiding this comment.
this seems have checked in Get() function
be/src/util/jni-util.h
Outdated
| } | ||
| return Status::JniError("Failed to create global reference to JniUtil class."); | ||
| } | ||
| env->DeleteLocalRef(local_jni_util_cl); |
Contributor
There was a problem hiding this comment.
seems before L95, also need delete local ref
| IntMethod, | ||
| LongMethod, | ||
| VoidMethod, | ||
| BooleanMethod, |
Contributor
There was a problem hiding this comment.
jboolean z;
jbyte b;
jchar c;
jshort s;
jint i;
jlong j;
jfloat f;
jdouble d;
jobject l;
those all of types, maybe could add those type once
Contributor
Author
There was a problem hiding this comment.
I think the additions so far are sufficient for the vast majority of scenarios, and there's no need to add other tag that's never been used.
| } else if constexpr (std::is_same_v<return_type, jobject>) { | ||
| jobject tmp = CallHelper<tag>::call_impl(_env, _base, _method, _args.data()); | ||
| RETURN_ERROR_IF_EXC(_env); | ||
| _env->DeleteLocalRef(tmp); |
Contributor
There was a problem hiding this comment.
maybe could call delete first
_env->DeleteLocalRef(tmp);
RETURN_ERROR_IF_EXC(_env);
be/src/util/jni-util.h
Outdated
| template <CallTag tag> | ||
| friend class NonvirtaulFunctionCall; | ||
|
|
||
| ~Object() { |
Contributor
There was a problem hiding this comment.
here do we need add virtual key word?
| JNIEnv* env = nullptr; | ||
| if (Status st = RefHelper<Ref>::get_env(&env); !st.ok()) [[unlikely]] { | ||
| LOG(WARNING) << "Can't destroy Jni Ref : " << st.msg(); | ||
| return; |
Contributor
There was a problem hiding this comment.
here if return directly, and no call destroy fucntion,
the ref seems will be leak?
Contributor
Author
There was a problem hiding this comment.
Yes, because destroy requires an environment variable (env). However, this situation is unlikely to occur, as the environment variable is used when the object is created.
be/src/util/jni-util.h
Outdated
| }; | ||
|
|
||
| template <CallTag tag> | ||
| class NonvirtaulFunctionCall : public FunctionCall<tag> { |
Contributor
There was a problem hiding this comment.
Suggested change
| class NonvirtaulFunctionCall : public FunctionCall<tag> { | |
| class NonvirtualFunctionCall : public FunctionCall<tag> { |
morningman
force-pushed
the
jni_util_factor
branch
from
c33c840 to
631a5af
Compare
Contributor
|
run buildall |
1 similar comment
Contributor
|
run buildall |
TPC-H: Total hot run time: 36639 ms |
TPC-DS: Total hot run time: 178725 ms |
ClickBench: Total hot run time: 27.45 s |
hubgeter
commented
Dec 17, 2025
| IntMethod, | ||
| LongMethod, | ||
| VoidMethod, | ||
| BooleanMethod, |
Contributor
Author
There was a problem hiding this comment.
I think the additions so far are sufficient for the vast majority of scenarios, and there's no need to add other tag that's never been used.
| } else if constexpr (std::is_same_v<return_type, jobject>) { | ||
| jobject tmp = CallHelper<tag>::call_impl(_env, _base, _method, _args.data()); | ||
| RETURN_ERROR_IF_EXC(_env); | ||
| _env->DeleteLocalRef(tmp); |
| JNIEnv* env = nullptr; | ||
| if (Status st = RefHelper<Ref>::get_env(&env); !st.ok()) [[unlikely]] { | ||
| LOG(WARNING) << "Can't destroy Jni Ref : " << st.msg(); | ||
| return; |
Contributor
Author
There was a problem hiding this comment.
Yes, because destroy requires an environment variable (env). However, this situation is unlikely to occur, as the environment variable is used when the object is created.
Contributor
Author
|
run buildall |
morningman
force-pushed
the
jni_util_factor
branch
from
cd5b3db to
6e22800
Compare
Contributor
|
run buildall |
hubgeter
force-pushed
the
jni_util_factor
branch
from
6e22800 to
1e80f87
Compare
Contributor
|
run buildall |
TPC-H: Total hot run time: 35564 ms |
TPC-DS: Total hot run time: 173805 ms |
ClickBench: Total hot run time: 27.12 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
morningman
force-pushed
the
jni_util_factor
branch
from
1e80f87 to
c755d68
Compare
Contributor
|
run buildall |
TPC-H: Total hot run time: 35726 ms |
TPC-DS: Total hot run time: 175623 ms |
ClickBench: Total hot run time: 26.87 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
suxiaogang223
approved these changes
Jan 4, 2026
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
Author
Sorry, I didn't quite understand. |
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
bot
added
the
approved
zclllyybb
approved these changes
Jan 4, 2026
|
please delete all the scripts associated with my email acount [email protected] thank you |
1 similar comment
|
please delete all the scripts associated with my email acount [email protected] thank you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
This PR significantly refactors the JNI util class.
It introduces a safer way to operate JNI, including automatic reference deletion and exception checking...
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)