github-actions[bot] commented on code in PR #65404:
URL: https://github.com/apache/doris/pull/65404#discussion_r3550626291
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java:
##########
@@ -132,7 +133,8 @@ public void open() throws IOException {
preExecutionAuthenticator.execute(() -> {
PaimonJdbcDriverUtils.registerDriverIfNeeded(params,
classLoader);
Review Comment:
This breaks the legacy Paimon JNI reader path. `open()` no longer builds a
`reader` from the constructor's `paimon_split`; it only initializes the
table/projection and leaves `reader` null until `prepareForSplit()` runs.
Format-v2 calls that new method, but the existing
`be/src/format/table/paimon_jni_reader.cpp` path still passes `paimon_split` in
the constructor params and then uses `JniReader::open()`/`getNextBatchMeta()`
directly, with no `prepareForSplit` call. That path is still reachable whenever
`enable_file_scanner_v2=false` (and for paths where v2 is not selected), so the
first read hits `Preconditions.checkState(reader != null, "Paimon split is not
prepared")` and the scan fails. Please keep backward compatibility by either
preparing the constructor-provided split in `open()` when present, or teaching
the legacy C++ reader to invoke `prepareForSplit` before reading.
##########
be/src/format_v2/jni/paimon_jni_reader.cpp:
##########
@@ -125,6 +124,57 @@ Status
PaimonJniReader::build_scanner_params(std::map<std::string, std::string>*
return Status::OK();
}
+Status PaimonJniReader::open_jni_scanner_for_split() {
+ if (!jni_scanner_opened()) {
+ RETURN_IF_ERROR(JniTableReader::open_jni_scanner_for_split());
+ }
+ return _prepare_for_split();
+}
+
+Status PaimonJniReader::close_jni_scanner_for_split() {
+ RETURN_IF_ERROR(_reset_current_split());
+ reset_jni_eof();
+ return Status::OK();
+}
+
+Status PaimonJniReader::_prepare_for_split() {
+ DORIS_CHECK(jni_scanner_opened());
+ DORIS_CHECK(!_current_split_prepared);
+ std::map<std::string, std::string> split_params;
+ split_params["paimon_split"] =
_current_range.table_format_params.paimon_params.paimon_split;
+
+ JNIEnv* env = nullptr;
+ RETURN_IF_ERROR(Jni::Env::Get(&env));
+ Jni::LocalObject hashmap_object;
+ RETURN_IF_ERROR(Jni::Util::convert_to_java_map(env, split_params,
&hashmap_object));
+ RETURN_IF_ERROR(jni_scanner_obj()
+ .call_void_method(env,
jni_scanner_prepare_for_split())
+ .with_arg(hashmap_object)
+ .call());
+ RETURN_ERROR_IF_EXC(env);
+ _current_split_prepared = true;
+ reset_jni_eof();
+ return Status::OK();
+}
+
+Status PaimonJniReader::close() {
Review Comment:
`close()` should still run the base JNI cleanup even if resetting the
current split reports an error. `resetCurrentSplit()` can throw when releasing
an active batch or closing the Paimon `reader`, and Java `close()` is
explicitly written to keep going with IOManager cleanup/metric accounting after
that reset error. This C++ override returns immediately, so
`JniTableReader::close()` never reaches the idempotent `releaseTable()` or Java
`close()` call, and `FileScannerV2::close()` has already marked the scanner
closed so it will not retry. Please preserve the first reset error but still
attempt `JniTableReader::close()` before returning.
--
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]
