parthchandra commented on code in PR #3753:
URL: https://github.com/apache/datafusion-comet/pull/3753#discussion_r3024543651
##########
native/core/src/execution/jni_api.rs:
##########
@@ -778,33 +778,31 @@ pub unsafe extern "system" fn
Java_org_apache_comet_Native_writeSortedFileNative
compression_level: jint,
tracing_enabled: jboolean,
) -> jlongArray {
- try_unwrap_or_throw(&e, |mut env| unsafe {
+ try_unwrap_or_throw(&e, |env| unsafe {
with_trace(
"writeSortedFileNative",
tracing_enabled != JNI_FALSE,
|| {
- let data_types = convert_datatype_arrays(&mut env,
serialized_datatypes)?;
+ let data_types = convert_datatype_arrays(env,
serialized_datatypes)?;
- let row_num = env.get_array_length(&row_addresses)? as usize;
- let row_addresses =
- env.get_array_elements(&row_addresses,
ReleaseMode::NoCopyBack)?;
+ let row_num = row_addresses.len(env)?;
+ let row_addresses = row_addresses.get_elements(env,
ReleaseMode::NoCopyBack)?;
- let row_sizes = env.get_array_elements(&row_sizes,
ReleaseMode::NoCopyBack)?;
+ let row_sizes = row_sizes.get_elements(env,
ReleaseMode::NoCopyBack)?;
let row_addresses_ptr = row_addresses.as_ptr();
let row_sizes_ptr = row_sizes.as_ptr();
- let output_path: String =
env.get_string(&file_path).unwrap().into();
+ let output_path: String =
file_path.try_to_string(env).unwrap();
- let checksum_enabled = checksum_enabled == 1;
Review Comment:
Not sure how removing this works. `checksum_enabled` (`jbool`) is passed in
as a parameter to this function and this line explicitly converts it to
`checksum_enabled` (`bool`) which is passed to `process_sorted_row_partition`.
I don't think `jbool` is implicitly cast to `bool` so I'm not sure how the
code compiles without this line.
##########
native/jni-bridge/src/lib.rs:
##########
@@ -281,27 +299,34 @@ impl JVMClasses<'_> {
unsafe { JVM_CLASSES.get_unchecked() }
}
- /// Gets the JNIEnv for the current thread.
- pub fn get_env() -> CometResult<AttachGuard<'static>> {
+ /// Runs a closure with an attached JNI environment for the current thread.
+ pub fn with_env<T, E, F>(f: F) -> Result<T, E>
Review Comment:
+1. Nice.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
