github-actions[bot] commented on code in PR #66247:
URL: https://github.com/apache/doris/pull/66247#discussion_r3679793467
##########
fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/vec/ColumnType.java:
##########
@@ -419,16 +431,27 @@ public static ColumnType parseType(String columnName,
String hiveType) {
} else if (lowerCaseType.startsWith("struct")) {
if (lowerCaseType.indexOf("<") == 6
&& lowerCaseType.lastIndexOf(">") ==
lowerCaseType.length() - 1) {
- String listFields = lowerCaseType.substring(7,
lowerCaseType.length() - 1);
+ String listFields = originalType.substring(7,
originalType.length() - 1);
Review Comment:
[P1] Preserve the legacy STRUCT-key normalization
`parseType(...)` still serves every unencoded `VectorTable` caller, but this
now recurses through `originalType` even when `encodedStructFields` is false.
Before this change, `struct<Mixed:int>` produced child key `mixed`; it now
produces `Mixed`. `VectorColumn.appendStruct` and `getStruct` use these names
as exact HashMap keys, including the Java UDF/UDAF path, so existing functions
that read or return the formerly normalized key can silently receive/write
NULL. Please use the original spelling only for the encoded Paimon grammar (or
introduce an explicit compatibility migration) and add an unencoded mixed-case
STRUCT test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java:
##########
@@ -0,0 +1,155 @@
+// 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.datasource.paimon;
+
+import com.google.common.collect.ImmutableSet;
+import org.apache.paimon.CoreOptions;
+import org.apache.paimon.options.ConfigOption;
+import org.apache.paimon.options.MemorySize;
+import org.apache.paimon.options.Options;
+import org.apache.paimon.table.FallbackReadFileStoreTable;
+import org.apache.paimon.table.Table;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+
+/** Validation shared by catalog-scoped and relation-scoped Paimon reader
tuning. */
+public final class PaimonReaderOptions {
+ public static final String TABLE_OPTION_PREFIX = "paimon.table-option.";
+ public static final int MIN_READ_BATCH_SIZE = 1;
+ public static final int MAX_READ_BATCH_SIZE = 65536;
+ public static final long MIN_ASYNC_THRESHOLD_BYTES = 1024L * 1024L;
+ public static final long MAX_ASYNC_THRESHOLD_BYTES = 1024L * 1024L * 1024L;
+
+ // Only options which cannot select another table context or mutate write
behavior are safe to
+ // apply with Table.copy after Doris has bound the table schema.
+ private static final Set<String> SUPPORTED_OPTIONS = ImmutableSet.of(
+ CoreOptions.READ_BATCH_SIZE.key(),
+ CoreOptions.FILE_READER_ASYNC_THRESHOLD.key());
+
+ private PaimonReaderOptions() {
+ }
+
+ public static Set<String> supportedOptions() {
+ return SUPPORTED_OPTIONS;
+ }
+
+ public static void validate(String key, String value) {
+ if (!SUPPORTED_OPTIONS.contains(key)) {
+ throw new IllegalArgumentException("Unsupported Paimon dynamic
reader option '" + key
+ + "'. Supported options are " + SUPPORTED_OPTIONS);
+ }
+
+ if (CoreOptions.READ_BATCH_SIZE.key().equals(key)) {
+ int batchSize = parse(key, value, CoreOptions.READ_BATCH_SIZE);
+ // A zero batch can make Paimon's vectorized reader report success
without
+ // advancing input; the upper bound also prevents one relation
from over-allocating.
+ requireRange(key, batchSize, MIN_READ_BATCH_SIZE,
MAX_READ_BATCH_SIZE);
+ } else {
+ MemorySize threshold = parse(key, value,
CoreOptions.FILE_READER_ASYNC_THRESHOLD);
+ // Bound the trigger on both sides so a query cannot fan out tiny
async reads or
+ // silently disable asynchronous reading with an effectively
infinite threshold.
+ requireRange(key, threshold.getBytes(),
+ MIN_ASYNC_THRESHOLD_BYTES, MAX_ASYNC_THRESHOLD_BYTES);
+ }
+ }
+
+ public static void validateCatalogProperties(Map<String, String>
properties) {
+ properties.forEach((key, value) -> {
+ if (!key.toLowerCase(Locale.ROOT).startsWith(TABLE_OPTION_PREFIX))
{
+ return;
+ }
+ String optionKey = key.substring(TABLE_OPTION_PREFIX.length());
+ if (optionKey.isEmpty()) {
+ throw new IllegalArgumentException(
+ "Paimon table option name must not be empty after
prefix " + TABLE_OPTION_PREFIX);
+ }
+ validate(optionKey, value);
+ });
+ }
+
+ public static Map<String, String> compatibleCatalogOptions(Map<String,
String> properties) {
+ Map<String, String> compatibleOptions = new LinkedHashMap<>();
+ properties.forEach((key, value) -> {
+ if (!key.toLowerCase(Locale.ROOT).startsWith(TABLE_OPTION_PREFIX))
{
+ return;
+ }
+ String optionKey = key.substring(TABLE_OPTION_PREFIX.length());
+ try {
+ validate(optionKey, value);
+ compatibleOptions.put(optionKey, value);
+ } catch (IllegalArgumentException ignored) {
+ // Images written before the reader-only allowlist may contain
arbitrary Paimon
+ // options. Keep the catalog loadable, but never apply an
unsafe legacy option.
+ }
+ });
+ return Collections.unmodifiableMap(compatibleOptions);
+ }
+
+ public static void validateEffectiveTableOptions(Map<String, String>
options) {
+ SUPPORTED_OPTIONS.stream()
+ .filter(options::containsKey)
+ .forEach(key -> validate(key, options.get(key)));
+
validateManifestParallelism(options.get(CoreOptions.SCAN_MANIFEST_PARALLELISM.key()));
+ }
+
+ public static void validateEffectiveTable(Table table) {
+ validateEffectiveTableOptions(table.options());
+ if (table instanceof FallbackReadFileStoreTable) {
Review Comment:
[P1] Unwrap privilege delegates before checking fallback children
With Paimon's privilege layer enabled, `PrivilegedCatalog.getTable` wraps
the `FallbackReadFileStoreTable` in `PrivilegedFileStoreTable`. This outer
object fails the `instanceof FallbackReadFileStoreTable` check; its `options()`
still exposes only the main child, while `newScan()` delegates to the wrapped
fallback planner. Consequently an unsafe physical fallback value remains
unvalidated despite the fix in the existing fallback thread. Please traverse
the supported delegated wrapper (`wrapped()`) before recursing into fallback
children and cover a privilege-wrapped fallback fixture.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java:
##########
@@ -135,19 +141,33 @@ public Table getSysPaimonTable() {
}
public Table getSysPaimonTable(TableScanParams scanParams) {
- Table table = getSysPaimonTable();
if (scanParams == null || !scanParams.isOptions()) {
- return table;
+ return getSysPaimonTable();
}
- Map<String, String> resolvedOptions = scanParams.getOrResolveMapParams(
- options ->
PaimonScanParams.resolveOptions(sourceTable.getBasePaimonTable(), options));
+ Map<String, String> resolvedOptions = resolvedOptions(scanParams);
+ validateEffectiveDataTable(scanParams);
if
(PaimonScanParams.getPinnedFileCreationTime(resolvedOptions).isPresent()) {
// Generic system-table wrappers cannot carry Paimon's
manifest-entry predicate.
// Reject the fallback instead of silently widening it to the
whole pinned snapshot.
throw new IllegalArgumentException(
"Paimon system tables cannot apply a creation-time file
filter.");
}
- return PaimonScanParams.applyOptions(table, resolvedOptions);
+ return PaimonScanParams.applyOptions(getRawSysPaimonTable(),
resolvedOptions);
+ }
+
+ public void validateEffectiveDataTable(TableScanParams scanParams) {
+ Table dataTable = sourceTable.getBasePaimonTable();
Review Comment:
[P1] Validate the exact data handle embedded in the system wrapper
`sourceTable.getBasePaimonTable()` comes from Doris' table cache, whereas
`getRawSysPaimonTable()` loads the system wrapper separately through the Paimon
catalog. If the source handle is still cached after Paimon's catalog cache
expires/is disabled and the physical options change, the wrapper can contain a
newer unsafe child while this method validates only the older safe handle. The
wrapper's `options()` is empty, and `$partitions` later plans its private child
directly, so the final generic guard cannot catch the mismatch. Please
construct the wrapper from the exact effective source handle or expose and
validate its actual child, and add a divergent-handle test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java:
##########
@@ -178,6 +178,14 @@ private PaimonSnapshotCacheValue
getPaimonSnapshotCacheValue(Optional<TableSnaps
"Failed to get Paimon snapshot: " + (e.getMessage() ==
null ? "unknown cause" : e.getMessage()),
e);
}
+ } else if (scanParams.isPresent() && scanParams.get().isOptions()) {
+ Table baseTable = getBasePaimonTable();
+ Map<String, String> resolvedOptions =
scanParams.get().getOrResolveMapParams(
+ options -> PaimonScanParams.resolveOptions(baseTable,
options));
+ Table effectiveTable = PaimonScanParams.applyOptions(baseTable,
resolvedOptions);
+ // The shared latest cache was built from the catalog-scoped
handle. Relation options
+ // need their own projection so partition enumeration uses the
final safe table copy.
+ return PaimonUtils.loadSnapshotProjection(this, effectiveTable);
Review Comment:
[P2] Keep reader-only OPTIONS on the memoized projection
This branch also handles `read.batch-size` and
`file-reader-async-threshold`, neither of which changes the selected snapshot,
schema, or partitions. Nevertheless every such relation calls the uncached
projection loader, whose partitioned path runs
`CatalogUtils.listPartitionsFromFileSystem`; the ordinary latest projection is
memoized in `PaimonTableCacheValue`. On a high-partition table this turns a
reader tuning option into a full metadata enumeration for every statement.
Please reuse the memoized latest projection after validating the effective
handle when the supplied options do not select startup metadata, and add a
partitioned counting test.
--
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]
