github-actions[bot] commented on code in PR #61513:
URL: https://github.com/apache/doris/pull/61513#discussion_r2977947331
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -258,6 +261,9 @@ private void setPaimonParams(TFileRangeDesc rangeDesc,
PaimonSplit paimonSplit)
fileDesc.setSchemaId(paimonSplit.getSchemaId());
}
fileDesc.setFileFormat(fileFormat);
+ if (!backendPaimonOptions.isEmpty()) {
+ fileDesc.setPaimonOptions(backendPaimonOptions);
Review Comment:
**Per-split redundancy**: `backendPaimonOptions` contains the same JDBC
driver URL and class for every split, yet it's set per-split in
`TPaimonFileDesc.paimon_options`. This is inconsistent with the existing
pattern — see the comment at line 267-268 ("Hadoop conf is set at ScanNode
level via params.properties in createScanRangeLocations(), no need to set it
for each split to avoid redundant configuration") and line 205-207 where
`paimon_predicate` is also set at the scan-node level.
For queries with many splits, this means the same two key-value pairs are
serialized redundantly in every `TFileRangeDesc`. Consider setting these
options at the scan-node level (e.g., via `params.properties` or a dedicated
field in `TFileScanRangeParams`) instead.
##########
fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jdbc/JdbcDriverUtils.java:
##########
@@ -0,0 +1,114 @@
+// 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.
+
+package org.apache.doris.common.jdbc;
+
+import org.apache.doris.common.classloader.JniScannerClassLoader;
+
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.sql.Connection;
+import java.sql.Driver;
+import java.sql.DriverManager;
+import java.sql.DriverPropertyInfo;
+import java.sql.SQLFeatureNotSupportedException;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+public final class JdbcDriverUtils {
+ private static final ConcurrentHashMap<URL, ClassLoader>
DRIVER_CLASS_LOADER_CACHE = new ConcurrentHashMap<>();
+ private static final Set<String> REGISTERED_DRIVER_KEYS =
ConcurrentHashMap.newKeySet();
+
+ private JdbcDriverUtils() {
+ }
+
+ public static void registerDriver(String driverUrl, String
driverClassName, ClassLoader classLoader) {
+ try {
+ URL url = new URL(driverUrl);
+ String driverKey = driverUrl + "#" + driverClassName;
+ if (!REGISTERED_DRIVER_KEYS.add(driverKey)) {
+ return;
+ }
+ try {
+ ClassLoader driverClassLoader = prepareDriverClassLoader(url,
classLoader);
+ Class<?> loadedDriverClass = Class.forName(driverClassName,
true, driverClassLoader);
+ Driver driver = (Driver)
loadedDriverClass.getDeclaredConstructor().newInstance();
+ DriverManager.registerDriver(new DriverShim(driver));
+ } catch (Exception e) {
+ REGISTERED_DRIVER_KEYS.remove(driverKey);
+ throw new RuntimeException("Failed to register JDBC driver: "
+ driverClassName, e);
+ }
+ } catch (MalformedURLException e) {
+ throw new IllegalArgumentException("Invalid JDBC driver URL: " +
driverUrl, e);
+ }
+ }
+
+ private static ClassLoader prepareDriverClassLoader(URL driverUrl,
ClassLoader classLoader) {
+ if (classLoader instanceof JniScannerClassLoader) {
+ JniScannerClassLoader scannerClassLoader = (JniScannerClassLoader)
classLoader;
+ scannerClassLoader.addURLIfAbsent(driverUrl);
+ return scannerClassLoader;
+ }
+ return DRIVER_CLASS_LOADER_CACHE.computeIfAbsent(driverUrl,
+ url -> URLClassLoader.newInstance(new URL[] {url},
classLoader));
+ }
+
+ private static final class DriverShim implements Driver {
+ private final Driver delegate;
Review Comment:
**Code duplication**: This `DriverShim` class is the third identical copy in
the codebase — the other two are:
- `PaimonJdbcMetaStoreProperties.DriverShim` (FE side, line 222)
- `IcebergJdbcMetaStoreProperties.DriverShim` (FE side, line 191)
Since `JdbcDriverUtils` was explicitly created as a shared utility class,
consider also consolidating the FE-side copies to reduce maintenance burden.
(The FE copies live in a different classloader context, but the `DriverShim`
class itself is trivially shareable — it's a pure delegation wrapper with no
classloader-specific behavior.)
This is a minor improvement suggestion, not a blocking issue.
##########
be/src/gen_cpp/CMakeLists.txt:
##########
@@ -23,6 +23,7 @@ file(GLOB SRC_FILES CONFIGURE_DEPENDS
${GEN_CPP_DIR}/*.cpp
${GEN_CPP_DIR}/*.cc
)
+list(FILTER SRC_FILES EXCLUDE REGEX "_server\\.skeleton\\.cpp$")
Review Comment:
This change (skeleton filter + trailing blank line removal) is unrelated to
the Paimon JDBC driver fix. It was introduced by a different commit (JuiceFS
compatibility). Per the commit standards in AGENTS.md: "Files in git commit
should only be related to the current modification task." This should be in a
separate PR or excluded from this one.
--
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]
