sunchao commented on code in PR #5602: URL: https://github.com/apache/datafusion-comet/pull/5602#discussion_r3910354329
########## native/jni-bridge/src/comet_schema_utils.rs: ########## @@ -0,0 +1,51 @@ +// 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. + +use jni::{ + errors::Result as JniResult, + objects::{JClass, JStaticMethodID}, + signature::ReturnType, + strings::JNIString, + Env, +}; + +/// JNI bindings for the JVM `org.apache.comet.CometSchemaUtils` helper. Used by the native +/// Parquet schema adapter to fold field names with the JVM's `String.toLowerCase(Locale.ROOT)`, +/// so case-insensitive field resolution matches Spark's `ParquetReadSupport` exactly. +pub struct CometSchemaUtils<'a> { + pub class: JClass<'a>, + pub method_to_lower_case_root: JStaticMethodID, + pub method_to_lower_case_root_ret: ReturnType, +} + +impl<'a> CometSchemaUtils<'a> { + pub const JVM_CLASS: &'static str = "org/apache/comet/CometSchemaUtils"; + + pub fn new(env: &mut Env<'a>) -> JniResult<CometSchemaUtils<'a>> { + let class = env.find_class(JNIString::new(Self::JVM_CLASS))?; Review Comment: [P1] Retain a global reference for the cached helper class Could this class be retained as a JNI global reference before the binding is stored in `JVMClasses`? `find_class` returns a local reference, but `JVMClasses::init` retains this binding process-wide by extending the `Env` lifetime. Once the initializing native call returns, a later field-name cache miss can call `toLowerCaseRoot` with an expired `jclass`. The name cache only hides this for names already encountered. An isolated probe using this exact constructor, JNI 0.22.4 and OpenJDK 21 successfully folded `A1Σ` inside the creation frame, then aborted under `-Xcheck:jni` when folding `Ω` after that frame closed: `Bad global or local ref passed to JNI`. The same probe retaining a global reference passed. Please keep that global reference alive with the cached binding and cover a call after the initialization frame closes. This is a focused JNI reproduction, not a complete Comet scan reproduction. -- 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]
