github-actions[bot] commented on code in PR #66018:
URL: https://github.com/apache/doris/pull/66018#discussion_r3654352948
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java:
##########
@@ -110,6 +109,9 @@ public PaimonJniScanner(int batchSize, Map<String, String>
params) {
}
paimonSplit = params.get("paimon_split");
paimonPredicate = params.get("paimon_predicate");
+ tableCacheKey = params.get("serialized_table_cache_key");
+ Preconditions.checkState(tableCacheKey != null &&
!tableCacheKey.isEmpty(),
Review Comment:
[P1] Preserve the old-FE fallback when this key is absent. Field 35 is
optional, and both C++ readers only forward it when `__isset`; during a
BE-first rolling upgrade an older FE still sends `serialized_table` but can
never send `serialized_table_cache_key`. This precondition therefore makes
every such Paimon JNI scanner fail in its constructor instead of using the
previous uncached path. Please let a missing/empty key bypass the cache, and
cover that request shape through both V1 and V2 parameter builders.
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java:
##########
@@ -571,26 +573,44 @@ static Optional<Long> parseDataSizeBytes(String value) {
private void initTable() {
Preconditions.checkState(params.containsKey("serialized_table"));
table = PaimonUtils.deserialize(params.get("serialized_table"));
- table = table.copy(buildTableOptions(table.options()));
paimonAllFieldNames = PaimonUtils.getFieldNames(this.table.rowType());
if (LOG.isDebugEnabled()) {
LOG.debug("paimonAllFieldNames:{}", paimonAllFieldNames);
}
}
+ private void initTableAndReader() throws IOException {
+ PaimonTableCache.TableCacheEntry cachedEntry =
PaimonTableCache.acquire(tableCacheKey);
+ if (cachedEntry != null) {
+ // get from cache
+ tableCacheEntry = cachedEntry;
+ table = cachedEntry.table();
+ paimonAllFieldNames = cachedEntry.fieldNames();
+ initReader();
+ return;
+ }
+ // no cache deserialize by self
+ initTable();
Review Comment:
[P1] Make the cold load single-flight before deserializing. If several split
scanners reach this miss together, every one runs `PaimonUtils.deserialize()`
and `initReader()` before any entry is published; `putIfAbsent` only discards
candidates after the peak allocation has already happened, and each loser keeps
its private table for the split. That leaves the O(concurrent scanners) memory
spike this PR is meant to remove. Install a per-key loading reservation/future
first (with failure removal/retry), and add a barrier test proving one
deserialize for a cold key.
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java:
##########
@@ -571,26 +573,44 @@ static Optional<Long> parseDataSizeBytes(String value) {
private void initTable() {
Preconditions.checkState(params.containsKey("serialized_table"));
table = PaimonUtils.deserialize(params.get("serialized_table"));
Review Comment:
[P1] Preserve the per-scanner dynamic options on top of the cached base
table. The removed `table.copy(buildTableOptions(...))` was the only place that
set Paimon's physical `read.batch-size` to the Doris scanner size and converted
`paimon.jni.enable_file_reader_async=false` into the disabling async threshold.
FE/C++ still send that flag, but this raw table now reaches `newReadBuilder()`
unchanged, silently re-enabling async reads and decoupling Paimon's physical
read batches from the Doris scanner size. Cache the deserialized base, then
create a cheap scanner-local copy with those options on both hit and miss
paths, with option application tests.
##########
fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonTableCache.java:
##########
@@ -0,0 +1,82 @@
+// 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.paimon;
+
+import com.google.common.base.Preconditions;
+import org.apache.paimon.table.Table;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.ConcurrentHashMap;
+
+final class PaimonTableCache {
+ private static final ConcurrentHashMap<String, TableCacheEntry>
TABLE_CACHE = new ConcurrentHashMap<>();
+
+ private PaimonTableCache() {
+ }
+
+ static TableCacheEntry acquire(String cacheKey) {
+ return TABLE_CACHE.computeIfPresent(cacheKey, (key, entry) -> {
+ entry.users++;
+ return entry;
+ });
+ }
+
+ static boolean publish(String cacheKey, TableCacheEntry entry) {
+ return TABLE_CACHE.putIfAbsent(cacheKey, entry) == null;
+ }
+
+ static void release(String cacheKey, TableCacheEntry expectedEntry) {
+ TABLE_CACHE.compute(cacheKey, (key, currentEntry) -> {
+ Preconditions.checkState(currentEntry == expectedEntry,
+ "Paimon table cache entry changed unexpectedly for key
%s", cacheKey);
+ Preconditions.checkState(currentEntry.users > 0,
+ "Paimon table cache reference count is invalid for key
%s", cacheKey);
+ currentEntry.users--;
+ return currentEntry.users == 0 ? null : currentEntry;
Review Comment:
[P1] Keep the entry alive across split boundaries. Both scanner paths close
the Java scanner for the current split before opening the next one (V1 before
fetching the next range; V2 at split EOF), so with one scan worker—or any gap
between concurrent waves—this zero-user removal makes the next split miss and
deserialize the same table again. The stated many-split case therefore gets no
cache benefit in a common execution shape. Tie eviction to the scan-node
lifetime, or use bounded post-close retention, and test close-then-open
sequential reuse plus bounded eviction.
--
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]
