Gabriel39 commented on code in PR #66247:
URL: https://github.com/apache/doris/pull/66247#discussion_r3679847280
##########
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:
Fixed in 43f91409ddc. The unencoded branch now lowercases each STRUCT field
name before building child types and maps, while the versioned encoded branch
continues to Base64-decode and preserve the exact UTF-8 spelling. Added a
nested mixed-case legacy test; its RED result was `[Mixed, UPPER]`, and the
targeted GREEN run now returns `[mixed, upper]`. The 15 encoded Paimon scanner
tests also pass.
##########
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:
Fixed in 43f91409ddc. `validateEffectiveTable` now traverses every
`DelegatedFileStoreTable.wrapped()` handle in addition to the fallback child,
so `PrivilegedFileStoreTable -> FallbackReadFileStoreTable -> fallback` is
fully validated after copies. Added a real privilege-wrapped Paimon fixture.
The standalone 1.3.1 probe accepted the unsafe hidden child before the fix and
rejects it now.
##########
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:
Fixed in 43f91409ddc. `PaimonSysExternalTable` no longer reloads the wrapper
through the catalog. It caches the source `FileStoreTable` handle and
constructs every raw/effective wrapper from that same handle with Paimon's
`SystemTableLoader`; relation options are applied to the data handle before
wrapper construction. Added a divergent-handle test where the source is safe
and a separately mocked catalog reload is unsafe; the catalog reload must never
be called or returned.
##########
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:
Fixed in 43f91409ddc. After applying and validating relation options, pure
`read.batch-size` / `file-reader-async-threshold` tuning now returns the
memoized latest projection. Manifest and startup-sensitive options continue
through the effective-handle projection loader. Added a real partitioned table
fixture with a direct-projection counter that must remain zero for reader-only
tuning; the existing manifest override test continues to require the direct
path.
--
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]
