From 8b515efade9e8d84cb286c834e6708e0c65647e4 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 29 Jul 2026 23:32:44 +0800 Subject: [PATCH 1/6] [fix](paimon) Harden JNI reader options and transport ### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Paimon dynamic reader settings accepted unsafe or unbounded values, query-scoped settings could resize a JVM-global executor, and failed catalog validation could leave tentative properties installed. JNI projection framing also split quoted column names containing commas, while debug logging and scanner statistics could expose raw credentials or repeatedly scan every JVM thread. This change introduces a reader-only option allowlist and bounded validation, keeps catalog and relation precedence explicit, rolls back every catalog validation failure, and transports projected identifiers with per-field Base64 framing. It also replaces raw parameter logging with a fixed safe summary and shares JVM thread-count samples for one second. ### Release note Paimon catalogs and relation @options support bounded read.batch-size and file-reader-async-threshold tuning. Unsafe catalog table options are rejected, manifest planning parallelism is bounded by JVM processor capacity, and JNI projection now supports quoted identifiers containing delimiters. ### Check List (For Author) - Test - Unit Test - Regression test - Behavior changed: - Paimon dynamic reader settings now use an explicit safe allowlist and bounds. - Does this need documentation? - Yes. This PR documents the supported query options in its description. --- be/src/format_v2/jni/jni_table_reader.cpp | 15 ++ be/src/format_v2/jni/jni_table_reader.h | 2 + .../format_v2/jni/jni_table_reader_test.cpp | 5 + .../apache/doris/paimon/PaimonJniScanner.java | 72 ++++++++- .../doris/paimon/PaimonJniScannerTest.java | 94 +++++++++++ .../apache/doris/datasource/CatalogMgr.java | 10 +- .../paimon/PaimonExternalCatalog.java | 3 +- .../paimon/PaimonReaderOptions.java | 84 ++++++++++ .../datasource/paimon/PaimonScanParams.java | 52 +++++-- .../metastore/AbstractPaimonProperties.java | 60 +------ .../doris/datasource/CatalogMgrTest.java | 66 ++++++++ .../paimon/PaimonScanParamsTest.java | 47 ++++++ .../AbstractPaimonPropertiesTest.java | 72 ++++++--- .../test_paimon_jni_reader_guardrails.groovy | 147 ++++++++++++++++++ 14 files changed, 635 insertions(+), 94 deletions(-) create mode 100644 fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java create mode 100644 fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogMgrTest.java create mode 100644 regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy diff --git a/be/src/format_v2/jni/jni_table_reader.cpp b/be/src/format_v2/jni/jni_table_reader.cpp index 1c691fc3129c95..f88cf6132a3c7d 100644 --- a/be/src/format_v2/jni/jni_table_reader.cpp +++ b/be/src/format_v2/jni/jni_table_reader.cpp @@ -27,9 +27,21 @@ #include "runtime/file_scan_profile.h" #include "runtime/runtime_state.h" #include "util/string_util.h" +#include "util/url_coding.h" namespace doris::format { +std::string encode_jni_required_fields(const std::vector& required_fields) { + std::vector encoded_fields; + encoded_fields.reserve(required_fields.size()); + for (const auto& field : required_fields) { + std::string encoded; + base64_encode(field, &encoded); + encoded_fields.emplace_back(std::move(encoded)); + } + return join(encoded_fields, ","); +} + Status JniTableReader::init(TableReadOptions&& options) { RETURN_IF_ERROR(TableReader::init(std::move(options))); { @@ -517,6 +529,9 @@ void JniTableReader::_prepare_jni_scanner_schema() { {column.transfer_type->create_column(), column.transfer_type, column.java_name}); } _scanner_params["required_fields"] = join(required_fields, ","); + // Preserve the legacy field during rolling upgrades, while new scanners use independently + // encoded identifiers so commas and other legal identifier characters cannot alter framing. + _scanner_params["required_fields_base64"] = encode_jni_required_fields(required_fields); _scanner_params["columns_types"] = join(column_types, "#"); if (has_replace_type) { _scanner_params["replace_string"] = join(replace_types, ","); diff --git a/be/src/format_v2/jni/jni_table_reader.h b/be/src/format_v2/jni/jni_table_reader.h index 5e48270515f996..c9bfa038b6a709 100644 --- a/be/src/format_v2/jni/jni_table_reader.h +++ b/be/src/format_v2/jni/jni_table_reader.h @@ -30,6 +30,8 @@ namespace doris::format { +std::string encode_jni_required_fields(const std::vector& required_fields); + class JniTableReader : public TableReader { public: struct JniColumn { diff --git a/be/test/format_v2/jni/jni_table_reader_test.cpp b/be/test/format_v2/jni/jni_table_reader_test.cpp index 95eda4a557e790..2c9e066bc3213a 100644 --- a/be/test/format_v2/jni/jni_table_reader_test.cpp +++ b/be/test/format_v2/jni/jni_table_reader_test.cpp @@ -109,6 +109,11 @@ Status init_reader(FakeJniTableReader* reader, const std::shared_ptr(); FakeJniTableReader reader; diff --git a/fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java b/fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java index 3279aa3df746af..a6951a22b0e7ad 100644 --- a/fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java +++ b/fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java @@ -46,16 +46,21 @@ import java.lang.management.ThreadMXBean; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Paths; import java.util.Arrays; +import java.util.Base64; import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Optional; import java.util.TimeZone; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.IntSupplier; +import java.util.function.LongSupplier; import java.util.stream.Collectors; public class PaimonJniScanner extends JniScanner { @@ -68,6 +73,11 @@ public class PaimonJniScanner extends JniScanner { static final String JNI_IO_MANAGER_TMP_DIR = "paimon.jni.io_manager.tmp_dir"; static final String JNI_IO_MANAGER_IMPL_CLASS = "paimon.jni.io_manager.impl_class"; private static final AtomicInteger ACTIVE_SCANNERS = new AtomicInteger(); + // Scanner profiles are collected per split, but the asynchronous reader pool is JVM-global. + // Share one sample so profile collection does not allocate ThreadInfo for every scanner. + private static final CachedThreadCounter ASYNC_READER_THREAD_COUNTER = new CachedThreadCounter( + TimeUnit.SECONDS.toNanos(1L), System::nanoTime, + () -> countThreadsByNamePrefix(ASYNC_READER_THREAD_NAME_PREFIX)); private final Map params; private final Map hadoopOptionParams; @@ -92,11 +102,8 @@ public class PaimonJniScanner extends JniScanner { public PaimonJniScanner(int batchSize, Map params) { this.classLoader = this.getClass().getClassLoader(); - if (LOG.isDebugEnabled()) { - LOG.debug("params:{}", params); - } this.params = params; - String[] requiredFields = splitParam(params.get("required_fields"), ","); + String[] requiredFields = requiredFields(params); String[] requiredTypes = splitParam(params.get("columns_types"), "#"); Preconditions.checkArgument(requiredFields.length == requiredTypes.length, "required_fields size %s does not match columns_types size %s", @@ -105,6 +112,11 @@ public PaimonJniScanner(int batchSize, Map params) { for (int i = 0; i < requiredTypes.length; i++) { columnTypes[i] = ColumnType.parseType(requiredFields[i], requiredTypes[i]); } + if (LOG.isDebugEnabled()) { + // The raw map may carry storage or JDBC credentials; diagnostics must only use + // values derived from a fixed non-sensitive whitelist. + LOG.debug(buildDebugSummary(batchSize, requiredFields.length)); + } paimonSplit = params.get("paimon_split"); paimonPredicate = params.get("paimon_predicate"); String timeZone = params.getOrDefault("time_zone", TimeZone.getDefault().getID()); @@ -483,7 +495,27 @@ private static long nonNegative(long value) { } private static int currentAsyncReaderThreadCount() { - return countThreadsByNamePrefix(ASYNC_READER_THREAD_NAME_PREFIX); + return ASYNC_READER_THREAD_COUNTER.get(); + } + + static String buildDebugSummary(int batchSize, int requiredFieldCount) { + return "Paimon JNI scanner configuration: batchSize=" + batchSize + + ", requiredFieldCount=" + requiredFieldCount; + } + + static String[] requiredFields(Map params) { + String encodedFields = params.get("required_fields_base64"); + if (encodedFields == null) { + return splitParam(params.get("required_fields"), ","); + } + if (encodedFields.isEmpty()) { + return new String[0]; + } + // Each identifier is encoded independently, so delimiters in quoted identifiers cannot + // change field cardinality. The legacy parameter remains the rolling-upgrade fallback. + return Arrays.stream(encodedFields.split(",", -1)) + .map(encoded -> new String(Base64.getDecoder().decode(encoded), StandardCharsets.UTF_8)) + .toArray(String[]::new); } static int countThreadsByNamePrefix(String threadNamePrefix) { @@ -498,6 +530,36 @@ static int countThreadsByNamePrefix(String threadNamePrefix) { return count; } + static final class CachedThreadCounter { + private final long ttlNanos; + private final LongSupplier ticker; + private final IntSupplier sampler; + private volatile long lastSampleNanos = Long.MIN_VALUE; + private volatile int cachedValue; + + CachedThreadCounter(long ttlNanos, LongSupplier ticker, IntSupplier sampler) { + this.ttlNanos = ttlNanos; + this.ticker = ticker; + this.sampler = sampler; + } + + int get() { + long now = ticker.getAsLong(); + long sampledAt = lastSampleNanos; + if (sampledAt != Long.MIN_VALUE && now - sampledAt < ttlNanos) { + return cachedValue; + } + synchronized (this) { + now = ticker.getAsLong(); + if (lastSampleNanos == Long.MIN_VALUE || now - lastSampleNanos >= ttlNanos) { + cachedValue = sampler.getAsInt(); + lastSampleNanos = now; + } + return cachedValue; + } + } + } + private void markScannerOpenedForMetrics() { if (!scannerCounted) { scannerCounted = true; diff --git a/fe/be-java-extensions/paimon-scanner/src/test/java/org/apache/doris/paimon/PaimonJniScannerTest.java b/fe/be-java-extensions/paimon-scanner/src/test/java/org/apache/doris/paimon/PaimonJniScannerTest.java index d43f251f58deb1..50e2979f8a7136 100644 --- a/fe/be-java-extensions/paimon-scanner/src/test/java/org/apache/doris/paimon/PaimonJniScannerTest.java +++ b/fe/be-java-extensions/paimon-scanner/src/test/java/org/apache/doris/paimon/PaimonJniScannerTest.java @@ -17,6 +17,13 @@ package org.apache.doris.paimon; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.core.appender.AbstractAppender; +import org.apache.logging.log4j.core.config.Configurator; +import org.apache.logging.log4j.core.config.Property; import org.apache.paimon.data.InternalRow; import org.apache.paimon.disk.BufferFileReader; import org.apache.paimon.disk.BufferFileWriter; @@ -34,14 +41,18 @@ import java.io.IOException; import java.lang.reflect.Field; import java.lang.reflect.Proxy; +import java.nio.charset.StandardCharsets; import java.util.Arrays; +import java.util.Base64; import java.util.Collections; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; public class PaimonJniScannerTest { @Rule @@ -52,6 +63,70 @@ public void testConstructorAcceptsEmptyProjection() { new PaimonJniScanner(128, createBaseParams()); } + @Test + public void testConstructorDecodesDelimiterSafeProjectionNames() { + Map params = createBaseParams(); + params.put("required_fields", "legacy,value,is,ambiguous"); + params.put("required_fields_base64", encodeFields( + "region,code", "hash#name", "地区 名")); + params.put("columns_types", "string#string#string"); + + PaimonJniScanner scanner = new PaimonJniScanner(128, params); + + Assert.assertArrayEquals(new String[] {"region,code", "hash#name", "地区 名"}, + PaimonJniScanner.requiredFields(params)); + Assert.assertEquals("3", scanner.getStatistics().get("gauge:PaimonJniRequiredFieldCount")); + Assert.assertEquals(0, PaimonJniScanner.getFieldIndex( + Arrays.asList("region,code", "hash#name", "地区 名"), "REGION,CODE")); + } + + @Test + public void testDebugSummaryNeverIncludesRawScannerParams() { + Map params = createBaseParams(); + params.put("hadoop.fs.s3a.secret.key", "FAKE_SECRET_MARKER"); + params.put("paimon.jdbc.url", "jdbc:test?password=FAKE_PASSWORD_MARKER"); + + Logger logger = (Logger) LogManager.getLogger(PaimonJniScanner.class); + Level originalLevel = logger.getLevel(); + CapturingAppender appender = new CapturingAppender(); + appender.start(); + logger.addAppender(appender); + Configurator.setLevel(PaimonJniScanner.class.getName(), Level.DEBUG); + try { + new PaimonJniScanner(128, params); + } finally { + logger.removeAppender(appender); + appender.stop(); + Configurator.setLevel(PaimonJniScanner.class.getName(), originalLevel); + } + + String captured = String.join("\n", appender.messages); + Assert.assertTrue(captured.contains("batchSize=128, requiredFieldCount=0")); + Assert.assertFalse(captured.contains("FAKE_SECRET_MARKER")); + Assert.assertFalse(captured.contains("FAKE_PASSWORD_MARKER")); + Assert.assertFalse(captured.contains("hadoop.fs.s3a.secret.key")); + Assert.assertFalse(captured.contains("paimon.jdbc.url")); + } + + @Test + public void testThreadCountSampleIsSharedWithinTtl() { + AtomicLong ticker = new AtomicLong(1000L); + AtomicInteger sampleCalls = new AtomicInteger(); + PaimonJniScanner.CachedThreadCounter counter = new PaimonJniScanner.CachedThreadCounter( + 100L, ticker::get, () -> { + sampleCalls.incrementAndGet(); + return 7; + }); + + Assert.assertEquals(7, counter.get()); + Assert.assertEquals(7, counter.get()); + Assert.assertEquals(1, sampleCalls.get()); + + ticker.addAndGet(100L); + Assert.assertEquals(7, counter.get()); + Assert.assertEquals(2, sampleCalls.get()); + } + @Test public void testIOManagerOptionHelpers() throws Exception { Map params = createBaseParams(); @@ -264,6 +339,12 @@ private Map createBaseParams() { return params; } + private String encodeFields(String... fields) { + return Arrays.stream(fields) + .map(field -> Base64.getEncoder().encodeToString(field.getBytes(StandardCharsets.UTF_8))) + .collect(java.util.stream.Collectors.joining(",")); + } + private void setTableOptions(PaimonJniScanner scanner, Map options) throws Exception { Table table = (Table) Proxy.newProxyInstance( Table.class.getClassLoader(), new Class[] {Table.class}, (proxy, method, args) -> { @@ -337,6 +418,19 @@ public void close() { } } + private static class CapturingAppender extends AbstractAppender { + private final CopyOnWriteArrayList messages = new CopyOnWriteArrayList<>(); + + CapturingAppender() { + super("paimon-test-capture", null, null, false, Property.EMPTY_ARRAY); + } + + @Override + public void append(LogEvent event) { + messages.add(event.getMessage().getFormattedMessage()); + } + } + @Test public void testGetFieldIndexMatchesMixedCaseColumns() { Assert.assertEquals(1, PaimonJniScanner.getFieldIndex(Arrays.asList("data", "mIxEd_COL", "PART"), diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java index 83e778ac2f0ced..73455ad2de9ff6 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java @@ -656,11 +656,17 @@ public void replayAlterCatalogProps(CatalogLog log, Map oldPrope if (!isReplay) { try { ((ExternalCatalog) catalog).checkProperties(); - } catch (DdlException ddlException) { + } catch (Exception validationException) { + // Connector-specific validators may throw unchecked exceptions after the + // tentative mutation; every validation failure must preserve atomic ALTER. if (oldProperties != null) { ((ExternalCatalog) catalog).rollBackCatalogProps(oldProperties); } - throw ddlException; + if (validationException instanceof DdlException) { + throw (DdlException) validationException; + } + throw new DdlException("Invalid catalog properties: " + + validationException.getMessage(), validationException); } } if (newProps.containsKey(METADATA_REFRESH_INTERVAL_SEC)) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java index 3409df1e42c079..0760c9fd2544d9 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java @@ -136,8 +136,7 @@ public Table getPaimonTable(NameMapping nameMapping, String branch, String query } return executionAuthenticator.execute(() -> { Table table = catalog.getTable(identifier); - Map tableOptions = - paimonProperties.getTableOptionsForCopy(table.options()); + Map tableOptions = paimonProperties.getTableOptionsForCopy(); return tableOptions.isEmpty() ? table : table.copy(tableOptions); }); } catch (Exception e) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java new file mode 100644 index 00000000000000..8adab5b39d7038 --- /dev/null +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java @@ -0,0 +1,84 @@ +// 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 java.util.Collections; +import java.util.Set; + +/** Validation shared by catalog-scoped and relation-scoped Paimon reader tuning. */ +public final class PaimonReaderOptions { + 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 SUPPORTED_OPTIONS = ImmutableSet.of( + CoreOptions.READ_BATCH_SIZE.key(), + CoreOptions.FILE_READER_ASYNC_THRESHOLD.key()); + + private PaimonReaderOptions() { + } + + public static Set 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); + } + } + + private static T parse(String key, String value, ConfigOption option) { + try { + return new Options(Collections.singletonMap(key, value)).get(option); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Invalid value for Paimon option '" + key + "': " + + e.getMessage(), e); + } + } + + private static void requireRange(String key, long value, long minimum, long maximum) { + if (value < minimum || value > maximum) { + throw new IllegalArgumentException("Paimon option '" + key + "' must be between " + + minimum + " and " + maximum + ", but was " + value); + } + } +} diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java index 5152f63775cc95..e28db5be64e6e2 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java @@ -45,18 +45,20 @@ public final class PaimonScanParams { "doris.internal.paimon.file-creation-time-millis"; private static final String PINNED_EMPTY_SCAN = "doris.internal.paimon.empty-scan"; - private static final Set QUERY_OPTION_KEYS = ImmutableSet.of( - CoreOptions.SCAN_MODE.key(), - CoreOptions.SCAN_TIMESTAMP.key(), - CoreOptions.SCAN_TIMESTAMP_MILLIS.key(), - CoreOptions.SCAN_WATERMARK.key(), - CoreOptions.SCAN_FILE_CREATION_TIME_MILLIS.key(), - CoreOptions.SCAN_CREATION_TIME_MILLIS.key(), - CoreOptions.SCAN_SNAPSHOT_ID.key(), - CoreOptions.SCAN_TAG_NAME.key(), - CoreOptions.SCAN_VERSION.key(), - CoreOptions.SCAN_MANIFEST_PARALLELISM.key(), - CoreOptions.SCAN_PLAN_SORT_PARTITION.key()); + private static final Set QUERY_OPTION_KEYS = ImmutableSet.builder() + .add(CoreOptions.SCAN_MODE.key()) + .add(CoreOptions.SCAN_TIMESTAMP.key()) + .add(CoreOptions.SCAN_TIMESTAMP_MILLIS.key()) + .add(CoreOptions.SCAN_WATERMARK.key()) + .add(CoreOptions.SCAN_FILE_CREATION_TIME_MILLIS.key()) + .add(CoreOptions.SCAN_CREATION_TIME_MILLIS.key()) + .add(CoreOptions.SCAN_SNAPSHOT_ID.key()) + .add(CoreOptions.SCAN_TAG_NAME.key()) + .add(CoreOptions.SCAN_VERSION.key()) + .add(CoreOptions.SCAN_MANIFEST_PARALLELISM.key()) + .add(CoreOptions.SCAN_PLAN_SORT_PARTITION.key()) + .addAll(PaimonReaderOptions.supportedOptions()) + .build(); private static final Set STARTUP_POSITION_KEYS = ImmutableSet.of( CoreOptions.SCAN_TIMESTAMP.key(), @@ -101,6 +103,11 @@ public static void validateOptions(Map options) { throw new IllegalArgumentException("Unsupported Paimon query option(s): " + unsupported); } + PaimonReaderOptions.supportedOptions().stream() + .filter(options::containsKey) + .forEach(key -> PaimonReaderOptions.validate(key, options.get(key))); + validateManifestParallelism(options.get(CoreOptions.SCAN_MANIFEST_PARALLELISM.key())); + String scanMode = options.get(CoreOptions.SCAN_MODE.key()); if ("from-creation-timestamp".equalsIgnoreCase(scanMode) && options.get(CoreOptions.SCAN_CREATION_TIME_MILLIS.key()) == null) { @@ -129,6 +136,27 @@ public static void validateOptions(Map options) { } } + private static void validateManifestParallelism(String value) { + if (value == null) { + return; + } + int parallelism; + try { + parallelism = Integer.parseInt(value); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Invalid value for Paimon query option '" + + CoreOptions.SCAN_MANIFEST_PARALLELISM.key() + "': " + value, e); + } + int maximum = Runtime.getRuntime().availableProcessors(); + // Paimon replaces a JVM-static manifest executor when this value exceeds its CPU-sized + // default. Keeping the query value within that capacity prevents cross-query mutation. + if (parallelism < 1 || parallelism > maximum) { + throw new IllegalArgumentException("Paimon query option '" + + CoreOptions.SCAN_MANIFEST_PARALLELISM.key() + "' must be between 1 and " + + maximum + ", but was " + parallelism); + } + } + public static Table applyOptions(Table table, Map options) { Map tableOptions = userOptions(options); validateOptions(tableOptions); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractPaimonProperties.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractPaimonProperties.java index 70d16dfebc1937..8095d9e883d000 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractPaimonProperties.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractPaimonProperties.java @@ -18,6 +18,7 @@ package org.apache.doris.datasource.property.metastore; import org.apache.doris.common.security.authentication.ExecutionAuthenticator; +import org.apache.doris.datasource.paimon.PaimonReaderOptions; import org.apache.doris.datasource.storage.StorageAdapter; import org.apache.doris.foundation.property.ConnectorProperty; @@ -25,11 +26,8 @@ import lombok.Getter; import org.apache.commons.lang3.StringUtils; import org.apache.hadoop.conf.Configuration; -import org.apache.paimon.CoreOptions; import org.apache.paimon.catalog.Catalog; import org.apache.paimon.options.CatalogOptions; -import org.apache.paimon.options.ConfigOption; -import org.apache.paimon.options.FallbackKey; import org.apache.paimon.options.Options; import java.util.Collections; @@ -62,7 +60,6 @@ public abstract class AbstractPaimonProperties extends MetastoreProperties { private static final String DORIS_JNI_PROPERTY_PREFIX = "paimon.jni."; /** The suffix after this prefix is passed to Paimon as a dynamic table option. */ public static final String TABLE_OPTION_PREFIX = "paimon.table-option."; - private static final SupportedTableOptions SUPPORTED_TABLE_OPTIONS = SupportedTableOptions.build(); private Map tableOptionsMap = Collections.emptyMap(); @@ -146,25 +143,12 @@ public Map getTableOptionsMap() { } /** - * Returns Catalog table options which are not explicitly configured by the Paimon table. - * - *

The comparison is based on Paimon {@link ConfigOption}s so canonical and fallback keys - * follow the same precedence rule. + * Returns Catalog-scoped dynamic reader options to copy onto a Paimon table. + * Catalog options intentionally override physical table values, while a subsequent + * relation-scoped copy can override the Catalog for one relation only. */ - public Map getTableOptionsForCopy(Map currentTableOptions) { - if (tableOptionsMap.isEmpty() || currentTableOptions.isEmpty()) { - return tableOptionsMap; - } - - Options existingOptions = new Options(currentTableOptions); - Map optionsForCopy = new LinkedHashMap<>(); - tableOptionsMap.forEach((key, value) -> { - ConfigOption option = SUPPORTED_TABLE_OPTIONS.find(key); - if (!existingOptions.contains(option)) { - optionsForCopy.put(key, value); - } - }); - return Collections.unmodifiableMap(optionsForCopy); + public Map getTableOptionsForCopy() { + return tableOptionsMap; } public static boolean isTableOptionProperty(String key) { @@ -188,44 +172,14 @@ private Map extractTableOptions() { } private void validateTableOption(String key, String value) { - ConfigOption option = SUPPORTED_TABLE_OPTIONS.find(key); - if (option == null) { - throw new IllegalArgumentException("Unsupported Paimon table option '" + key - + "' for the bundled Paimon version"); - } - try { - new Options(Collections.singletonMap(key, value)).get(option); + PaimonReaderOptions.validate(key, value); } catch (IllegalArgumentException e) { throw new IllegalArgumentException("Invalid value for Paimon table option '" + key + "': " + e.getMessage(), e); } } - private static final class SupportedTableOptions { - /** Canonical and fallback option names which support direct lookup. */ - private final Map> exactOptions; - - private SupportedTableOptions(Map> exactOptions) { - this.exactOptions = exactOptions; - } - - private static SupportedTableOptions build() { - Map> exactOptions = new HashMap<>(); - for (ConfigOption option : CoreOptions.getOptions()) { - exactOptions.put(option.key(), option); - for (FallbackKey fallbackKey : option.fallbackKeys()) { - exactOptions.put(fallbackKey.getKey(), option); - } - } - return new SupportedTableOptions(Collections.unmodifiableMap(exactOptions)); - } - - private ConfigOption find(String key) { - return exactOptions.get(key); - } - } - /** * @See org.apache.paimon.s3.S3FileIO * Possible S3 config key prefixes: diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogMgrTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogMgrTest.java new file mode 100644 index 00000000000000..da55fa69464eff --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogMgrTest.java @@ -0,0 +1,66 @@ +// 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; + +import org.apache.doris.catalog.DatabaseIf; +import org.apache.doris.catalog.TableIf; +import org.apache.doris.common.DdlException; + +import com.google.common.collect.ImmutableMap; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +import java.lang.reflect.Field; +import java.util.Map; +import java.util.concurrent.ConcurrentMap; + +public class CatalogMgrTest { + + @Test + void testAlterCatalogRollsBackUncheckedValidationFailure() throws Exception { + CatalogMgr catalogMgr = new CatalogMgr(); + ExternalCatalog catalog = Mockito.mock(ExternalCatalog.class); + long catalogId = 42L; + Mockito.when(catalog.getId()).thenReturn(catalogId); + + Field idToCatalogField = CatalogMgr.class.getDeclaredField("idToCatalog"); + idToCatalogField.setAccessible(true); + @SuppressWarnings("unchecked") + ConcurrentMap>> idToCatalog = + (ConcurrentMap>>) + idToCatalogField.get(catalogMgr); + idToCatalog.put(catalogId, catalog); + + Map oldProperties = ImmutableMap.of("read.batch-size", "1024"); + Map newProperties = ImmutableMap.of("read.batch-size", "0"); + CatalogLog log = new CatalogLog(); + log.setCatalogId(catalogId); + log.setNewProps(newProperties); + Mockito.doThrow(new IllegalArgumentException("invalid reader option")) + .when(catalog).checkProperties(); + + DdlException exception = Assertions.assertThrows(DdlException.class, + () -> catalogMgr.replayAlterCatalogProps(log, oldProperties, false)); + + Assertions.assertTrue(exception.getMessage().contains("invalid reader option")); + Mockito.verify(catalog).tryModifyCatalogProps(newProperties); + Mockito.verify(catalog).rollBackCatalogProps(oldProperties); + Mockito.verify(catalog, Mockito.never()).modifyCatalogProps(newProperties); + } +} diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonScanParamsTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonScanParamsTest.java index b003acac955fa7..6da81b8082aa02 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonScanParamsTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonScanParamsTest.java @@ -44,6 +44,53 @@ public void testValidateKnownScanOptions() { "scan.plan-sort-partition", "true")); } + @Test + public void testValidateRelationScopedReaderOptions() { + PaimonScanParams.validateOptions(ImmutableMap.of( + "read.batch-size", "4096", + "file-reader-async-threshold", "16 MB")); + + for (Map options : new Map[] { + ImmutableMap.of("read.batch-size", "0"), + ImmutableMap.of("read.batch-size", "-1"), + ImmutableMap.of("read.batch-size", "65537"), + ImmutableMap.of("file-reader-async-threshold", "512 KB"), + ImmutableMap.of("file-reader-async-threshold", "2 GB") + }) { + Assert.assertThrows(IllegalArgumentException.class, + () -> PaimonScanParams.validateOptions(options)); + } + } + + @Test + public void testManifestParallelismCannotMutateGlobalPoolCapacity() { + int availableProcessors = Runtime.getRuntime().availableProcessors(); + PaimonScanParams.validateOptions(ImmutableMap.of( + "scan.manifest.parallelism", String.valueOf(availableProcessors))); + + for (int invalid : new int[] {0, -1, availableProcessors + 1}) { + IllegalArgumentException exception = Assert.assertThrows( + IllegalArgumentException.class, + () -> PaimonScanParams.validateOptions(ImmutableMap.of( + "scan.manifest.parallelism", String.valueOf(invalid)))); + Assert.assertTrue(exception.getMessage().contains("scan.manifest.parallelism")); + } + } + + @Test + public void testRelationReaderOptionsAreAppliedWithoutMutatingBaseTable() { + Table table = Mockito.mock(Table.class); + Table copied = Mockito.mock(Table.class); + Mockito.when(table.copy(ArgumentMatchers.anyMap())).thenReturn(copied); + + Assert.assertSame(copied, PaimonScanParams.applyOptions(table, ImmutableMap.of( + "read.batch-size", "8192", + "file-reader-async-threshold", "32 MB"))); + Mockito.verify(table).copy(ImmutableMap.of( + "read.batch-size", "8192", + "file-reader-async-threshold", "32 MB")); + } + @Test public void testRejectUnknownAndConflictingOptions() { IllegalArgumentException typo = Assert.assertThrows( diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/property/metastore/AbstractPaimonPropertiesTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/property/metastore/AbstractPaimonPropertiesTest.java index e9391e9e6d49cc..edb4f8b58dced3 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/property/metastore/AbstractPaimonPropertiesTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/property/metastore/AbstractPaimonPropertiesTest.java @@ -24,7 +24,6 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -93,40 +92,32 @@ void testExtractAndValidateTableOptions() { input.put("warehouse", "s3://tmp/warehouse"); input.put("paimon.jni.enable_jni_io_manager", "true"); input.put("paimon.table-option.read.batch-size", "4096"); - input.put("paimon.table-option.file.compression.per.level", "0:lz4,1:zstd"); + input.put("paimon.table-option.file-reader-async-threshold", "16 MB"); TestPaimonProperties testProps = new TestPaimonProperties(input); testProps.initNormalizeAndCheckProps(); testProps.buildCatalogOptions(); Assertions.assertEquals("4096", testProps.getTableOptionsMap().get("read.batch-size")); - Assertions.assertEquals( - "0:lz4,1:zstd", testProps.getTableOptionsMap().get("file.compression.per.level")); + Assertions.assertEquals("16 MB", + testProps.getTableOptionsMap().get("file-reader-async-threshold")); Assertions.assertFalse(testProps.getCatalogOptionsMap().containsKey("table-option.read.batch-size")); Assertions.assertFalse(testProps.getCatalogOptionsMap().containsKey("jni.enable_jni_io_manager")); } @Test - void testPaimonTableOptionsTakePrecedenceOverCatalogOptions() { + void testCatalogReaderOptionsTakePrecedenceOverPhysicalTableOptions() { Map input = new HashMap<>(); input.put("warehouse", "s3://tmp/warehouse"); input.put("paimon.table-option.read.batch-size", "4096"); - input.put("paimon.table-option.write.batch-size", "2048"); - input.put("paimon.table-option.file.compression.per.level", "0:lz4,1:zstd"); + input.put("paimon.table-option.file-reader-async-threshold", "16 MB"); TestPaimonProperties testProps = new TestPaimonProperties(input); testProps.initNormalizeAndCheckProps(); - Map currentTableOptions = new HashMap<>(); - currentTableOptions.put("read.batch-size", "1024"); - currentTableOptions.put("orc.write.batch-size", "512"); - currentTableOptions.put("file.compression.per.level", "0:snappy"); - - Map optionsForCopy = - testProps.getTableOptionsForCopy(currentTableOptions); + Map optionsForCopy = testProps.getTableOptionsForCopy(); - Assertions.assertFalse(optionsForCopy.containsKey("read.batch-size")); - Assertions.assertFalse(optionsForCopy.containsKey("write.batch-size")); - Assertions.assertFalse(optionsForCopy.containsKey("file.compression.per.level")); + Assertions.assertEquals("4096", optionsForCopy.get("read.batch-size")); + Assertions.assertEquals("16 MB", optionsForCopy.get("file-reader-async-threshold")); } @Test @@ -137,9 +128,7 @@ void testCatalogTableOptionsFillMissingPaimonTableOptions() { TestPaimonProperties testProps = new TestPaimonProperties(input); testProps.initNormalizeAndCheckProps(); - Map optionsForCopy = - testProps.getTableOptionsForCopy(Collections.singletonMap( - "path", "s3://tmp/warehouse/test.db/test")); + Map optionsForCopy = testProps.getTableOptionsForCopy(); Assertions.assertEquals("4096", optionsForCopy.get("read.batch-size")); } @@ -183,6 +172,49 @@ void testRejectInvalidTableOptionValue() { Assertions.assertTrue(exception.getMessage().contains("read.batch-size")); } + @Test + void testRejectUnsafeTableOptions() { + for (String option : new String[] { + "branch", "path", "scan.tag-name", "scan.snapshot-id", + "write.batch-size", "file.compression.per.level" + }) { + Map input = new HashMap<>(); + input.put("warehouse", "s3://tmp/warehouse"); + input.put("paimon.table-option." + option, "1"); + + IllegalArgumentException exception = Assertions.assertThrows( + IllegalArgumentException.class, + () -> new TestPaimonProperties(input).initNormalizeAndCheckProps(), + option); + Assertions.assertTrue(exception.getMessage().contains(option), option); + } + } + + @Test + void testRejectOutOfRangeReaderOptions() { + Map invalidOptions = new HashMap<>(); + invalidOptions.put("read.batch-size", "0"); + invalidOptions.put("read.batch-size-negative", "-1"); + invalidOptions.put("read.batch-size-too-large", "65537"); + invalidOptions.put("file-reader-async-threshold", "0 B"); + invalidOptions.put("file-reader-async-threshold-too-small", "512 KB"); + invalidOptions.put("file-reader-async-threshold-too-large", "2 GB"); + + invalidOptions.forEach((caseName, value) -> { + String option = caseName.startsWith("read.batch-size") + ? "read.batch-size" : "file-reader-async-threshold"; + Map input = new HashMap<>(); + input.put("warehouse", "s3://tmp/warehouse"); + input.put("paimon.table-option." + option, value); + + IllegalArgumentException exception = Assertions.assertThrows( + IllegalArgumentException.class, + () -> new TestPaimonProperties(input).initNormalizeAndCheckProps(), + caseName); + Assertions.assertTrue(exception.getMessage().contains(option), caseName); + }); + } + @Test void testForwardTableDefaultOptionsToPaimonCatalog() { Map input = new HashMap<>(); diff --git a/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy b/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy new file mode 100644 index 00000000000000..038be85bc29d33 --- /dev/null +++ b/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy @@ -0,0 +1,147 @@ +// 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. + +suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { + String enabled = context.config.otherConfigs.get("enablePaimonTest") + if (enabled == null || !enabled.equalsIgnoreCase("true")) { + logger.info("disable paimon test") + return + } + + String minioPort = context.config.otherConfigs.get("iceberg_minio_port") + String externalEnvIp = context.config.otherConfigs.get("externalEnvIp") + String catalogName = "test_paimon_jni_reader_guardrails" + String dbName = "paimon_jni_guardrails_db" + + def catalogDdl = { String name, String extraProperty -> + return """ + create catalog ${name} properties ( + 'type'='paimon', + 'warehouse'='s3://warehouse/wh', + 's3.endpoint'='http://${externalEnvIp}:${minioPort}', + 's3.access_key'='admin', + 's3.secret_key'='password', + 's3.path.style.access'='true' + ${extraProperty} + ) + """ + } + + for (def invalid : [ + ["invalid_batch", ", 'paimon.table-option.read.batch-size'='0'", "read.batch-size"], + ["unsafe_branch", ", 'paimon.table-option.branch'='archive'", "branch"] + ]) { + String invalidCatalog = "${catalogName}_${invalid[0]}" + sql "drop catalog if exists ${invalidCatalog}" + try { + test { + sql(catalogDdl(invalidCatalog, invalid[1])) + exception invalid[2] + } + } finally { + sql "drop catalog if exists ${invalidCatalog}" + } + } + + sql "drop catalog if exists ${catalogName}" + sql(catalogDdl(catalogName, """ + , 'paimon.table-option.read.batch-size'='1024' + , 'paimon.table-option.file-reader-async-threshold'='16 MB' + """)) + + try { + spark_paimon_multi """ + create database if not exists paimon.${dbName}; + drop table if exists paimon.${dbName}.quoted_reader_options; + create table paimon.${dbName}.quoted_reader_options ( + id int, + `region,code` string, + `hash#name` string, + `display name` string, + `地区 名` string + ) using paimon + tblproperties ('file.format'='parquet', 'read.batch-size'='512'); + insert into paimon.${dbName}.quoted_reader_options values + (1, 'east,01', 'hash-one', 'first row', '华东'), + (2, 'west,02', 'hash-two', 'second row', '华西'); + """ + + sql "switch ${catalogName}" + sql "use ${dbName}" + sql "set force_jni_scanner=true" + + test { + sql """ + select `region,code`, `hash#name`, `display name`, `地区 名` + from quoted_reader_options + where `region,code` in ('east,01', 'west,02') + order by id + """ + } + + test { + sql """ + select id from quoted_reader_options@options( + 'read.batch-size'='4096', + 'file-reader-async-threshold'='32 MB') + order by id + """ + } + test { + sql """ + select small.id, large.id + from quoted_reader_options@options('read.batch-size'='1') small + join quoted_reader_options@options('read.batch-size'='8192') large + on small.id = large.id + order by small.id + """ + } + + for (def invalidOption : [ + ["read.batch-size", "0"], + ["read.batch-size", "65537"], + ["file-reader-async-threshold", "512 KB"], + ["file-reader-async-threshold", "2 GB"], + ["scan.manifest.parallelism", "0"], + ["scan.manifest.parallelism", "2147483647"] + ]) { + test { + sql """ + select * from quoted_reader_options@options( + '${invalidOption[0]}'='${invalidOption[1]}') + """ + exception invalidOption[0] + } + } + + test { + sql """ + alter catalog ${catalogName} set properties ( + 'paimon.table-option.read.batch-size'='0') + """ + exception "read.batch-size" + } + // A failed ALTER must not leave read.batch-size=0 behind; on the old path this follow-up + // JNI scan can stop making progress instead of completing. + test { + sql "select count(*) from quoted_reader_options" + } + } finally { + sql "set force_jni_scanner=false" + sql "drop catalog if exists ${catalogName}" + } +} From b336eb4293f1857488e9d58dcdbf274ad908b05b Mon Sep 17 00:00:00 2001 From: Gabriel Date: Thu, 30 Jul 2026 08:45:09 +0800 Subject: [PATCH 2/6] [fix](paimon) Address JNI reader review findings Use paired delimiter-safe schema framing in both scanner routes, validate effective physical options, validate Paimon ALTER candidates off the live catalog, and preserve load compatibility for legacy catalog options. Add unit and P0 coverage for each review scenario. --- be/src/format/jni/jni_data_bridge.cpp | 50 +++++++ be/src/format/jni/jni_data_bridge.h | 6 + be/src/format/table/paimon_jni_reader.cpp | 10 ++ be/src/format_v2/jni/jni_table_reader.cpp | 21 +-- be/src/format_v2/jni/jni_table_reader.h | 2 - .../format_v2/jni/jni_table_reader_test.cpp | 15 +- .../doris/common/jni/vec/ColumnType.java | 39 ++++- .../apache/doris/paimon/PaimonJniScanner.java | 33 ++++- .../doris/paimon/PaimonJniScannerTest.java | 30 ++++ .../apache/doris/datasource/CatalogMgr.java | 19 ++- .../doris/datasource/ExternalCatalog.java | 18 ++- .../paimon/PaimonExternalCatalog.java | 35 ++++- .../paimon/PaimonReaderOptions.java | 60 ++++++++ .../datasource/paimon/PaimonScanParams.java | 26 +--- .../metastore/AbstractPaimonProperties.java | 26 +--- .../doris/datasource/CatalogMgrTest.java | 139 +++++++++++++++++- .../paimon/PaimonReaderOptionsTest.java | 56 +++++++ .../AbstractPaimonPropertiesTest.java | 58 +++++--- .../test_paimon_jni_reader_guardrails.groovy | 54 ++++++- 19 files changed, 573 insertions(+), 124 deletions(-) create mode 100644 fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java diff --git a/be/src/format/jni/jni_data_bridge.cpp b/be/src/format/jni/jni_data_bridge.cpp index 1a88f4c317b28a..8fa1b894233ec4 100644 --- a/be/src/format/jni/jni_data_bridge.cpp +++ b/be/src/format/jni/jni_data_bridge.cpp @@ -38,6 +38,8 @@ #include "core/data_type/primitive_type.h" #include "core/types.h" #include "core/value/decimalv2_value.h" +#include "util/string_util.h" +#include "util/url_coding.h" namespace doris { @@ -490,6 +492,54 @@ std::string JniDataBridge::get_jni_type_with_different_string(const DataTypePtr& } } +std::string JniDataBridge::encode_schema_values(const std::vector& values) { + std::vector encoded_values; + encoded_values.reserve(values.size()); + for (const auto& value : values) { + std::string encoded; + base64_encode(value, &encoded); + encoded_values.emplace_back(std::move(encoded)); + } + return join(encoded_values, ","); +} + +std::string JniDataBridge::get_jni_type_with_encoded_struct_fields(const DataTypePtr& data_type) { + switch (data_type->get_primitive_type()) { + case TYPE_STRUCT: { + const auto* type_struct = + assert_cast(remove_nullable(data_type).get()); + std::ostringstream buffer; + buffer << "struct<"; + for (int i = 0; i < type_struct->get_elements().size(); ++i) { + if (i != 0) { + buffer << ","; + } + std::string encoded_name; + base64_encode(type_struct->get_element_name(i), &encoded_name); + // '$' versions the nested-name token and cannot collide with Base64 or grammar + // delimiters; Java rejects an unversioned name in the encoded schema path. + buffer << "$" << encoded_name << ":" + << get_jni_type_with_encoded_struct_fields(type_struct->get_element(i)); + } + buffer << ">"; + return buffer.str(); + } + case TYPE_ARRAY: { + const auto* type_array = + assert_cast(remove_nullable(data_type).get()); + return "array<" + get_jni_type_with_encoded_struct_fields(type_array->get_nested_type()) + + ">"; + } + case TYPE_MAP: { + const auto* type_map = assert_cast(remove_nullable(data_type).get()); + return "map<" + get_jni_type_with_encoded_struct_fields(type_map->get_key_type()) + "," + + get_jni_type_with_encoded_struct_fields(type_map->get_value_type()) + ">"; + } + default: + return get_jni_type_with_different_string(data_type); + } +} + Status JniDataBridge::_fill_column_meta(const ColumnPtr& doris_column, const DataTypePtr& data_type, std::vector& meta_data) { auto logical_type = data_type->get_primitive_type(); diff --git a/be/src/format/jni/jni_data_bridge.h b/be/src/format/jni/jni_data_bridge.h index d3deca8a85076d..e037ffec3d4d5a 100644 --- a/be/src/format/jni/jni_data_bridge.h +++ b/be/src/format/jni/jni_data_bridge.h @@ -140,6 +140,12 @@ class JniDataBridge { */ static std::string get_jni_type_with_different_string(const DataTypePtr& data_type); + /** Encodes every list element independently so schema delimiters remain unambiguous. */ + static std::string encode_schema_values(const std::vector& values); + + /** Encodes STRUCT field names inside a JNI type descriptor. */ + static std::string get_jni_type_with_encoded_struct_fields(const DataTypePtr& data_type); + private: // Column fill helpers for various types static Status _fill_string_column(TableMetaAddress& address, MutableColumnPtr& doris_column, diff --git a/be/src/format/table/paimon_jni_reader.cpp b/be/src/format/table/paimon_jni_reader.cpp index e03e28186a29f8..15f81fe6fcab79 100644 --- a/be/src/format/table/paimon_jni_reader.cpp +++ b/be/src/format/table/paimon_jni_reader.cpp @@ -54,10 +54,14 @@ PaimonJniReader::PaimonJniReader(const std::vector& file_slot_d [&]() { std::vector column_names; std::vector column_types; + std::vector encoded_column_types; for (const auto& desc : file_slot_descs) { column_names.emplace_back(desc->col_name()); column_types.emplace_back( JniDataBridge::get_jni_type_with_different_string(desc->type())); + encoded_column_types.emplace_back( + JniDataBridge::get_jni_type_with_encoded_struct_fields( + desc->type())); } const auto& paimon_params = range.table_format_params.paimon_params; std::map params; @@ -70,6 +74,12 @@ PaimonJniReader::PaimonJniReader(const std::vector& file_slot_d } params["required_fields"] = join(column_names, ","); params["columns_types"] = join(column_types, "#"); + // V1 and V2 must publish the same safe schema protocol because session + // routing can select either producer for the same Paimon scanner. + params["required_fields_base64"] = + JniDataBridge::encode_schema_values(column_names); + params["columns_types_base64"] = + JniDataBridge::encode_schema_values(encoded_column_types); params["time_zone"] = state->timezone(); if (range_params->__isset.serialized_table) { params["serialized_table"] = range_params->serialized_table; diff --git a/be/src/format_v2/jni/jni_table_reader.cpp b/be/src/format_v2/jni/jni_table_reader.cpp index f88cf6132a3c7d..c41d9075bb3fab 100644 --- a/be/src/format_v2/jni/jni_table_reader.cpp +++ b/be/src/format_v2/jni/jni_table_reader.cpp @@ -27,21 +27,9 @@ #include "runtime/file_scan_profile.h" #include "runtime/runtime_state.h" #include "util/string_util.h" -#include "util/url_coding.h" namespace doris::format { -std::string encode_jni_required_fields(const std::vector& required_fields) { - std::vector encoded_fields; - encoded_fields.reserve(required_fields.size()); - for (const auto& field : required_fields) { - std::string encoded; - base64_encode(field, &encoded); - encoded_fields.emplace_back(std::move(encoded)); - } - return join(encoded_fields, ","); -} - Status JniTableReader::init(TableReadOptions&& options) { RETURN_IF_ERROR(TableReader::init(std::move(options))); { @@ -510,9 +498,11 @@ Status JniTableReader::_set_open_scanner_batch_size(size_t batch_size) { void JniTableReader::_prepare_jni_scanner_schema() { std::vector required_fields; std::vector column_types; + std::vector encoded_column_types; std::vector replace_types; required_fields.reserve(_jni_columns.size()); column_types.reserve(_jni_columns.size()); + encoded_column_types.reserve(_jni_columns.size()); replace_types.reserve(_jni_columns.size()); _jni_block_template.clear(); _jni_block_template.reserve(_jni_columns.size()); @@ -523,6 +513,8 @@ void JniTableReader::_prepare_jni_scanner_schema() { required_fields.push_back(column.java_name); column_types.push_back( JniDataBridge::get_jni_type_with_different_string(column.transfer_type)); + encoded_column_types.push_back( + JniDataBridge::get_jni_type_with_encoded_struct_fields(column.transfer_type)); replace_types.push_back(column.replace_type); has_replace_type = has_replace_type || column.replace_type != "not_replace"; _jni_block_template.insert( @@ -531,8 +523,11 @@ void JniTableReader::_prepare_jni_scanner_schema() { _scanner_params["required_fields"] = join(required_fields, ","); // Preserve the legacy field during rolling upgrades, while new scanners use independently // encoded identifiers so commas and other legal identifier characters cannot alter framing. - _scanner_params["required_fields_base64"] = encode_jni_required_fields(required_fields); + _scanner_params["required_fields_base64"] = + JniDataBridge::encode_schema_values(required_fields); _scanner_params["columns_types"] = join(column_types, "#"); + _scanner_params["columns_types_base64"] = + JniDataBridge::encode_schema_values(encoded_column_types); if (has_replace_type) { _scanner_params["replace_string"] = join(replace_types, ","); } diff --git a/be/src/format_v2/jni/jni_table_reader.h b/be/src/format_v2/jni/jni_table_reader.h index c9bfa038b6a709..5e48270515f996 100644 --- a/be/src/format_v2/jni/jni_table_reader.h +++ b/be/src/format_v2/jni/jni_table_reader.h @@ -30,8 +30,6 @@ namespace doris::format { -std::string encode_jni_required_fields(const std::vector& required_fields); - class JniTableReader : public TableReader { public: struct JniColumn { diff --git a/be/test/format_v2/jni/jni_table_reader_test.cpp b/be/test/format_v2/jni/jni_table_reader_test.cpp index 2c9e066bc3213a..eb8ba25027967d 100644 --- a/be/test/format_v2/jni/jni_table_reader_test.cpp +++ b/be/test/format_v2/jni/jni_table_reader_test.cpp @@ -27,6 +27,9 @@ #include #include +#include "core/data_type/data_type_string.h" +#include "core/data_type/data_type_struct.h" +#include "format/jni/jni_data_bridge.h" #include "io/io_common.h" namespace doris::format { @@ -110,10 +113,20 @@ Status init_reader(FakeJniTableReader* reader, const std::shared_ptr( + DataTypes {std::make_shared(), std::make_shared(), + std::make_shared()}, + Strings {"hash#name", "region,code", "colon:name"}); + + EXPECT_EQ(JniDataBridge::get_jni_type_with_encoded_struct_fields(type), + "struct<$aGFzaCNuYW1l:string,$cmVnaW9uLGNvZGU:string,$Y29sb246bmFtZQ:string>"); +} + TEST(JniTableReaderTest, CancellationStopsBeforeFetchingAnotherJavaBatch) { auto io_ctx = std::make_shared(); FakeJniTableReader reader; diff --git a/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/vec/ColumnType.java b/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/vec/ColumnType.java index dfd40b1541e877..f1c40b45b4bdd4 100644 --- a/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/vec/ColumnType.java +++ b/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/vec/ColumnType.java @@ -17,8 +17,10 @@ package org.apache.doris.common.jni.vec; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; +import java.util.Base64; import java.util.Collections; import java.util.List; import java.util.regex.Matcher; @@ -277,6 +279,15 @@ private static int findNextNestedField(String commaSplitFields) { } public static ColumnType parseType(String columnName, String hiveType) { + return parseType(columnName, hiveType, false); + } + + public static ColumnType parseTypeWithEncodedStructFields(String columnName, String hiveType) { + return parseType(columnName, hiveType, true); + } + + private static ColumnType parseType(String columnName, String hiveType, boolean encodedStructFields) { + String originalType = hiveType.trim(); String lowerCaseType = hiveType.toLowerCase(); Type type = Type.UNSUPPORTED; int length = -1; @@ -397,7 +408,7 @@ public static ColumnType parseType(String columnName, String hiveType) { if (lowerCaseType.indexOf("<") == 5 && lowerCaseType.lastIndexOf(">") == lowerCaseType.length() - 1) { ColumnType nestedType = parseType("element", - lowerCaseType.substring(6, lowerCaseType.length() - 1)); + originalType.substring(6, originalType.length() - 1), encodedStructFields); ColumnType arrayType = new ColumnType(columnName, Type.ARRAY); arrayType.setChildTypes(Collections.singletonList(nestedType)); return arrayType; @@ -405,12 +416,13 @@ public static ColumnType parseType(String columnName, String hiveType) { } else if (lowerCaseType.startsWith("map")) { if (lowerCaseType.indexOf("<") == 3 && lowerCaseType.lastIndexOf(">") == lowerCaseType.length() - 1) { - String keyValue = lowerCaseType.substring(4, lowerCaseType.length() - 1); + String keyValue = originalType.substring(4, originalType.length() - 1); int index = findNextNestedField(keyValue); if (index != keyValue.length() && index != 0) { - ColumnType keyType = parseType("key", keyValue.substring(0, index).trim()); + ColumnType keyType = parseType( + "key", keyValue.substring(0, index).trim(), encodedStructFields); ColumnType valueType = - parseType("value", keyValue.substring(index + 1).trim()); + parseType("value", keyValue.substring(index + 1).trim(), encodedStructFields); ColumnType mapType = new ColumnType(columnName, Type.MAP); mapType.setChildTypes(Arrays.asList(keyType, valueType)); return mapType; @@ -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); ArrayList fields = new ArrayList<>(); ArrayList names = new ArrayList<>(); while (listFields.length() > 0) { int index = findNextNestedField(listFields); int pivot = listFields.indexOf(':'); if (pivot > 0 && pivot < listFields.length() - 1) { - fields.add(parseType(listFields.substring(0, pivot), - listFields.substring(pivot + 1, index))); - names.add(listFields.substring(0, pivot)); + String fieldName = listFields.substring(0, pivot); + if (encodedStructFields) { + if (!fieldName.startsWith("$")) { + throw new IllegalArgumentException( + "Encoded JNI struct field is missing its version marker"); + } + // The encoded producer removes every grammar delimiter from + // names before parsing, then restores the exact UTF-8 spelling. + fieldName = new String(Base64.getDecoder().decode(fieldName.substring(1)), + StandardCharsets.UTF_8); + } + fields.add(parseType(fieldName, + listFields.substring(pivot + 1, index), encodedStructFields)); + names.add(fieldName); listFields = listFields.substring(Math.min(index + 1, listFields.length())); } else { break; diff --git a/fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java b/fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java index a6951a22b0e7ad..931a45b8bc3c47 100644 --- a/fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java +++ b/fe/be-java-extensions/paimon-scanner/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java @@ -103,14 +103,17 @@ public class PaimonJniScanner extends JniScanner { public PaimonJniScanner(int batchSize, Map params) { this.classLoader = this.getClass().getClassLoader(); this.params = params; + boolean encodedSchema = usesEncodedSchema(params); String[] requiredFields = requiredFields(params); - String[] requiredTypes = splitParam(params.get("columns_types"), "#"); + String[] requiredTypes = requiredTypes(params); Preconditions.checkArgument(requiredFields.length == requiredTypes.length, "required_fields size %s does not match columns_types size %s", requiredFields.length, requiredTypes.length); ColumnType[] columnTypes = new ColumnType[requiredTypes.length]; for (int i = 0; i < requiredTypes.length; i++) { - columnTypes[i] = ColumnType.parseType(requiredFields[i], requiredTypes[i]); + columnTypes[i] = encodedSchema + ? ColumnType.parseTypeWithEncodedStructFields(requiredFields[i], requiredTypes[i]) + : ColumnType.parseType(requiredFields[i], requiredTypes[i]); } if (LOG.isDebugEnabled()) { // The raw map may carry storage or JDBC credentials; diagnostics must only use @@ -513,7 +516,31 @@ static String[] requiredFields(Map params) { } // Each identifier is encoded independently, so delimiters in quoted identifiers cannot // change field cardinality. The legacy parameter remains the rolling-upgrade fallback. - return Arrays.stream(encodedFields.split(",", -1)) + return decodeSchemaValues(encodedFields); + } + + static String[] requiredTypes(Map params) { + String encodedTypes = params.get("columns_types_base64"); + return encodedTypes == null + ? splitParam(params.get("columns_types"), "#") + : decodeSchemaValues(encodedTypes); + } + + private static boolean usesEncodedSchema(Map params) { + boolean hasFields = params.containsKey("required_fields_base64"); + boolean hasTypes = params.containsKey("columns_types_base64"); + // Both halves describe one schema version; accepting a mixed pair would reintroduce the + // cardinality and nested-name ambiguity this protocol is intended to remove. + Preconditions.checkArgument(hasFields == hasTypes, + "required_fields_base64 and columns_types_base64 must be provided together"); + return hasFields; + } + + private static String[] decodeSchemaValues(String encodedValues) { + if (encodedValues.isEmpty()) { + return new String[0]; + } + return Arrays.stream(encodedValues.split(",", -1)) .map(encoded -> new String(Base64.getDecoder().decode(encoded), StandardCharsets.UTF_8)) .toArray(String[]::new); } diff --git a/fe/be-java-extensions/paimon-scanner/src/test/java/org/apache/doris/paimon/PaimonJniScannerTest.java b/fe/be-java-extensions/paimon-scanner/src/test/java/org/apache/doris/paimon/PaimonJniScannerTest.java index 50e2979f8a7136..47228828ef9123 100644 --- a/fe/be-java-extensions/paimon-scanner/src/test/java/org/apache/doris/paimon/PaimonJniScannerTest.java +++ b/fe/be-java-extensions/paimon-scanner/src/test/java/org/apache/doris/paimon/PaimonJniScannerTest.java @@ -17,6 +17,8 @@ package org.apache.doris.paimon; +import org.apache.doris.common.jni.vec.ColumnType; + import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.core.LogEvent; @@ -70,6 +72,7 @@ public void testConstructorDecodesDelimiterSafeProjectionNames() { params.put("required_fields_base64", encodeFields( "region,code", "hash#name", "地区 名")); params.put("columns_types", "string#string#string"); + params.put("columns_types_base64", encodeFields("string", "string", "string")); PaimonJniScanner scanner = new PaimonJniScanner(128, params); @@ -80,6 +83,23 @@ public void testConstructorDecodesDelimiterSafeProjectionNames() { Arrays.asList("region,code", "hash#name", "地区 名"), "REGION,CODE")); } + @Test + public void testConstructorDecodesDelimiterSafeNestedFieldNamesAndTypes() { + Map params = createBaseParams(); + params.put("required_fields_base64", encodeFields("nested#value")); + params.put("columns_types", "legacy#type#framing"); + params.put("columns_types_base64", encodeFields( + "struct<$aGFzaCNuYW1l:string,$cmVnaW9uLGNvZGU:string,$Y29sb246bmFtZQ:string>")); + + InspectablePaimonJniScanner scanner = new InspectablePaimonJniScanner(128, params); + + Assert.assertEquals(1, scanner.requiredTypes().length); + ColumnType structType = scanner.requiredTypes()[0]; + Assert.assertTrue(structType.isStruct()); + Assert.assertEquals(Arrays.asList("hash#name", "region,code", "colon:name"), + structType.getChildNames()); + } + @Test public void testDebugSummaryNeverIncludesRawScannerParams() { Map params = createBaseParams(); @@ -431,6 +451,16 @@ public void append(LogEvent event) { } } + private static class InspectablePaimonJniScanner extends PaimonJniScanner { + InspectablePaimonJniScanner(int batchSize, Map params) { + super(batchSize, params); + } + + ColumnType[] requiredTypes() { + return types; + } + } + @Test public void testGetFieldIndexMatchesMixedCaseColumns() { Assert.assertEquals(1, PaimonJniScanner.getFieldIndex(Arrays.asList("data", "mIxEd_COL", "PART"), diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java index 73455ad2de9ff6..96411f0730c122 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java @@ -652,14 +652,21 @@ public void replayAlterCatalogProps(CatalogLog log, Map oldPrope CatalogIf catalog = idToCatalog.get(log.getCatalogId()); if (catalog instanceof ExternalCatalog) { Map newProps = log.getNewProps(); - ((ExternalCatalog) catalog).tryModifyCatalogProps(newProps); if (!isReplay) { + boolean tentativelyMutated = false; try { - ((ExternalCatalog) catalog).checkProperties(); + ExternalCatalog externalCatalog = (ExternalCatalog) catalog; + boolean validatedWithoutMutation = externalCatalog.validatePropertiesBeforeUpdate( + oldProperties, newProps); + if (!validatedWithoutMutation) { + externalCatalog.tryModifyCatalogProps(newProps); + tentativelyMutated = true; + externalCatalog.checkProperties(); + } } catch (Exception validationException) { - // Connector-specific validators may throw unchecked exceptions after the - // tentative mutation; every validation failure must preserve atomic ALTER. - if (oldProperties != null) { + // Only legacy validators publish a tentative candidate. Detached validators + // leave the live CatalogProperty untouched while concurrent initialization runs. + if (oldProperties != null && tentativelyMutated) { ((ExternalCatalog) catalog).rollBackCatalogProps(oldProperties); } if (validationException instanceof DdlException) { @@ -668,6 +675,8 @@ public void replayAlterCatalogProps(CatalogLog log, Map oldPrope throw new DdlException("Invalid catalog properties: " + validationException.getMessage(), validationException); } + } else { + ((ExternalCatalog) catalog).tryModifyCatalogProps(newProps); } if (newProps.containsKey(METADATA_REFRESH_INTERVAL_SEC)) { long catalogId = catalog.getId(); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java index 71b7a7f5344714..ce855994b766bd 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java @@ -470,8 +470,12 @@ private void buildMetaCache() { // check if all required properties are set when creating catalog public void checkProperties() throws DdlException { + checkProperties(catalogProperty); + } + + protected void checkProperties(CatalogProperty property) throws DdlException { // check refresh parameter of catalog - Map properties = catalogProperty.getProperties(); + Map properties = property.getProperties(); if (properties.containsKey(CatalogMgr.METADATA_REFRESH_INTERVAL_SEC)) { try { int metadataRefreshIntervalSec = Integer.parseInt( @@ -485,7 +489,7 @@ public void checkProperties() throws DdlException { } // check schema.cache.ttl-second parameter - String schemaCacheTtlSecond = catalogProperty.getOrDefault(SCHEMA_CACHE_TTL_SECOND, null); + String schemaCacheTtlSecond = property.getOrDefault(SCHEMA_CACHE_TTL_SECOND, null); if (java.util.Objects.nonNull(schemaCacheTtlSecond) && NumberUtils.toInt(schemaCacheTtlSecond, CACHE_NO_TTL) < CACHE_TTL_DISABLE_CACHE) { throw new DdlException( @@ -493,6 +497,16 @@ public void checkProperties() throws DdlException { } } + /** + * Validate an ALTER candidate without publishing it to this catalog. A true return value + * declares that the connector performed complete detached validation; false retains the + * legacy tentative-mutation path for connectors that have not implemented this contract. + */ + public boolean validatePropertiesBeforeUpdate( + Map currentProperties, Map updatedProperties) throws DdlException { + return false; + } + /** * eg: * ( diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java index 0760c9fd2544d9..a285ce79f83a34 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java @@ -36,6 +36,7 @@ import org.apache.paimon.table.Table; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -137,7 +138,11 @@ public Table getPaimonTable(NameMapping nameMapping, String branch, String query return executionAuthenticator.execute(() -> { Table table = catalog.getTable(identifier); Map tableOptions = paimonProperties.getTableOptionsForCopy(); - return tableOptions.isEmpty() ? table : table.copy(tableOptions); + Table effectiveTable = tableOptions.isEmpty() ? table : table.copy(tableOptions); + // Physical table options bypass Doris property validation, so validate the final + // merged view before Paimon can allocate batches or replace its manifest executor. + PaimonReaderOptions.validateEffectiveTableOptions(effectiveTable.options()); + return effectiveTable; }); } catch (Exception e) { throw new RuntimeException("Failed to get Paimon table:" + getName() + "." @@ -163,14 +168,32 @@ public Map getPaimonOptionsMap() { @Override public void checkProperties() throws DdlException { - super.checkProperties(); - CacheSpec.checkBooleanProperty(catalogProperty.getOrDefault(PAIMON_TABLE_CACHE_ENABLE, null), + checkProperties(catalogProperty, catalogProperty.getProperties()); + } + + @Override + public boolean validatePropertiesBeforeUpdate( + Map currentProperties, Map updatedProperties) throws DdlException { + Map candidateProperties = currentProperties == null + ? new HashMap<>() : new HashMap<>(currentProperties); + candidateProperties.putAll(updatedProperties); + checkProperties(new CatalogProperty(null, candidateProperties), updatedProperties); + return true; + } + + private void checkProperties(CatalogProperty property, Map strictlyValidatedProperties) + throws DdlException { + super.checkProperties(property); + CacheSpec.checkBooleanProperty(property.getOrDefault(PAIMON_TABLE_CACHE_ENABLE, null), PAIMON_TABLE_CACHE_ENABLE); - CacheSpec.checkLongProperty(catalogProperty.getOrDefault(PAIMON_TABLE_CACHE_TTL_SECOND, null), + CacheSpec.checkLongProperty(property.getOrDefault(PAIMON_TABLE_CACHE_TTL_SECOND, null), -1L, PAIMON_TABLE_CACHE_TTL_SECOND); - CacheSpec.checkLongProperty(catalogProperty.getOrDefault(PAIMON_TABLE_CACHE_CAPACITY, null), + CacheSpec.checkLongProperty(property.getOrDefault(PAIMON_TABLE_CACHE_CAPACITY, null), 0L, PAIMON_TABLE_CACHE_CAPACITY); - catalogProperty.checkMetaStoreAndStorageProperties(AbstractPaimonProperties.class); + // Validate only newly supplied dynamic options on ALTER. This lets an old image containing + // a formerly accepted option survive an unrelated update while still rejecting new writes. + PaimonReaderOptions.validateCatalogProperties(strictlyValidatedProperties); + property.checkMetaStoreAndStorageProperties(AbstractPaimonProperties.class); } @Override diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java index 8adab5b39d7038..49ff11cc8f793e 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java @@ -24,10 +24,14 @@ import org.apache.paimon.options.Options; 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; @@ -66,6 +70,62 @@ public static void validate(String key, String value) { } } + public static void validateCatalogProperties(Map 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 compatibleCatalogOptions(Map properties) { + Map 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 options) { + SUPPORTED_OPTIONS.stream() + .filter(options::containsKey) + .forEach(key -> validate(key, options.get(key))); + validateManifestParallelism(options.get(CoreOptions.SCAN_MANIFEST_PARALLELISM.key())); + } + + private static void validateManifestParallelism(String value) { + if (value == null) { + return; + } + int parallelism; + try { + parallelism = Integer.parseInt(value); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Invalid value for Paimon option '" + + CoreOptions.SCAN_MANIFEST_PARALLELISM.key() + "': " + value, e); + } + int maximum = Runtime.getRuntime().availableProcessors(); + // Paimon replaces a JVM-static manifest executor above its CPU-sized default, so every + // option source must stay within that capacity to avoid cross-query thread-pool mutation. + requireRange(CoreOptions.SCAN_MANIFEST_PARALLELISM.key(), parallelism, 1, maximum); + } + private static T parse(String key, String value, ConfigOption option) { try { return new Options(Collections.singletonMap(key, value)).get(option); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java index e28db5be64e6e2..d41dec9a349595 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java @@ -103,10 +103,7 @@ public static void validateOptions(Map options) { throw new IllegalArgumentException("Unsupported Paimon query option(s): " + unsupported); } - PaimonReaderOptions.supportedOptions().stream() - .filter(options::containsKey) - .forEach(key -> PaimonReaderOptions.validate(key, options.get(key))); - validateManifestParallelism(options.get(CoreOptions.SCAN_MANIFEST_PARALLELISM.key())); + PaimonReaderOptions.validateEffectiveTableOptions(options); String scanMode = options.get(CoreOptions.SCAN_MODE.key()); if ("from-creation-timestamp".equalsIgnoreCase(scanMode) @@ -136,27 +133,6 @@ public static void validateOptions(Map options) { } } - private static void validateManifestParallelism(String value) { - if (value == null) { - return; - } - int parallelism; - try { - parallelism = Integer.parseInt(value); - } catch (NumberFormatException e) { - throw new IllegalArgumentException("Invalid value for Paimon query option '" - + CoreOptions.SCAN_MANIFEST_PARALLELISM.key() + "': " + value, e); - } - int maximum = Runtime.getRuntime().availableProcessors(); - // Paimon replaces a JVM-static manifest executor when this value exceeds its CPU-sized - // default. Keeping the query value within that capacity prevents cross-query mutation. - if (parallelism < 1 || parallelism > maximum) { - throw new IllegalArgumentException("Paimon query option '" - + CoreOptions.SCAN_MANIFEST_PARALLELISM.key() + "' must be between 1 and " - + maximum + ", but was " + parallelism); - } - } - public static Table applyOptions(Table table, Map options) { Map tableOptions = userOptions(options); validateOptions(tableOptions); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractPaimonProperties.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractPaimonProperties.java index 8095d9e883d000..5ea5a156a0d948 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractPaimonProperties.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractPaimonProperties.java @@ -32,7 +32,6 @@ import java.util.Collections; import java.util.HashMap; -import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -59,7 +58,7 @@ public abstract class AbstractPaimonProperties extends MetastoreProperties { private static final String USER_PROPERTY_PREFIX = "paimon."; private static final String DORIS_JNI_PROPERTY_PREFIX = "paimon.jni."; /** The suffix after this prefix is passed to Paimon as a dynamic table option. */ - public static final String TABLE_OPTION_PREFIX = "paimon.table-option."; + public static final String TABLE_OPTION_PREFIX = PaimonReaderOptions.TABLE_OPTION_PREFIX; private Map tableOptionsMap = Collections.emptyMap(); @@ -156,28 +155,7 @@ public static boolean isTableOptionProperty(String key) { } private Map extractTableOptions() { - Map tableOptions = new LinkedHashMap<>(); - origProps.forEach((key, value) -> { - if (isTableOptionProperty(key)) { - String tableOptionKey = key.substring(TABLE_OPTION_PREFIX.length()); - if (StringUtils.isBlank(tableOptionKey)) { - throw new IllegalArgumentException( - "Paimon table option name must not be empty after prefix " + TABLE_OPTION_PREFIX); - } - validateTableOption(tableOptionKey, value); - tableOptions.put(tableOptionKey, value); - } - }); - return Collections.unmodifiableMap(tableOptions); - } - - private void validateTableOption(String key, String value) { - try { - PaimonReaderOptions.validate(key, value); - } catch (IllegalArgumentException e) { - throw new IllegalArgumentException("Invalid value for Paimon table option '" + key + "': " - + e.getMessage(), e); - } + return PaimonReaderOptions.compatibleCatalogOptions(origProps); } /** diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogMgrTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogMgrTest.java index da55fa69464eff..48c447342fc949 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogMgrTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogMgrTest.java @@ -20,6 +20,8 @@ import org.apache.doris.catalog.DatabaseIf; import org.apache.doris.catalog.TableIf; import org.apache.doris.common.DdlException; +import org.apache.doris.datasource.paimon.PaimonExternalCatalog; +import org.apache.doris.datasource.property.metastore.AbstractPaimonProperties; import com.google.common.collect.ImmutableMap; import org.junit.jupiter.api.Assertions; @@ -27,11 +29,29 @@ import org.mockito.Mockito; import java.lang.reflect.Field; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; public class CatalogMgrTest { + private static void addCatalog(CatalogMgr catalogMgr, ExternalCatalog catalog) throws Exception { + Field idToCatalogField = CatalogMgr.class.getDeclaredField("idToCatalog"); + idToCatalogField.setAccessible(true); + @SuppressWarnings("unchecked") + ConcurrentMap>> idToCatalog = + (ConcurrentMap>>) + idToCatalogField.get(catalogMgr); + idToCatalog.put(catalog.getId(), catalog); + } + @Test void testAlterCatalogRollsBackUncheckedValidationFailure() throws Exception { CatalogMgr catalogMgr = new CatalogMgr(); @@ -39,13 +59,7 @@ void testAlterCatalogRollsBackUncheckedValidationFailure() throws Exception { long catalogId = 42L; Mockito.when(catalog.getId()).thenReturn(catalogId); - Field idToCatalogField = CatalogMgr.class.getDeclaredField("idToCatalog"); - idToCatalogField.setAccessible(true); - @SuppressWarnings("unchecked") - ConcurrentMap>> idToCatalog = - (ConcurrentMap>>) - idToCatalogField.get(catalogMgr); - idToCatalog.put(catalogId, catalog); + addCatalog(catalogMgr, catalog); Map oldProperties = ImmutableMap.of("read.batch-size", "1024"); Map newProperties = ImmutableMap.of("read.batch-size", "0"); @@ -63,4 +77,115 @@ void testAlterCatalogRollsBackUncheckedValidationFailure() throws Exception { Mockito.verify(catalog).rollBackCatalogProps(oldProperties); Mockito.verify(catalog, Mockito.never()).modifyCatalogProps(newProperties); } + + @Test + void testDetachedValidationNeverPublishesCandidateToConcurrentInitialization() throws Exception { + CatalogMgr catalogMgr = new CatalogMgr(); + Map oldProperties = ImmutableMap.of("read.batch-size", "1024"); + Map newProperties = ImmutableMap.of( + "read.batch-size", "4096", + CatalogMgr.METADATA_REFRESH_INTERVAL_SEC, "invalid"); + LatchingValidationCatalog catalog = new LatchingValidationCatalog(43L, oldProperties); + addCatalog(catalogMgr, catalog); + CatalogLog log = new CatalogLog(); + log.setCatalogId(catalog.getId()); + log.setNewProps(newProperties); + ExecutorService executor = Executors.newSingleThreadExecutor(); + + try { + Future alterResult = executor.submit(() -> { + try { + catalogMgr.replayAlterCatalogProps(log, oldProperties, false); + return null; + } catch (DdlException e) { + return e; + } + }); + Assertions.assertTrue(catalog.validationStarted.await(10, TimeUnit.SECONDS)); + + Assertions.assertThrows(RuntimeException.class, catalog::makeSureInitialized); + DdlException validationFailure = alterResult.get(10, TimeUnit.SECONDS); + + Assertions.assertNotNull(validationFailure); + Assertions.assertEquals(oldProperties, catalog.propertiesSeenByInitialization); + Assertions.assertEquals(oldProperties, catalog.getProperties()); + } finally { + executor.shutdownNow(); + } + } + + @Test + void testReplayKeepsPersistedLegacyPaimonOptionLoadableButInactive() throws Exception { + CatalogMgr catalogMgr = new CatalogMgr(); + Map persistedProperties = new HashMap<>(); + persistedProperties.put("type", "paimon"); + persistedProperties.put("paimon.catalog.type", "filesystem"); + persistedProperties.put("warehouse", "s3://example-bucket/warehouse"); + persistedProperties.put("paimon.table-option.write.batch-size", "2048"); + ReplayCompatiblePaimonCatalog catalog = new ReplayCompatiblePaimonCatalog(44L, persistedProperties); + addCatalog(catalogMgr, catalog); + CatalogLog log = new CatalogLog(); + log.setCatalogId(catalog.getId()); + log.setNewProps(ImmutableMap.of(ExternalCatalog.USE_META_CACHE, "false")); + + catalogMgr.replayAlterCatalogProps(log, persistedProperties, true); + + AbstractPaimonProperties restoredProperties = (AbstractPaimonProperties) + catalog.getCatalogProperty().getMetastoreProperties(); + Assertions.assertEquals("2048", + catalog.getProperties().get("paimon.table-option.write.batch-size")); + Assertions.assertTrue(restoredProperties.getTableOptionsMap().isEmpty()); + } + + private static class LatchingValidationCatalog extends ExternalCatalog { + private final CountDownLatch validationStarted = new CountDownLatch(1); + private final CountDownLatch initializationReadProperties = new CountDownLatch(1); + private volatile Map propertiesSeenByInitialization; + + LatchingValidationCatalog(long id, Map properties) { + super(id, "latching_catalog", InitCatalogLog.Type.TEST, ""); + catalogProperty = new CatalogProperty(null, properties); + } + + @Override + public boolean validatePropertiesBeforeUpdate( + Map currentProperties, Map updatedProperties) { + validationStarted.countDown(); + try { + Assertions.assertTrue(initializationReadProperties.await(10, TimeUnit.SECONDS)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException(e); + } + throw new IllegalArgumentException("invalid candidate properties"); + } + + @Override + protected void initLocalObjectsImpl() { + propertiesSeenByInitialization = getProperties(); + initializationReadProperties.countDown(); + throw new IllegalStateException("stop after observing properties"); + } + + @Override + protected List listTableNamesFromRemote(SessionContext ctx, String dbName) { + return Collections.emptyList(); + } + + @Override + public boolean tableExist(SessionContext ctx, String dbName, String tblName) { + return false; + } + } + + private static class ReplayCompatiblePaimonCatalog extends PaimonExternalCatalog { + ReplayCompatiblePaimonCatalog(long id, Map properties) { + super(id, "persisted_paimon_catalog", null, properties, ""); + } + + @Override + public void notifyPropertiesUpdated(Map updatedProps) { + // This test isolates edit-log property restoration from environment-owned cache services. + } + } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java new file mode 100644 index 00000000000000..dc9bdcd0f53107 --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java @@ -0,0 +1,56 @@ +// 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 org.apache.doris.datasource.property.metastore.AbstractPaimonProperties; + +import com.google.common.collect.ImmutableMap; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.Map; + +public class PaimonReaderOptionsTest { + + @Test + void testRejectUnsafeOptionsFromCreateOrAlterProperties() { + for (Map properties : new Map[] { + ImmutableMap.of(AbstractPaimonProperties.TABLE_OPTION_PREFIX + "branch", "archive"), + ImmutableMap.of(AbstractPaimonProperties.TABLE_OPTION_PREFIX + "read.batch-size", "0"), + ImmutableMap.of(AbstractPaimonProperties.TABLE_OPTION_PREFIX + + "file-reader-async-threshold", "2 GB") + }) { + Assertions.assertThrows(IllegalArgumentException.class, + () -> PaimonReaderOptions.validateCatalogProperties(properties)); + } + } + + @Test + void testRejectUnsafeEffectivePhysicalTableOptions() { + for (Map options : new Map[] { + ImmutableMap.of("read.batch-size", "0"), + ImmutableMap.of("file-reader-async-threshold", "512 KB"), + ImmutableMap.of("scan.manifest.parallelism", "0"), + ImmutableMap.of("scan.manifest.parallelism", + String.valueOf(Runtime.getRuntime().availableProcessors() + 1)) + }) { + Assertions.assertThrows(IllegalArgumentException.class, + () -> PaimonReaderOptions.validateEffectiveTableOptions(options)); + } + } +} diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/property/metastore/AbstractPaimonPropertiesTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/property/metastore/AbstractPaimonPropertiesTest.java index edb4f8b58dced3..ca632484801a66 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/property/metastore/AbstractPaimonPropertiesTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/property/metastore/AbstractPaimonPropertiesTest.java @@ -17,7 +17,9 @@ package org.apache.doris.datasource.property.metastore; +import org.apache.doris.datasource.CatalogProperty; import org.apache.doris.datasource.storage.StorageAdapter; +import org.apache.doris.persist.gson.GsonUtils; import org.apache.paimon.catalog.Catalog; import org.junit.jupiter.api.Assertions; @@ -134,46 +136,43 @@ void testCatalogTableOptionsFillMissingPaimonTableOptions() { } @Test - void testRejectUnknownTableOption() { + void testPersistedUnknownTableOptionDoesNotPreventCatalogLoading() { Map input = new HashMap<>(); input.put("warehouse", "s3://tmp/warehouse"); input.put("paimon.table-option.option-does-not-exist", "value"); TestPaimonProperties testProps = new TestPaimonProperties(input); - IllegalArgumentException exception = Assertions.assertThrows( - IllegalArgumentException.class, testProps::initNormalizeAndCheckProps); + testProps.initNormalizeAndCheckProps(); - Assertions.assertTrue(exception.getMessage().contains("option-does-not-exist")); + Assertions.assertTrue(testProps.getTableOptionsMap().isEmpty()); } @Test - void testRejectPrefixMapTableOption() { + void testPersistedPrefixMapTableOptionDoesNotPreventCatalogLoading() { Map input = new HashMap<>(); input.put("warehouse", "s3://tmp/warehouse"); input.put("paimon.table-option.file.compression.per.level.0", "lz4"); TestPaimonProperties testProps = new TestPaimonProperties(input); - IllegalArgumentException exception = Assertions.assertThrows( - IllegalArgumentException.class, testProps::initNormalizeAndCheckProps); + testProps.initNormalizeAndCheckProps(); - Assertions.assertTrue(exception.getMessage().contains("file.compression.per.level.0")); + Assertions.assertTrue(testProps.getTableOptionsMap().isEmpty()); } @Test - void testRejectInvalidTableOptionValue() { + void testPersistedInvalidReaderOptionIsIgnoredDuringCatalogLoading() { Map input = new HashMap<>(); input.put("warehouse", "s3://tmp/warehouse"); input.put("paimon.table-option.read.batch-size", "not-an-integer"); TestPaimonProperties testProps = new TestPaimonProperties(input); - IllegalArgumentException exception = Assertions.assertThrows( - IllegalArgumentException.class, testProps::initNormalizeAndCheckProps); + testProps.initNormalizeAndCheckProps(); - Assertions.assertTrue(exception.getMessage().contains("read.batch-size")); + Assertions.assertTrue(testProps.getTableOptionsMap().isEmpty()); } @Test - void testRejectUnsafeTableOptions() { + void testPersistedUnsafeTableOptionsAreIgnoredDuringCatalogLoading() { for (String option : new String[] { "branch", "path", "scan.tag-name", "scan.snapshot-id", "write.batch-size", "file.compression.per.level" @@ -182,16 +181,14 @@ void testRejectUnsafeTableOptions() { input.put("warehouse", "s3://tmp/warehouse"); input.put("paimon.table-option." + option, "1"); - IllegalArgumentException exception = Assertions.assertThrows( - IllegalArgumentException.class, - () -> new TestPaimonProperties(input).initNormalizeAndCheckProps(), - option); - Assertions.assertTrue(exception.getMessage().contains(option), option); + TestPaimonProperties testProps = new TestPaimonProperties(input); + testProps.initNormalizeAndCheckProps(); + Assertions.assertTrue(testProps.getTableOptionsMap().isEmpty(), option); } } @Test - void testRejectOutOfRangeReaderOptions() { + void testPersistedOutOfRangeReaderOptionsAreIgnoredDuringCatalogLoading() { Map invalidOptions = new HashMap<>(); invalidOptions.put("read.batch-size", "0"); invalidOptions.put("read.batch-size-negative", "-1"); @@ -207,14 +204,27 @@ void testRejectOutOfRangeReaderOptions() { input.put("warehouse", "s3://tmp/warehouse"); input.put("paimon.table-option." + option, value); - IllegalArgumentException exception = Assertions.assertThrows( - IllegalArgumentException.class, - () -> new TestPaimonProperties(input).initNormalizeAndCheckProps(), - caseName); - Assertions.assertTrue(exception.getMessage().contains(option), caseName); + TestPaimonProperties testProps = new TestPaimonProperties(input); + testProps.initNormalizeAndCheckProps(); + Assertions.assertTrue(testProps.getTableOptionsMap().isEmpty(), caseName); }); } + @Test + void testImageRoundTripKeepsLegacyCatalogLoadableButDoesNotApplyUnsafeOption() { + Map input = new HashMap<>(); + input.put("warehouse", "s3://tmp/warehouse"); + input.put("paimon.table-option.write.batch-size", "2048"); + CatalogProperty persisted = new CatalogProperty(null, input); + + CatalogProperty restored = GsonUtils.GSON.fromJson( + GsonUtils.GSON.toJson(persisted), CatalogProperty.class); + TestPaimonProperties testProps = new TestPaimonProperties(restored.getProperties()); + testProps.initNormalizeAndCheckProps(); + + Assertions.assertTrue(testProps.getTableOptionsMap().isEmpty()); + } + @Test void testForwardTableDefaultOptionsToPaimonCatalog() { Map input = new HashMap<>(); diff --git a/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy b/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy index 038be85bc29d33..b4b3391a6fff79 100644 --- a/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy +++ b/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy @@ -25,7 +25,10 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { String minioPort = context.config.otherConfigs.get("iceberg_minio_port") String externalEnvIp = context.config.otherConfigs.get("externalEnvIp") String catalogName = "test_paimon_jni_reader_guardrails" + String physicalCatalogName = "test_paimon_jni_reader_physical_guardrails" String dbName = "paimon_jni_guardrails_db" + def scannerV2Rows = sql "show variables like 'enable_file_scanner_v2'" + String originalScannerV2 = scannerV2Rows[0][1] def catalogDdl = { String name, String extraProperty -> return """ @@ -72,12 +75,23 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { `region,code` string, `hash#name` string, `display name` string, - `地区 名` string + `地区 名` string, + `nested#value` struct<`hash#name`:string,`region,code`:string,`colon:name`:string> ) using paimon tblproperties ('file.format'='parquet', 'read.batch-size'='512'); insert into paimon.${dbName}.quoted_reader_options values - (1, 'east,01', 'hash-one', 'first row', '华东'), - (2, 'west,02', 'hash-two', 'second row', '华西'); + (1, 'east,01', 'hash-one', 'first row', '华东', + named_struct('hash#name', 'nested-one', 'region,code', 'east,01', 'colon:name', 'a:1')), + (2, 'west,02', 'hash-two', 'second row', '华西', + named_struct('hash#name', 'nested-two', 'region,code', 'west,02', 'colon:name', 'b:2')); + drop table if exists paimon.${dbName}.unsafe_physical_batch; + create table paimon.${dbName}.unsafe_physical_batch (id int) using paimon + tblproperties ('read.batch-size'='0'); + insert into paimon.${dbName}.unsafe_physical_batch values (1); + drop table if exists paimon.${dbName}.unsafe_physical_manifest; + create table paimon.${dbName}.unsafe_physical_manifest (id int) using paimon + tblproperties ('scan.manifest.parallelism'='0'); + insert into paimon.${dbName}.unsafe_physical_manifest values (1); """ sql "switch ${catalogName}" @@ -86,13 +100,30 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { test { sql """ - select `region,code`, `hash#name`, `display name`, `地区 名` + select `region,code`, `hash#name`, `display name`, `地区 名`, `nested#value` from quoted_reader_options where `region,code` in ('east,01', 'west,02') order by id """ } + sql "set enable_file_scanner_v2=false" + test { + sql """ + select `region,code`, `nested#value` + from quoted_reader_options + order by id + """ + } + sql "set enable_file_scanner_v2=true" + test { + sql """ + select `region,code`, `nested#value` + from quoted_reader_options + order by id + """ + } + test { sql """ select id from quoted_reader_options@options( @@ -140,8 +171,23 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { test { sql "select count(*) from quoted_reader_options" } + + sql "drop catalog if exists ${physicalCatalogName}" + sql(catalogDdl(physicalCatalogName, "")) + sql "switch ${physicalCatalogName}" + sql "use ${dbName}" + test { + sql "select * from unsafe_physical_batch" + exception "read.batch-size" + } + test { + sql "select * from unsafe_physical_manifest" + exception "scan.manifest.parallelism" + } } finally { sql "set force_jni_scanner=false" + sql "set enable_file_scanner_v2=${originalScannerV2}" + sql "drop catalog if exists ${physicalCatalogName}" sql "drop catalog if exists ${catalogName}" } } From 918e48adea19efc2d495c94ee543f55000d58e80 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Thu, 30 Jul 2026 09:54:18 +0800 Subject: [PATCH 3/6] [fix](paimon) preserve schema arity and option precedence --- be/src/format/jni/jni_data_bridge.cpp | 3 ++- .../format_v2/jni/jni_table_reader_test.cpp | 4 ++- .../apache/doris/paimon/PaimonJniScanner.java | 7 ++++- .../doris/paimon/PaimonJniScannerTest.java | 18 ++++++++++++- .../paimon/PaimonExternalCatalog.java | 8 +++--- .../datasource/paimon/PaimonScanParams.java | 6 ++++- .../paimon/source/PaimonScanNode.java | 20 ++++++++++---- .../paimon/PaimonExternalTableTest.java | 2 ++ .../paimon/PaimonReaderOptionsTest.java | 26 +++++++++++++++++++ .../paimon/PaimonScanParamsTest.java | 12 +++++++-- .../paimon/source/PaimonScanNodeTest.java | 23 ++++++++++++++++ .../test_paimon_jni_reader_guardrails.groovy | 20 ++++++++++++++ 12 files changed, 132 insertions(+), 17 deletions(-) diff --git a/be/src/format/jni/jni_data_bridge.cpp b/be/src/format/jni/jni_data_bridge.cpp index 8fa1b894233ec4..9dc935e0b62da5 100644 --- a/be/src/format/jni/jni_data_bridge.cpp +++ b/be/src/format/jni/jni_data_bridge.cpp @@ -498,7 +498,8 @@ std::string JniDataBridge::encode_schema_values(const std::vector& for (const auto& value : values) { std::string encoded; base64_encode(value, &encoded); - encoded_values.emplace_back(std::move(encoded)); + // Prefix every element so an empty Base64 token remains distinct from an empty list. + encoded_values.emplace_back("$" + encoded); } return join(encoded_values, ","); } diff --git a/be/test/format_v2/jni/jni_table_reader_test.cpp b/be/test/format_v2/jni/jni_table_reader_test.cpp index eb8ba25027967d..68567675473ae5 100644 --- a/be/test/format_v2/jni/jni_table_reader_test.cpp +++ b/be/test/format_v2/jni/jni_table_reader_test.cpp @@ -114,7 +114,9 @@ Status init_reader(FakeJniTableReader* reader, const std::shared_ptr new String(Base64.getDecoder().decode(encoded), StandardCharsets.UTF_8)) + .map(encoded -> { + // A marker on every token preserves list arity when the encoded value itself is empty. + Preconditions.checkArgument(encoded.startsWith("$"), + "Encoded JNI schema token is missing its version marker"); + return new String(Base64.getDecoder().decode(encoded.substring(1)), StandardCharsets.UTF_8); + }) .toArray(String[]::new); } diff --git a/fe/be-java-extensions/paimon-scanner/src/test/java/org/apache/doris/paimon/PaimonJniScannerTest.java b/fe/be-java-extensions/paimon-scanner/src/test/java/org/apache/doris/paimon/PaimonJniScannerTest.java index 47228828ef9123..e04bb4375675e1 100644 --- a/fe/be-java-extensions/paimon-scanner/src/test/java/org/apache/doris/paimon/PaimonJniScannerTest.java +++ b/fe/be-java-extensions/paimon-scanner/src/test/java/org/apache/doris/paimon/PaimonJniScannerTest.java @@ -100,6 +100,22 @@ public void testConstructorDecodesDelimiterSafeNestedFieldNamesAndTypes() { structType.getChildNames()); } + @Test + public void testConstructorDistinguishesEmptyIdentifierFromEmptyProjection() { + Map oneEmptyField = createBaseParams(); + oneEmptyField.put("required_fields_base64", "$"); + oneEmptyField.put("columns_types_base64", "$c3RyaW5n"); + + new PaimonJniScanner(128, oneEmptyField); + + Assert.assertArrayEquals(new String[] {""}, PaimonJniScanner.requiredFields(oneEmptyField)); + Map noFields = createBaseParams(); + noFields.put("required_fields_base64", ""); + noFields.put("columns_types_base64", ""); + Assert.assertEquals(0, PaimonJniScanner.requiredFields(noFields).length); + new PaimonJniScanner(128, noFields); + } + @Test public void testDebugSummaryNeverIncludesRawScannerParams() { Map params = createBaseParams(); @@ -361,7 +377,7 @@ private Map createBaseParams() { private String encodeFields(String... fields) { return Arrays.stream(fields) - .map(field -> Base64.getEncoder().encodeToString(field.getBytes(StandardCharsets.UTF_8))) + .map(field -> "$" + Base64.getEncoder().encodeToString(field.getBytes(StandardCharsets.UTF_8))) .collect(java.util.stream.Collectors.joining(",")); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java index a285ce79f83a34..365826afefb0d3 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java @@ -138,11 +138,9 @@ public Table getPaimonTable(NameMapping nameMapping, String branch, String query return executionAuthenticator.execute(() -> { Table table = catalog.getTable(identifier); Map tableOptions = paimonProperties.getTableOptionsForCopy(); - Table effectiveTable = tableOptions.isEmpty() ? table : table.copy(tableOptions); - // Physical table options bypass Doris property validation, so validate the final - // merged view before Paimon can allocate batches or replace its manifest executor. - PaimonReaderOptions.validateEffectiveTableOptions(effectiveTable.options()); - return effectiveTable; + // Relation options are applied after this cached handle is returned. Defer final + // validation so a safe relation value can override an unsafe physical value. + return tableOptions.isEmpty() ? table : table.copy(tableOptions); }); } catch (Exception e) { throw new RuntimeException("Failed to get Paimon table:" + getName() + "." diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java index d41dec9a349595..8c87a9c20717d0 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java @@ -144,7 +144,11 @@ public static Table applyOptions(Table table, Map options) { .filter(key -> !tableOptions.containsKey(key)) .forEach(key -> isolatedOptions.put(key, null)); } - return table.copy(isolatedOptions); + Table effectiveTable = table.copy(isolatedOptions); + // Validate after every copy so relation options participate in the documented + // relation > catalog > physical precedence before the effective value is judged. + PaimonReaderOptions.validateEffectiveTableOptions(effectiveTable.options()); + return effectiveTable; } private static Set inheritedReadStateKeys() { diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java index 39e8cfcfbf0185..78c1d55f0ad500 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java @@ -32,6 +32,7 @@ import org.apache.doris.datasource.credentials.CredentialUtils; import org.apache.doris.datasource.credentials.VendedCredentialsFactory; import org.apache.doris.datasource.paimon.PaimonExternalCatalog; +import org.apache.doris.datasource.paimon.PaimonReaderOptions; import org.apache.doris.datasource.paimon.PaimonScanParams; import org.apache.doris.datasource.paimon.PaimonSysExternalTable; import org.apache.doris.datasource.paimon.PaimonUtil; @@ -995,18 +996,27 @@ private Table getProcessedTable() throws UserException { throw new UserException("Can not specify scan params and table snapshot at same time."); } + Table finalTable; if (theScanParams != null && theScanParams.incrementalRead()) { // System table handles are cached, so preserve query isolation by applying dynamic // options to a copied Paimon table instead of changing the shared handle. - return baseTable.copy(getIncrReadParams()); - } - if (theScanParams != null && theScanParams.isOptions()) { + finalTable = baseTable.copy(getIncrReadParams()); + } else if (theScanParams != null && theScanParams.isOptions()) { try { - return source.getPaimonTable(theScanParams); + finalTable = source.getPaimonTable(theScanParams); } catch (IllegalArgumentException e) { throw new UserException(e.getMessage(), e); } + } else { + finalTable = baseTable; + } + try { + // This is the last common boundary before planning and serialization, including scans + // with no relation copy and incremental/system-table paths that bypass applyOptions. + PaimonReaderOptions.validateEffectiveTableOptions(finalTable.options()); + } catch (IllegalArgumentException e) { + throw new UserException(e.getMessage(), e); } - return baseTable; + return finalTable; } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java index 6b0116433514f9..da72285aa7dbb9 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java @@ -51,6 +51,7 @@ public void testSelectorFreeOptionsPreserveStatementSnapshot() { Mockito.doReturn(statementTable).when(externalTable).getPaimonTable(statementSnapshot); Mockito.when(statementTable.copy(scanParams.getMapParams())).thenReturn(statementCopy); + Mockito.when(statementCopy.options()).thenReturn(scanParams.getMapParams()); Mockito.when(baseTable.copy(scanParams.getMapParams())).thenReturn(baseCopy); try (MockedStatic mvccUtil = Mockito.mockStatic(MvccUtil.class); @@ -79,6 +80,7 @@ public void testModeOnlyLatestUsesStatementSnapshotAcrossPhases() { Mockito.doReturn(statementTable).when(externalTable).getPaimonTable(statementSnapshot); Mockito.when(statementTable.options()).thenReturn(ImmutableMap.of("scan.snapshot-id", "7")); Mockito.when(baseTable.copy(ArgumentMatchers.anyMap())).thenReturn(pinnedCopy); + Mockito.when(pinnedCopy.options()).thenReturn(ImmutableMap.of("scan.snapshot-id", "7")); try (MockedStatic mvccUtil = Mockito.mockStatic(MvccUtil.class); MockedStatic paimonUtils = Mockito.mockStatic(PaimonUtils.class)) { diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java index dc9bdcd0f53107..0319f11c4b755f 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java @@ -20,9 +20,12 @@ import org.apache.doris.datasource.property.metastore.AbstractPaimonProperties; import com.google.common.collect.ImmutableMap; +import org.apache.paimon.table.Table; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import org.mockito.Mockito; +import java.util.Collections; import java.util.Map; public class PaimonReaderOptionsTest { @@ -53,4 +56,27 @@ void testRejectUnsafeEffectivePhysicalTableOptions() { () -> PaimonReaderOptions.validateEffectiveTableOptions(options)); } } + + @Test + void testRejectUnsafeOptionOnlyAfterFinalTableCopy() { + Table physicalTable = Mockito.mock(Table.class); + Table finalTable = Mockito.mock(Table.class); + Mockito.when(physicalTable.copy(Collections.emptyMap())).thenReturn(finalTable); + Mockito.when(finalTable.options()).thenReturn(ImmutableMap.of("read.batch-size", "0")); + + Assertions.assertThrows(IllegalArgumentException.class, + () -> PaimonScanParams.applyOptions(physicalTable, Collections.emptyMap())); + } + + @Test + void testSafeRelationOptionOverridesUnsafePhysicalOption() { + Table physicalTable = Mockito.mock(Table.class); + Table finalTable = Mockito.mock(Table.class); + Map relationOptions = ImmutableMap.of("read.batch-size", "4096"); + Mockito.when(physicalTable.options()).thenReturn(ImmutableMap.of("read.batch-size", "0")); + Mockito.when(physicalTable.copy(relationOptions)).thenReturn(finalTable); + Mockito.when(finalTable.options()).thenReturn(relationOptions); + + Assertions.assertSame(finalTable, PaimonScanParams.applyOptions(physicalTable, relationOptions)); + } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonScanParamsTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonScanParamsTest.java index 6da81b8082aa02..735dfdfc92c4c8 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonScanParamsTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonScanParamsTest.java @@ -82,6 +82,9 @@ public void testRelationReaderOptionsAreAppliedWithoutMutatingBaseTable() { Table table = Mockito.mock(Table.class); Table copied = Mockito.mock(Table.class); Mockito.when(table.copy(ArgumentMatchers.anyMap())).thenReturn(copied); + Mockito.when(copied.options()).thenReturn(ImmutableMap.of( + "read.batch-size", "8192", + "file-reader-async-threshold", "32 MB")); Assert.assertSame(copied, PaimonScanParams.applyOptions(table, ImmutableMap.of( "read.batch-size", "8192", @@ -145,6 +148,7 @@ public void testApplyPositionClearsInheritedStartupState() { Table table = Mockito.mock(Table.class); Table copied = Mockito.mock(Table.class); Mockito.when(table.copy(ArgumentMatchers.anyMap())).thenReturn(copied); + Mockito.when(copied.options()).thenReturn(Collections.emptyMap()); Assert.assertSame(copied, PaimonScanParams.applyOptions( table, ImmutableMap.of("scan.creation-time-millis", "1000"))); @@ -169,7 +173,9 @@ && containsNull(applied, "incremental-between-scan-mode") @Test public void testApplyModeClearsInheritedPositions() { Table table = Mockito.mock(Table.class); - Mockito.when(table.copy(ArgumentMatchers.anyMap())).thenReturn(Mockito.mock(Table.class)); + Table copied = Mockito.mock(Table.class); + Mockito.when(table.copy(ArgumentMatchers.anyMap())).thenReturn(copied); + Mockito.when(copied.options()).thenReturn(Collections.emptyMap()); PaimonScanParams.applyOptions(table, ImmutableMap.of("scan.mode", "latest")); @@ -188,7 +194,9 @@ && containsNull(applied, "scan.file-creation-time-millis") @Test public void testIsolationClearsFallbackReadStateKeys() { Table table = Mockito.mock(Table.class); - Mockito.when(table.copy(ArgumentMatchers.anyMap())).thenReturn(Mockito.mock(Table.class)); + Table copied = Mockito.mock(Table.class); + Mockito.when(table.copy(ArgumentMatchers.anyMap())).thenReturn(copied); + Mockito.when(copied.options()).thenReturn(Collections.emptyMap()); PaimonScanParams.applyOptions(table, ImmutableMap.of("scan.snapshot-id", "1")); diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/source/PaimonScanNodeTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/source/PaimonScanNodeTest.java index b520573de6ad8b..15ac70ca6daeea 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/source/PaimonScanNodeTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/source/PaimonScanNodeTest.java @@ -630,6 +630,7 @@ public void testSystemTablePassesIncrementalOptionsToPaimonTable() throws Except expectedOptions.put("incremental-to-auto-tag", null); expectedOptions.put("incremental-between", "1,2"); Mockito.when(baseTable.copy(expectedOptions)).thenReturn(copiedTable); + Mockito.when(copiedTable.options()).thenReturn(Collections.emptyMap()); try { Assert.assertSame(copiedTable, invokePrivateMethod(node, "getProcessedTable")); @@ -732,6 +733,7 @@ public void testSystemTablePassesDynamicOptionsToPaimonTable() throws Exception node.setScanParams(new TableScanParams( TableScanParams.OPTIONS, options, Collections.emptyList())); Mockito.when(baseTable.copy(ArgumentMatchers.anyMap())).thenReturn(copiedTable); + Mockito.when(copiedTable.options()).thenReturn(options); try { Assert.assertSame(copiedTable, invokePrivateMethod(node, "getProcessedTable")); @@ -786,6 +788,25 @@ public void testDataTableQueryOptionsOverrideDefaultsWithoutMutation() throws Ex Assert.assertFalse(baseTable.options().containsKey("scan.snapshot-id")); } + @Test + public void testRejectsUnsafePhysicalOptionsAtFinalPlanningBoundary() throws Exception { + PaimonScanNode node = newTestNode(new PlanNodeId(0), new TupleId(0), sv); + PaimonSource source = Mockito.mock(PaimonSource.class); + PaimonExternalTable externalTable = Mockito.mock(PaimonExternalTable.class); + Table unsafePhysicalTable = Mockito.mock(Table.class); + Mockito.when(source.getExternalTable()).thenReturn(externalTable); + Mockito.when(source.getPaimonTable()).thenReturn(unsafePhysicalTable); + Mockito.when(unsafePhysicalTable.options()).thenReturn(ImmutableMap.of("read.batch-size", "0")); + node.setSource(source); + + try { + invokePrivateMethod(node, "getProcessedTable"); + Assert.fail("The final planning boundary must reject an effective zero batch size"); + } catch (java.lang.reflect.InvocationTargetException e) { + Assert.assertTrue(e.getTargetException().getMessage().contains("read.batch-size")); + } + } + @Test public void testDataTableOptionsUseRelationScopedCatalogHandle() throws Exception { PaimonScanNode node = newTestNode(new PlanNodeId(0), new TupleId(0), sv); @@ -803,6 +824,7 @@ public void testDataTableOptionsUseRelationScopedCatalogHandle() throws Exceptio Collections.emptyList()); node.setScanParams(scanParams); Mockito.when(source.getPaimonTable(scanParams)).thenReturn(relationScopedTable); + Mockito.when(relationScopedTable.options()).thenReturn(Collections.emptyMap()); Assert.assertSame(relationScopedTable, invokePrivateMethod(node, "getProcessedTable")); Mockito.verify(source).getPaimonTable(scanParams); @@ -897,6 +919,7 @@ public void testBackendSerializationUsesDynamicOptionsTable() throws Exception { node.setScanParams(new TableScanParams( TableScanParams.OPTIONS, options, Collections.emptyList())); Mockito.when(baseTable.copy(ArgumentMatchers.anyMap())).thenReturn(copiedTable); + Mockito.when(copiedTable.options()).thenReturn(options); try { invokePrivateMethod(node, "serializeProcessedTable"); diff --git a/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy b/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy index b4b3391a6fff79..2e869b3f6deb4c 100644 --- a/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy +++ b/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy @@ -92,6 +92,9 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { create table paimon.${dbName}.unsafe_physical_manifest (id int) using paimon tblproperties ('scan.manifest.parallelism'='0'); insert into paimon.${dbName}.unsafe_physical_manifest values (1); + drop table if exists paimon.${dbName}.empty_identifier; + create table paimon.${dbName}.empty_identifier (`` string) using paimon; + insert into paimon.${dbName}.empty_identifier values ('empty-name'); """ sql "switch ${catalogName}" @@ -115,6 +118,9 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { order by id """ } + test { + sql "select * from empty_identifier" + } sql "set enable_file_scanner_v2=true" test { sql """ @@ -123,6 +129,14 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { order by id """ } + test { + sql "select * from empty_identifier" + } + + // The safe catalog value must override the physical read.batch-size=0 value. + test { + sql "select * from unsafe_physical_batch order by id" + } test { sql """ @@ -180,10 +194,16 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { sql "select * from unsafe_physical_batch" exception "read.batch-size" } + test { + sql "select * from unsafe_physical_batch@options('read.batch-size'='4096') order by id" + } test { sql "select * from unsafe_physical_manifest" exception "scan.manifest.parallelism" } + test { + sql "select * from unsafe_physical_manifest@options('scan.manifest.parallelism'='1') order by id" + } } finally { sql "set force_jni_scanner=false" sql "set enable_file_scanner_v2=${originalScannerV2}" From 8ea521522915c65fcbcf1239de94d125d89c0ff9 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Thu, 30 Jul 2026 11:06:24 +0800 Subject: [PATCH 4/6] [fix](paimon) Guard hidden manifest planning paths ### What problem does this PR solve? Issue Number: None Related PR: #66247 Problem Summary: Paimon read-only system tables and fallback tables hide the file-store handles that actually plan manifests, while snapshot partition loading and row-count collection can plan before ScanNode. An unsafe physical manifest parallelism could therefore bypass the final visible-table check, and early validation could also reject a safe relation-level override. Validate each effective planning handle, preserve the fully merged table during partition projection, and guard row-count plans before they start. ### Release note Paimon manifest parallelism safeguards now cover system-table, fallback-branch, partition-metadata, and statistics planning paths. ### Check List (For Author) - Test: FE checkstyle, Groovy compilation, and manual Paimon 1.3.1 fixture probe passed; unit and P0 regression coverage added but not run locally because the installed third-party toolchain is absent - Behavior changed: Yes. Unsafe hidden manifest-planning handles are rejected before planning while safe relation overrides remain effective. - Does this need documentation: No --- .../paimon/PaimonPartitionInfoLoader.java | 14 +- .../paimon/PaimonExternalMetaCache.java | 6 +- .../paimon/PaimonExternalTable.java | 14 +- .../paimon/PaimonReaderOptions.java | 11 ++ .../datasource/paimon/PaimonScanParams.java | 2 +- .../paimon/PaimonSysExternalTable.java | 34 +++- .../doris/datasource/paimon/PaimonUtils.java | 4 + .../paimon/source/PaimonScanNode.java | 7 +- .../paimon/source/PaimonSource.java | 4 +- .../paimon/PaimonExternalMetaCacheTest.java | 63 +++++++- .../paimon/PaimonExternalTableTest.java | 148 ++++++++++++++++++ .../paimon/PaimonReaderOptionsTest.java | 44 ++++++ .../doris/statistics/AnalysisManagerTest.java | 75 +++++++++ .../test_paimon_jni_reader_guardrails.groovy | 15 +- 14 files changed, 421 insertions(+), 20 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/paimon/PaimonPartitionInfoLoader.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/paimon/PaimonPartitionInfoLoader.java index c29a359b9592d1..766a9b62f536fd 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/paimon/PaimonPartitionInfoLoader.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/paimon/PaimonPartitionInfoLoader.java @@ -22,22 +22,21 @@ import org.apache.doris.datasource.CacheException; import org.apache.doris.datasource.NameMapping; import org.apache.doris.datasource.paimon.PaimonPartitionInfo; +import org.apache.doris.datasource.paimon.PaimonReaderOptions; import org.apache.doris.datasource.paimon.PaimonUtil; import org.apache.commons.collections4.CollectionUtils; +import org.apache.paimon.catalog.CatalogUtils; import org.apache.paimon.partition.Partition; import org.apache.paimon.table.Table; import java.util.List; /** - * Loads partition info for a snapshot projection from the base Paimon table and catalog metadata. + * Loads partition info for a snapshot projection from the effective Paimon table handle. */ public final class PaimonPartitionInfoLoader { - private final PaimonTableLoader tableLoader; - - public PaimonPartitionInfoLoader(PaimonTableLoader tableLoader) { - this.tableLoader = tableLoader; + public PaimonPartitionInfoLoader() { } public PaimonPartitionInfo load(NameMapping nameMapping, Table paimonTable, List partitionColumns) @@ -46,7 +45,10 @@ public PaimonPartitionInfo load(NameMapping nameMapping, Table paimonTable, List return PaimonPartitionInfo.EMPTY; } try { - List paimonPartitions = tableLoader.catalog(nameMapping).getPaimonPartitions(nameMapping); + // Catalog.listPartitions reloads the raw physical table and loses Doris catalog/relation + // copies. Enumerate the already merged handle so validation and planning see one table. + PaimonReaderOptions.validateEffectiveTable(paimonTable); + List paimonPartitions = CatalogUtils.listPartitionsFromFileSystem(paimonTable); boolean legacyPartitionName = PaimonUtil.isLegacyPartitionName(paimonTable); return PaimonUtil.generatePartitionInfo(partitionColumns, paimonPartitions, legacyPartitionName); } catch (Exception e) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalMetaCache.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalMetaCache.java index 1d08ba1274e256..979c61abdc56a4 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalMetaCache.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalMetaCache.java @@ -66,7 +66,7 @@ public PaimonExternalMetaCache(ExecutorService refreshExecutor) { super(ENGINE, refreshExecutor); tableLoader = new PaimonTableLoader(); latestSnapshotProjectionLoader = new PaimonLatestSnapshotProjectionLoader( - new PaimonPartitionInfoLoader(tableLoader), this::getPaimonSchemaCacheValue); + new PaimonPartitionInfoLoader(), this::getPaimonSchemaCacheValue); tableEntry = registerEntry(MetaCacheEntryDef.of(ENTRY_TABLE, NameMapping.class, PaimonTableCacheValue.class, this::loadTableCacheValue, defaultEntryCacheSpec(), MetaCacheEntryInvalidation.forNameMapping(nameMapping -> nameMapping))); @@ -89,6 +89,10 @@ public PaimonSnapshotCacheValue getSnapshotCache(ExternalTable dorisTable) { return tableEntry.get(nameMapping.getCtlId()).get(nameMapping).getLatestSnapshotCacheValue(); } + public PaimonSnapshotCacheValue loadSnapshotProjection(ExternalTable dorisTable, Table effectiveTable) { + return latestSnapshotProjectionLoader.load(dorisTable.getOrBuildNameMapping(), effectiveTable); + } + public PaimonSchemaCacheValue getPaimonSchemaCacheValue(NameMapping nameMapping, long schemaId) { SchemaCacheValue schemaCacheValue = schemaEntry.get(nameMapping.getCtlId()) .get(new PaimonSchemaCacheKey(nameMapping, schemaId)); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java index 6aeacc9058d115..e40c4997668975 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java @@ -178,6 +178,14 @@ private PaimonSnapshotCacheValue getPaimonSnapshotCacheValue(Optional 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); } else if (scanParams.isPresent() && scanParams.get().isBranch()) { try { Table baseTable = getBasePaimonTable(); @@ -236,7 +244,11 @@ public BaseAnalysisTask createAnalysisTask(AnalysisInfo info) { public long fetchRowCount() { makeSureInitialized(); long rowCount = 0; - List splits = getBasePaimonTable().newReadBuilder().newScan().plan().splits(); + Table effectiveTable = getBasePaimonTable(); + // Statistics and row-count cache planning run before ScanNode and must not reach an + // unsafe manifest executor, even when the foreground relation later supplies an override. + PaimonReaderOptions.validateEffectiveTable(effectiveTable); + List splits = effectiveTable.newReadBuilder().newScan().plan().splits(); for (Split split : splits) { rowCount += split.rowCount(); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java index 49ff11cc8f793e..e7f604b1ef4fcc 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java @@ -22,6 +22,8 @@ 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; @@ -109,6 +111,15 @@ public static void validateEffectiveTableOptions(Map options) { validateManifestParallelism(options.get(CoreOptions.SCAN_MANIFEST_PARALLELISM.key())); } + public static void validateEffectiveTable(Table table) { + validateEffectiveTableOptions(table.options()); + if (table instanceof FallbackReadFileStoreTable) { + // The fallback scan plans its private child independently, so the visible main options + // cannot prove that every manifest executor input is safe. + validateEffectiveTable(((FallbackReadFileStoreTable) table).fallback()); + } + } + private static void validateManifestParallelism(String value) { if (value == null) { return; diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java index 8c87a9c20717d0..3154cc3fe4afd1 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java @@ -147,7 +147,7 @@ public static Table applyOptions(Table table, Map options) { Table effectiveTable = table.copy(isolatedOptions); // Validate after every copy so relation options participate in the documented // relation > catalog > physical precedence before the effective value is judged. - PaimonReaderOptions.validateEffectiveTableOptions(effectiveTable.options()); + PaimonReaderOptions.validateEffectiveTable(effectiveTable); return effectiveTable; } diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java index 25297de6862e26..63615521db70f8 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java @@ -117,6 +117,12 @@ private static long generateSysTableId(long sourceTableId, String sysTableType) * Note: system tables currently ignore snapshot semantics. */ public Table getSysPaimonTable() { + validateEffectiveDataTable(null); + return getRawSysPaimonTable(); + } + + /** Returns the cached wrapper without validating its hidden data table. */ + public Table getRawSysPaimonTable() { if (paimonSysTable == null) { synchronized (this) { if (paimonSysTable == null) { @@ -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 resolvedOptions = scanParams.getOrResolveMapParams( - options -> PaimonScanParams.resolveOptions(sourceTable.getBasePaimonTable(), options)); + Map 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(); + if (scanParams != null && scanParams.isOptions()) { + // Apply the same relation copy to the data table hidden by ReadonlyTable wrappers. + PaimonScanParams.applyOptions(dataTable, resolvedOptions(scanParams)); + } else { + PaimonReaderOptions.validateEffectiveTable(dataTable); + } + } + + private Map resolvedOptions(TableScanParams scanParams) { + return scanParams.getOrResolveMapParams( + options -> PaimonScanParams.resolveOptions(sourceTable.getBasePaimonTable(), options)); } public List getFullSchema(TableScanParams scanParams) { @@ -187,7 +207,9 @@ private boolean resolveIsDataTable() { if (isDataTable == null) { synchronized (this) { if (isDataTable == null) { - isDataTable = getSysPaimonTable() instanceof DataTable; + // Type inspection happens before relation parameters reach ScanNode. It must not + // reject a hidden physical value that a later OPTIONS copy safely overrides. + isDataTable = getRawSysPaimonTable() instanceof DataTable; } } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonUtils.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonUtils.java index dc28c083ca10fa..3503493343d34e 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonUtils.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonUtils.java @@ -35,6 +35,10 @@ public static PaimonSnapshotCacheValue getLatestSnapshotCacheValue(ExternalTable return paimonExternalMetaCache(dorisTable).getSnapshotCache(dorisTable); } + public static PaimonSnapshotCacheValue loadSnapshotProjection(ExternalTable dorisTable, Table effectiveTable) { + return paimonExternalMetaCache(dorisTable).loadSnapshotProjection(dorisTable, effectiveTable); + } + public static PaimonSnapshotCacheValue getSnapshotCacheValue(Optional snapshot, ExternalTable dorisTable) { if (snapshot.isPresent() && snapshot.get() instanceof PaimonMvccSnapshot) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java index 78c1d55f0ad500..75b3c1800bc6a1 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java @@ -1013,7 +1013,12 @@ private Table getProcessedTable() throws UserException { try { // This is the last common boundary before planning and serialization, including scans // with no relation copy and incremental/system-table paths that bypass applyOptions. - PaimonReaderOptions.validateEffectiveTableOptions(finalTable.options()); + PaimonReaderOptions.validateEffectiveTable(finalTable); + if (source.getExternalTable() instanceof PaimonSysExternalTable) { + // Read-only system wrappers hide the data table that performs manifest planning. + ((PaimonSysExternalTable) source.getExternalTable()) + .validateEffectiveDataTable(theScanParams); + } } catch (IllegalArgumentException e) { throw new UserException(e.getMessage(), e); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonSource.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonSource.java index e8a3fb33d957ed..1ca508b4457e27 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonSource.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonSource.java @@ -86,7 +86,9 @@ private Table resolvePaimonTable(ExternalTable table) { return ((PaimonExternalTable) table).getPaimonTable(snapshot); } if (table instanceof PaimonSysExternalTable) { - return ((PaimonSysExternalTable) table).getSysPaimonTable(); + // Relation options are unavailable during source construction, so defer validation of + // the wrapper's hidden data table until getProcessedTable has the complete precedence chain. + return ((PaimonSysExternalTable) table).getRawSysPaimonTable(); } throw new IllegalArgumentException( "Expected Paimon table but got " + table.getClass().getSimpleName()); diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalMetaCacheTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalMetaCacheTest.java index 3389eb23918b07..6a018f8e06742f 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalMetaCacheTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalMetaCacheTest.java @@ -17,6 +17,9 @@ package org.apache.doris.datasource.paimon; +import org.apache.doris.catalog.Column; +import org.apache.doris.catalog.Type; +import org.apache.doris.datasource.CacheException; import org.apache.doris.datasource.ExternalCatalog; import org.apache.doris.datasource.NameMapping; import org.apache.doris.datasource.SchemaCacheValue; @@ -26,11 +29,19 @@ import org.apache.paimon.CoreOptions; import org.apache.paimon.Snapshot; +import org.apache.paimon.fs.Path; +import org.apache.paimon.fs.local.LocalFileIO; import org.apache.paimon.schema.SchemaManager; import org.apache.paimon.schema.TableSchema; +import org.apache.paimon.table.AppendOnlyFileStoreTable; +import org.apache.paimon.table.CatalogEnvironment; import org.apache.paimon.table.FileStoreTable; +import org.apache.paimon.types.DataField; +import org.apache.paimon.types.IntType; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TemporaryFolder; import org.mockito.Mockito; import java.util.Collections; @@ -40,11 +51,13 @@ import java.util.concurrent.Executors; public class PaimonExternalMetaCacheTest { + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder(); @Test public void testLatestSnapshotUsesLatestSchemaForPinnedRead() { PaimonLatestSnapshotProjectionLoader loader = new PaimonLatestSnapshotProjectionLoader( - new PaimonPartitionInfoLoader(null), + new PaimonPartitionInfoLoader(), (nameMapping, schemaId) -> new PaimonSchemaCacheValue( Collections.emptyList(), Collections.emptyList(), null)); NameMapping nameMapping = new NameMapping(1L, "db", "table", "remote_db", "remote_table"); @@ -71,6 +84,54 @@ public void testLatestSnapshotUsesLatestSchemaForPinnedRead() { Mockito.verify(pinnedTable).copyWithLatestSchema(); } + @Test + public void testPartitionProjectionRejectsUnsafeEffectiveTableBeforeEnumeration() throws Exception { + FileStoreTable unsafeTable = newPartitionedTable( + "unsafe", Collections.singletonMap("scan.manifest.parallelism", "0")); + PaimonPartitionInfoLoader loader = new PaimonPartitionInfoLoader(); + + try { + loader.load(new NameMapping(1L, "db", "table", "db", "table"), unsafeTable, + Collections.singletonList(new Column("part", Type.INT))); + Assert.fail("partition projection must reject unsafe manifest parallelism before enumeration"); + } catch (CacheException e) { + Assert.assertTrue(e.getMessage().contains("scan.manifest.parallelism")); + } + } + + @Test + public void testPartitionProjectionUsesSafeCopiedTableInsteadOfRawCatalogReload() throws Exception { + FileStoreTable unsafeTable = newPartitionedTable( + "safe_override", Collections.singletonMap("scan.manifest.parallelism", "0")); + FileStoreTable safeTable = unsafeTable.copy( + Collections.singletonMap("scan.manifest.parallelism", "1")); + PaimonPartitionInfoLoader loader = new PaimonPartitionInfoLoader(); + + PaimonPartitionInfo partitionInfo = loader.load( + new NameMapping(1L, "db", "table", "db", "table"), safeTable, + Collections.singletonList(new Column("part", Type.INT))); + + Assert.assertTrue(partitionInfo.getNameToPartition().isEmpty()); + } + + private FileStoreTable newPartitionedTable(String name, Map options) throws Exception { + TableSchema schema = new TableSchema( + 0, + java.util.Arrays.asList( + new DataField(0, "id", new IntType()), + new DataField(1, "part", new IntType())), + 1, + Collections.singletonList("part"), + Collections.emptyList(), + options, + null); + return new AppendOnlyFileStoreTable( + LocalFileIO.create(), + new Path(temporaryFolder.newFolder(name).toURI()), + schema, + CatalogEnvironment.empty()); + } + @Test public void testInvalidateTablePrecise() { ExecutorService executor = Executors.newSingleThreadExecutor(); diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java index da72285aa7dbb9..9b6a791efb112a 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java @@ -18,11 +18,25 @@ package org.apache.doris.datasource.paimon; import org.apache.doris.analysis.TableScanParams; +import org.apache.doris.catalog.TableIf; +import org.apache.doris.datasource.ExternalRowCountCache; import org.apache.doris.datasource.mvcc.MvccSnapshot; import org.apache.doris.datasource.mvcc.MvccUtil; +import org.apache.doris.statistics.util.StatisticsUtil; import com.google.common.collect.ImmutableMap; +import com.google.common.util.concurrent.MoreExecutors; +import org.apache.paimon.fs.FileIO; +import org.apache.paimon.fs.Path; +import org.apache.paimon.schema.TableSchema; +import org.apache.paimon.table.AppendOnlyFileStoreTable; +import org.apache.paimon.table.CatalogEnvironment; +import org.apache.paimon.table.FileStoreTable; import org.apache.paimon.table.Table; +import org.apache.paimon.table.source.ReadBuilder; +import org.apache.paimon.table.system.PartitionsTable; +import org.apache.paimon.types.DataField; +import org.apache.paimon.types.IntType; import org.junit.Assert; import org.junit.Test; import org.mockito.ArgumentMatchers; @@ -30,7 +44,9 @@ import org.mockito.Mockito; import java.util.Collections; +import java.util.Map; import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; public class PaimonExternalTableTest { @@ -96,4 +112,136 @@ public void testModeOnlyLatestUsesStatementSnapshotAcrossPhases() { && options.containsKey("scan.mode") && options.get("scan.mode") == null)); } + + @Test + public void testRelationOptionsLoadSnapshotProjectionFromEffectiveTable() { + PaimonExternalTable externalTable = Mockito.mock( + PaimonExternalTable.class, Mockito.CALLS_REAL_METHODS); + Mockito.doNothing().when(externalTable).makeSureInitialized(); + FileStoreTable unsafeDataTable = newFileStoreTable( + "snapshot_projection", ImmutableMap.of("scan.manifest.parallelism", "0"), null); + TableScanParams scanParams = new TableScanParams( + TableScanParams.OPTIONS, + ImmutableMap.of("scan.manifest.parallelism", "1"), + Collections.emptyList()); + PaimonSnapshotCacheValue projection = Mockito.mock(PaimonSnapshotCacheValue.class); + + try (MockedStatic paimonUtils = Mockito.mockStatic(PaimonUtils.class)) { + paimonUtils.when(() -> PaimonUtils.getPaimonTable(externalTable)).thenReturn(unsafeDataTable); + paimonUtils.when(() -> PaimonUtils.loadSnapshotProjection( + Mockito.eq(externalTable), Mockito.any(Table.class))).thenReturn(projection); + + PaimonMvccSnapshot snapshot = (PaimonMvccSnapshot) externalTable.loadSnapshot( + Optional.empty(), Optional.of(scanParams)); + + Assert.assertSame(projection, snapshot.getSnapshotCacheValue()); + paimonUtils.verify(() -> PaimonUtils.loadSnapshotProjection( + Mockito.eq(externalTable), Mockito.argThat(table -> + "1".equals(table.options().get("scan.manifest.parallelism"))))); + } + } + + @Test + public void testPartitionsTableValidatesHiddenDataTableWithOverridePrecedence() throws Exception { + FileStoreTable unsafeDataTable = newFileStoreTable( + "partitions", ImmutableMap.of("scan.manifest.parallelism", "0"), null); + PaimonSysExternalTable systemTable = newSystemTable(unsafeDataTable); + + try { + systemTable.getSysPaimonTable(); + Assert.fail("$partitions must reject an unsafe hidden data table without an override"); + } catch (IllegalArgumentException e) { + Assert.assertTrue(e.getMessage().contains("scan.manifest.parallelism")); + } + + TableScanParams safeOverride = new TableScanParams( + TableScanParams.OPTIONS, + ImmutableMap.of("scan.manifest.parallelism", "1"), + Collections.emptyList()); + Assert.assertTrue(systemTable.getSysPaimonTable(safeOverride) instanceof PartitionsTable); + Assert.assertFalse(systemTable.isDataTable()); + } + + @Test + public void testFetchRowCountValidatesBeforePlanning() { + AtomicBoolean planningStarted = new AtomicBoolean(false); + FileStoreTable unsafeDataTable = newFileStoreTable( + "row_count", ImmutableMap.of("scan.manifest.parallelism", "0"), planningStarted); + PaimonExternalTable externalTable = Mockito.mock( + PaimonExternalTable.class, Mockito.CALLS_REAL_METHODS); + Mockito.doNothing().when(externalTable).makeSureInitialized(); + + try (MockedStatic paimonUtils = Mockito.mockStatic(PaimonUtils.class)) { + paimonUtils.when(() -> PaimonUtils.getPaimonTable(externalTable)).thenReturn(unsafeDataTable); + try { + externalTable.fetchRowCount(); + Assert.fail("row-count planning must reject unsafe manifest parallelism"); + } catch (IllegalArgumentException e) { + Assert.assertTrue(e.getMessage().contains("scan.manifest.parallelism")); + } + } + Assert.assertFalse(planningStarted.get()); + } + + @Test + public void testExternalRowCountCacheUsesGuardedPaimonFetch() { + AtomicBoolean planningStarted = new AtomicBoolean(false); + FileStoreTable unsafeDataTable = newFileStoreTable( + "row_count_cache", ImmutableMap.of("scan.manifest.parallelism", "0"), planningStarted); + PaimonExternalTable externalTable = Mockito.mock( + PaimonExternalTable.class, Mockito.CALLS_REAL_METHODS); + Mockito.doNothing().when(externalTable).makeSureInitialized(); + + try (MockedStatic paimonUtils = Mockito.mockStatic(PaimonUtils.class); + MockedStatic statisticsUtil = Mockito.mockStatic(StatisticsUtil.class)) { + paimonUtils.when(() -> PaimonUtils.getPaimonTable(externalTable)).thenReturn(unsafeDataTable); + statisticsUtil.when(() -> StatisticsUtil.findTable(1, 2, 3)).thenReturn(externalTable); + ExternalRowCountCache cache = new ExternalRowCountCache(MoreExecutors.newDirectExecutorService()); + + Assert.assertEquals(TableIf.UNKNOWN_ROW_COUNT, cache.getCachedRowCount(1, 2, 3, false)); + } + Assert.assertFalse(planningStarted.get()); + } + + private PaimonSysExternalTable newSystemTable(FileStoreTable dataTable) throws Exception { + PaimonExternalTable sourceTable = Mockito.mock(PaimonExternalTable.class); + PaimonExternalCatalog catalog = Mockito.mock(PaimonExternalCatalog.class); + PaimonExternalDatabase database = Mockito.mock(PaimonExternalDatabase.class); + Mockito.when(catalog.getId()).thenReturn(1L); + Mockito.when(database.getFullName()).thenReturn("db"); + Mockito.when(database.getRemoteName()).thenReturn("db"); + Mockito.when(sourceTable.getId()).thenReturn(10L); + Mockito.when(sourceTable.getName()).thenReturn("source"); + Mockito.when(sourceTable.getRemoteName()).thenReturn("source"); + Mockito.when(sourceTable.getCatalog()).thenReturn(catalog); + Mockito.when(sourceTable.getDatabase()).thenReturn(database); + Mockito.doReturn(dataTable).when(sourceTable).getBasePaimonTable(); + PaimonSysExternalTable systemTable = new PaimonSysExternalTable(sourceTable, "partitions"); + java.lang.reflect.Field field = PaimonSysExternalTable.class.getDeclaredField("paimonSysTable"); + field.setAccessible(true); + field.set(systemTable, new PartitionsTable(dataTable)); + return systemTable; + } + + private FileStoreTable newFileStoreTable( + String name, Map options, AtomicBoolean planningStarted) { + TableSchema schema = new TableSchema( + 0, + Collections.singletonList(new DataField(0, "id", new IntType())), + 0, + Collections.emptyList(), + Collections.emptyList(), + options, + null); + return new AppendOnlyFileStoreTable( + Mockito.mock(FileIO.class), new Path("memory://" + name), schema, CatalogEnvironment.empty()) { + @Override + public ReadBuilder newReadBuilder() { + if (planningStarted != null) { + planningStarted.set(true); + } + return super.newReadBuilder(); + } + }; + } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java index 0319f11c4b755f..7790e49e8deeb6 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java @@ -20,7 +20,16 @@ import org.apache.doris.datasource.property.metastore.AbstractPaimonProperties; import com.google.common.collect.ImmutableMap; +import org.apache.paimon.fs.FileIO; +import org.apache.paimon.fs.Path; +import org.apache.paimon.schema.TableSchema; +import org.apache.paimon.table.AppendOnlyFileStoreTable; +import org.apache.paimon.table.CatalogEnvironment; +import org.apache.paimon.table.FallbackReadFileStoreTable; +import org.apache.paimon.table.FileStoreTable; import org.apache.paimon.table.Table; +import org.apache.paimon.types.DataField; +import org.apache.paimon.types.IntType; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -79,4 +88,39 @@ void testSafeRelationOptionOverridesUnsafePhysicalOption() { Assertions.assertSame(finalTable, PaimonScanParams.applyOptions(physicalTable, relationOptions)); } + + @Test + void testRejectUnsafeHiddenFallbackTableAfterCopy() { + FileStoreTable main = newFileStoreTable("main", Collections.emptyMap()); + FileStoreTable fallback = newFileStoreTable( + "fallback", ImmutableMap.of("scan.manifest.parallelism", "0")); + Table fallbackReadTable = new FallbackReadFileStoreTable(main, fallback); + + Assertions.assertThrows(IllegalArgumentException.class, + () -> PaimonScanParams.applyOptions(fallbackReadTable, Collections.emptyMap())); + } + + @Test + void testSafeRelationOptionOverridesUnsafeHiddenFallbackTable() { + FileStoreTable main = newFileStoreTable("main", Collections.emptyMap()); + FileStoreTable fallback = newFileStoreTable( + "fallback", ImmutableMap.of("scan.manifest.parallelism", "0")); + Table fallbackReadTable = new FallbackReadFileStoreTable(main, fallback); + + Assertions.assertDoesNotThrow(() -> PaimonScanParams.applyOptions( + fallbackReadTable, ImmutableMap.of("scan.manifest.parallelism", "1"))); + } + + private FileStoreTable newFileStoreTable(String name, Map options) { + TableSchema schema = new TableSchema( + 0, + Collections.singletonList(new DataField(0, "id", new IntType())), + 0, + Collections.emptyList(), + Collections.emptyList(), + options, + null); + return new AppendOnlyFileStoreTable( + Mockito.mock(FileIO.class), new Path("memory://" + name), schema, CatalogEnvironment.empty()); + } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/statistics/AnalysisManagerTest.java b/fe/fe-core/src/test/java/org/apache/doris/statistics/AnalysisManagerTest.java index 30b7bd55ed5010..0e1e95025fc8ae 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/statistics/AnalysisManagerTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/statistics/AnalysisManagerTest.java @@ -29,6 +29,10 @@ import org.apache.doris.common.DdlException; import org.apache.doris.common.Pair; import org.apache.doris.datasource.InternalCatalog; +import org.apache.doris.datasource.paimon.PaimonExternalCatalog; +import org.apache.doris.datasource.paimon.PaimonExternalDatabase; +import org.apache.doris.datasource.paimon.PaimonExternalTable; +import org.apache.doris.datasource.paimon.PaimonUtils; import org.apache.doris.nereids.trees.expressions.ExprId; import org.apache.doris.nereids.trees.expressions.Slot; import org.apache.doris.nereids.trees.expressions.SlotReference; @@ -38,9 +42,20 @@ import org.apache.doris.statistics.AnalysisInfo.AnalysisType; import org.apache.doris.statistics.AnalysisInfo.JobType; import org.apache.doris.statistics.AnalysisInfo.ScheduleType; +import org.apache.doris.statistics.util.StatisticsUtil; import org.apache.doris.thrift.TQueryColumn; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import org.apache.paimon.fs.FileIO; +import org.apache.paimon.fs.Path; +import org.apache.paimon.schema.TableSchema; +import org.apache.paimon.table.AppendOnlyFileStoreTable; +import org.apache.paimon.table.CatalogEnvironment; +import org.apache.paimon.table.FileStoreTable; +import org.apache.paimon.table.source.ReadBuilder; +import org.apache.paimon.types.DataField; +import org.apache.paimon.types.IntType; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.mockito.MockedStatic; @@ -53,6 +68,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; // CHECKSTYLE OFF @@ -118,6 +134,55 @@ public void testBuildAnalysisJobInfoAutoSampleCommandCollectsHotValue() { } } + @Test + public void testManualAnalysisValidatesPaimonRowCountBeforePlanning() { + AtomicBoolean planningStarted = new AtomicBoolean(false); + FileStoreTable unsafeTable = new AppendOnlyFileStoreTable( + Mockito.mock(FileIO.class), + new Path("memory://manual_analysis_guard"), + new TableSchema( + 0, + Collections.singletonList(new DataField(0, "id", new IntType())), + 0, + Collections.emptyList(), + Collections.emptyList(), + ImmutableMap.of("scan.manifest.parallelism", "0"), + null), + CatalogEnvironment.empty()) { + @Override + public ReadBuilder newReadBuilder() { + planningStarted.set(true); + return super.newReadBuilder(); + } + }; + PaimonExternalCatalog catalog = Mockito.mock(PaimonExternalCatalog.class); + PaimonExternalDatabase database = Mockito.mock(PaimonExternalDatabase.class); + Mockito.when(catalog.getId()).thenReturn(1L); + Mockito.when(database.getFullName()).thenReturn("db"); + Mockito.when(database.getRemoteName()).thenReturn("db"); + PaimonExternalTable table = Mockito.spy(new TestPaimonExternalTable(catalog, database)); + Mockito.doReturn(-1L).when(table).getRowCount(); + Mockito.doReturn(Collections.emptySet()).when(table).getColumnIndexPairs(Mockito.any()); + AnalyzeTableCommand command = mockAnalyzeCommand(AnalysisMethod.FULL, ScheduleType.ONCE, false, false); + Mockito.when(command.getTable()).thenReturn(table); + AnalysisManager manager = new AnalysisManager(); + Env env = Mockito.mock(Env.class); + + try (MockedStatic envMockedStatic = Mockito.mockStatic(Env.class); + MockedStatic paimonUtils = Mockito.mockStatic(PaimonUtils.class); + MockedStatic statisticsUtil = Mockito.mockStatic( + StatisticsUtil.class, Mockito.CALLS_REAL_METHODS)) { + envMockedStatic.when(Env::getCurrentEnv).thenReturn(env); + Mockito.when(env.getNextId()).thenReturn(1L); + paimonUtils.when(() -> PaimonUtils.getPaimonTable(table)).thenReturn(unsafeTable); + statisticsUtil.when(() -> StatisticsUtil.isEmptyTable(table, AnalysisMethod.FULL)).thenReturn(false); + + Assertions.assertThrows(IllegalArgumentException.class, + () -> manager.buildAnalysisJobInfo(command)); + } + Assertions.assertFalse(planningStarted.get()); + } + @Test public void testUpdateTaskStatus() { BaseAnalysisTask task1 = Mockito.mock(BaseAnalysisTask.class); @@ -473,4 +538,14 @@ private AnalyzeTableCommand mockAnalyzeCommand(AnalysisMethod analysisMethod, Sc Mockito.when(command.getAnalyzeProperties()).thenReturn(analyzeProperties); return command; } + + private static class TestPaimonExternalTable extends PaimonExternalTable { + private TestPaimonExternalTable(PaimonExternalCatalog catalog, PaimonExternalDatabase database) { + super(30001L, "table", "table", catalog, database); + } + + @Override + protected synchronized void makeSureInitialized() { + } + } } diff --git a/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy b/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy index 2e869b3f6deb4c..e5a9deabee17e1 100644 --- a/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy +++ b/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy @@ -89,9 +89,10 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { tblproperties ('read.batch-size'='0'); insert into paimon.${dbName}.unsafe_physical_batch values (1); drop table if exists paimon.${dbName}.unsafe_physical_manifest; - create table paimon.${dbName}.unsafe_physical_manifest (id int) using paimon + create table paimon.${dbName}.unsafe_physical_manifest (id int, part int) using paimon + partitioned by (part) tblproperties ('scan.manifest.parallelism'='0'); - insert into paimon.${dbName}.unsafe_physical_manifest values (1); + insert into paimon.${dbName}.unsafe_physical_manifest values (1, 10); drop table if exists paimon.${dbName}.empty_identifier; create table paimon.${dbName}.empty_identifier (`` string) using paimon; insert into paimon.${dbName}.empty_identifier values ('empty-name'); @@ -204,6 +205,16 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { test { sql "select * from unsafe_physical_manifest@options('scan.manifest.parallelism'='1') order by id" } + test { + sql "select count(*) from unsafe_physical_manifest\$partitions" + exception "scan.manifest.parallelism" + } + test { + sql """ + select count(*) from unsafe_physical_manifest\$partitions + @options('scan.manifest.parallelism'='1') + """ + } } finally { sql "set force_jni_scanner=false" sql "set enable_file_scanner_v2=${originalScannerV2}" From 43f91409ddca4dbf0d298b49ea14cec0da617b5d Mon Sep 17 00:00:00 2001 From: Gabriel Date: Thu, 30 Jul 2026 11:57:13 +0800 Subject: [PATCH 5/6] [fix](paimon) Address wrapper and parser review gaps ### What problem does this PR solve? Issue Number: None Related PR: #66247 Problem Summary: The unencoded JNI STRUCT parser no longer preserved its historical lowercase field-key contract. Paimon fallback validation also stopped at privilege delegates, system tables could be built from a separately reloaded data handle, and reader-only relation options unnecessarily bypassed the memoized partition projection. Restore legacy parser compatibility, traverse delegated planning handles, construct system wrappers from the exact validated source handle, and keep pure reader tuning on the cached projection. ### Release note Paimon reader tuning avoids redundant partition enumeration, privilege-wrapped fallback tables receive complete manifest validation, and legacy unencoded JNI STRUCT keys retain lowercase compatibility. ### Check List (For Author) - Test: PaimonJniScannerTest (15 passed), targeted legacy parser test, Paimon 1.3.1 privilege-wrapper probe, FE checkstyle, and diff checks passed; FE unit coverage added but not run locally because the installed third-party toolchain is absent - Behavior changed: Yes. System wrappers share the validated source handle and pure reader options reuse memoized metadata. - Does this need documentation: No --- .../doris/common/jni/vec/ColumnType.java | 4 + .../doris/common/jni/JniScannerTest.java | 12 +++ .../paimon/PaimonExternalTable.java | 5 ++ .../paimon/PaimonReaderOptions.java | 6 ++ .../datasource/paimon/PaimonScanParams.java | 6 ++ .../paimon/PaimonSysExternalTable.java | 40 ++++++--- .../paimon/PaimonExternalTableTest.java | 87 +++++++++++++++++-- .../paimon/PaimonReaderOptionsTest.java | 18 ++++ 8 files changed, 162 insertions(+), 16 deletions(-) diff --git a/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/vec/ColumnType.java b/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/vec/ColumnType.java index f1c40b45b4bdd4..983681d24dcb72 100644 --- a/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/vec/ColumnType.java +++ b/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/vec/ColumnType.java @@ -448,6 +448,10 @@ private static ColumnType parseType(String columnName, String hiveType, boolean // names before parsing, then restores the exact UTF-8 spelling. fieldName = new String(Base64.getDecoder().decode(fieldName.substring(1)), StandardCharsets.UTF_8); + } else { + // The public legacy grammar historically normalized STRUCT keys + // to lowercase; only the versioned Paimon payload preserves spelling. + fieldName = fieldName.toLowerCase(); } fields.add(parseType(fieldName, listFields.substring(pivot + 1, index), encodedStructFields)); diff --git a/fe/be-java-extensions/java-common/src/test/java/org/apache/doris/common/jni/JniScannerTest.java b/fe/be-java-extensions/java-common/src/test/java/org/apache/doris/common/jni/JniScannerTest.java index 74683955411801..9e9b15fc510c38 100644 --- a/fe/be-java-extensions/java-common/src/test/java/org/apache/doris/common/jni/JniScannerTest.java +++ b/fe/be-java-extensions/java-common/src/test/java/org/apache/doris/common/jni/JniScannerTest.java @@ -19,15 +19,27 @@ import org.apache.doris.common.jni.utils.OffHeap; +import org.apache.doris.common.jni.vec.ColumnType; import org.apache.doris.common.jni.vec.VectorTable; import org.junit.Assert; import org.junit.Test; import java.io.IOException; +import java.util.Arrays; import java.util.HashMap; public class JniScannerTest { + @Test + public void testUnencodedStructFieldNamesRemainLowerCase() { + ColumnType structType = ColumnType.parseType( + "value", "struct>"); + + Assert.assertEquals(Arrays.asList("mixed", "upper"), structType.getChildNames()); + Assert.assertEquals(Arrays.asList("nested"), + structType.getChildTypes().get(1).getChildNames()); + } + @Test public void testMockJniScanner() throws IOException { OffHeap.setTesting(); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java index e40c4997668975..96dff680734b98 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java @@ -183,6 +183,11 @@ private PaimonSnapshotCacheValue getPaimonSnapshotCacheValue(Optional resolvedOptions = scanParams.get().getOrResolveMapParams( options -> PaimonScanParams.resolveOptions(baseTable, options)); Table effectiveTable = PaimonScanParams.applyOptions(baseTable, resolvedOptions); + if (PaimonScanParams.hasOnlyReaderOptions(resolvedOptions)) { + // Reader tuning cannot change snapshot metadata. Reuse the memoized projection so + // a per-query batch-size change does not enumerate every partition again. + return PaimonUtils.getLatestSnapshotCacheValue(this); + } // 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); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java index e7f604b1ef4fcc..b898f55f465957 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java @@ -22,6 +22,7 @@ import org.apache.paimon.options.ConfigOption; import org.apache.paimon.options.MemorySize; import org.apache.paimon.options.Options; +import org.apache.paimon.table.DelegatedFileStoreTable; import org.apache.paimon.table.FallbackReadFileStoreTable; import org.apache.paimon.table.Table; @@ -118,6 +119,11 @@ public static void validateEffectiveTable(Table table) { // cannot prove that every manifest executor input is safe. validateEffectiveTable(((FallbackReadFileStoreTable) table).fallback()); } + if (table instanceof DelegatedFileStoreTable) { + // Privilege and other supported delegates can hide a fallback planner behind their + // own visible main options, so traverse the complete planning-handle chain. + validateEffectiveTable(((DelegatedFileStoreTable) table).wrapped()); + } } private static void validateManifestParallelism(String value) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java index 3154cc3fe4afd1..a96fc25f692077 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java @@ -185,6 +185,12 @@ public static boolean selectsSchema(Map options) { return hasStartupOptions(options); } + public static boolean hasOnlyReaderOptions(Map options) { + Map tableOptions = userOptions(options); + return !tableOptions.isEmpty() + && PaimonReaderOptions.supportedOptions().containsAll(tableOptions.keySet()); + } + public static boolean usesStatementSnapshot(Map options) { if (options.keySet().stream().anyMatch(STARTUP_POSITION_KEYS::contains)) { return false; diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java index 63615521db70f8..df36b23fce0504 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java @@ -36,8 +36,10 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.paimon.table.DataTable; +import org.apache.paimon.table.FileStoreTable; import org.apache.paimon.table.Table; import org.apache.paimon.table.source.Split; +import org.apache.paimon.table.system.SystemTableLoader; import org.apache.paimon.types.DataField; import org.apache.paimon.types.DataTypeRoot; @@ -70,6 +72,7 @@ public class PaimonSysExternalTable extends ExternalTable { private final PaimonExternalTable sourceTable; private final String sysTableType; private volatile Boolean isDataTable; + private volatile FileStoreTable paimonSysDataTable; private volatile Table paimonSysTable; private volatile List fullSchema; private volatile SchemaCacheValue schemaCacheValue; @@ -126,12 +129,15 @@ public Table getRawSysPaimonTable() { if (paimonSysTable == null) { synchronized (this) { if (paimonSysTable == null) { - PaimonExternalCatalog catalog = (PaimonExternalCatalog) getCatalog(); - paimonSysTable = catalog.getPaimonTable( - sourceTable.getOrBuildNameMapping(), - "main", // branch - sysTableType // queryType: snapshots, binlog, etc. - ); + Table dataTable = sourceTable.getBasePaimonTable(); + if (!(dataTable instanceof FileStoreTable)) { + throw new IllegalArgumentException( + "Paimon system tables require a file-store data table."); + } + paimonSysDataTable = (FileStoreTable) dataTable; + // Build the wrapper from this exact cached handle. Reloading it through the + // catalog can pair validation with one generation and planning with another. + paimonSysTable = createSystemTable(paimonSysDataTable); LOG.info("Created Paimon system table: {} for source table: {}", sysTableType, sourceTable.getName()); } @@ -145,18 +151,19 @@ public Table getSysPaimonTable(TableScanParams scanParams) { return getSysPaimonTable(); } Map 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(getRawSysPaimonTable(), resolvedOptions); + FileStoreTable effectiveDataTable = (FileStoreTable) PaimonScanParams.applyOptions( + getRawSysPaimonDataTable(), resolvedOptions); + return createSystemTable(effectiveDataTable); } public void validateEffectiveDataTable(TableScanParams scanParams) { - Table dataTable = sourceTable.getBasePaimonTable(); + Table dataTable = getRawSysPaimonDataTable(); if (scanParams != null && scanParams.isOptions()) { // Apply the same relation copy to the data table hidden by ReadonlyTable wrappers. PaimonScanParams.applyOptions(dataTable, resolvedOptions(scanParams)); @@ -167,7 +174,20 @@ public void validateEffectiveDataTable(TableScanParams scanParams) { private Map resolvedOptions(TableScanParams scanParams) { return scanParams.getOrResolveMapParams( - options -> PaimonScanParams.resolveOptions(sourceTable.getBasePaimonTable(), options)); + options -> PaimonScanParams.resolveOptions(getRawSysPaimonDataTable(), options)); + } + + private FileStoreTable getRawSysPaimonDataTable() { + getRawSysPaimonTable(); + return paimonSysDataTable; + } + + private Table createSystemTable(FileStoreTable dataTable) { + Table systemTable = SystemTableLoader.load(sysTableType, dataTable); + if (systemTable == null) { + throw new IllegalArgumentException("Unknown Paimon system table '" + sysTableType + "'."); + } + return systemTable; } public List getFullSchema(TableScanParams scanParams) { diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java index 9b6a791efb112a..42ed98324f97b6 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java @@ -43,10 +43,12 @@ import org.mockito.MockedStatic; import org.mockito.Mockito; +import java.util.Arrays; import java.util.Collections; import java.util.Map; import java.util.Optional; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; public class PaimonExternalTableTest { @@ -141,6 +143,68 @@ public void testRelationOptionsLoadSnapshotProjectionFromEffectiveTable() { } } + @Test + public void testReaderOnlyOptionsReusePartitionedMemoizedProjection() { + PaimonExternalTable externalTable = Mockito.mock( + PaimonExternalTable.class, Mockito.CALLS_REAL_METHODS); + Mockito.doNothing().when(externalTable).makeSureInitialized(); + FileStoreTable partitionedTable = newPartitionedFileStoreTable("reader_only_projection"); + TableScanParams scanParams = new TableScanParams( + TableScanParams.OPTIONS, + ImmutableMap.of("read.batch-size", "4096"), + Collections.emptyList()); + PaimonSnapshotCacheValue memoizedProjection = Mockito.mock(PaimonSnapshotCacheValue.class); + PaimonSnapshotCacheValue directProjection = Mockito.mock(PaimonSnapshotCacheValue.class); + AtomicInteger directProjectionLoads = new AtomicInteger(); + + try (MockedStatic paimonUtils = Mockito.mockStatic(PaimonUtils.class)) { + paimonUtils.when(() -> PaimonUtils.getPaimonTable(externalTable)).thenReturn(partitionedTable); + paimonUtils.when(() -> PaimonUtils.getLatestSnapshotCacheValue(externalTable)) + .thenReturn(memoizedProjection); + paimonUtils.when(() -> PaimonUtils.loadSnapshotProjection( + Mockito.eq(externalTable), Mockito.any(Table.class))).thenAnswer(invocation -> { + directProjectionLoads.incrementAndGet(); + return directProjection; + }); + + PaimonMvccSnapshot snapshot = (PaimonMvccSnapshot) externalTable.loadSnapshot( + Optional.empty(), Optional.of(scanParams)); + + Assert.assertSame(memoizedProjection, snapshot.getSnapshotCacheValue()); + Assert.assertEquals(0, directProjectionLoads.get()); + } + } + + @Test + public void testSystemTableIsBuiltFromTheValidatedSourceHandle() { + FileStoreTable safeSource = newFileStoreTable( + "safe_cached_source", ImmutableMap.of("scan.manifest.parallelism", "1"), null); + FileStoreTable unsafeReload = newFileStoreTable( + "unsafe_catalog_reload", ImmutableMap.of("scan.manifest.parallelism", "0"), null); + Table divergentWrapper = new PartitionsTable(unsafeReload); + PaimonExternalTable sourceTable = Mockito.mock(PaimonExternalTable.class); + PaimonExternalCatalog catalog = Mockito.mock(PaimonExternalCatalog.class); + PaimonExternalDatabase database = Mockito.mock(PaimonExternalDatabase.class); + Mockito.when(catalog.getId()).thenReturn(1L); + Mockito.when(database.getFullName()).thenReturn("db"); + Mockito.when(database.getRemoteName()).thenReturn("db"); + Mockito.when(sourceTable.getId()).thenReturn(10L); + Mockito.when(sourceTable.getName()).thenReturn("source"); + Mockito.when(sourceTable.getRemoteName()).thenReturn("source"); + Mockito.when(sourceTable.getCatalog()).thenReturn(catalog); + Mockito.when(sourceTable.getDatabase()).thenReturn(database); + Mockito.doReturn(safeSource).when(sourceTable).getBasePaimonTable(); + Mockito.when(catalog.getPaimonTable(Mockito.any(), Mockito.anyString(), Mockito.anyString())) + .thenReturn(divergentWrapper); + PaimonSysExternalTable systemTable = new PaimonSysExternalTable(sourceTable, "partitions"); + + Table actual = systemTable.getSysPaimonTable(); + + Assert.assertNotSame(divergentWrapper, actual); + Mockito.verify(catalog, Mockito.never()).getPaimonTable( + Mockito.any(), Mockito.anyString(), Mockito.anyString()); + } + @Test public void testPartitionsTableValidatesHiddenDataTableWithOverridePrecedence() throws Exception { FileStoreTable unsafeDataTable = newFileStoreTable( @@ -203,7 +267,7 @@ public void testExternalRowCountCacheUsesGuardedPaimonFetch() { Assert.assertFalse(planningStarted.get()); } - private PaimonSysExternalTable newSystemTable(FileStoreTable dataTable) throws Exception { + private PaimonSysExternalTable newSystemTable(FileStoreTable dataTable) { PaimonExternalTable sourceTable = Mockito.mock(PaimonExternalTable.class); PaimonExternalCatalog catalog = Mockito.mock(PaimonExternalCatalog.class); PaimonExternalDatabase database = Mockito.mock(PaimonExternalDatabase.class); @@ -216,11 +280,7 @@ private PaimonSysExternalTable newSystemTable(FileStoreTable dataTable) throws E Mockito.when(sourceTable.getCatalog()).thenReturn(catalog); Mockito.when(sourceTable.getDatabase()).thenReturn(database); Mockito.doReturn(dataTable).when(sourceTable).getBasePaimonTable(); - PaimonSysExternalTable systemTable = new PaimonSysExternalTable(sourceTable, "partitions"); - java.lang.reflect.Field field = PaimonSysExternalTable.class.getDeclaredField("paimonSysTable"); - field.setAccessible(true); - field.set(systemTable, new PartitionsTable(dataTable)); - return systemTable; + return new PaimonSysExternalTable(sourceTable, "partitions"); } private FileStoreTable newFileStoreTable( @@ -244,4 +304,19 @@ public ReadBuilder newReadBuilder() { } }; } + + private FileStoreTable newPartitionedFileStoreTable(String name) { + TableSchema schema = new TableSchema( + 0, + Arrays.asList( + new DataField(0, "id", new IntType()), + new DataField(1, "part", new IntType())), + 1, + Collections.singletonList("part"), + Collections.emptyList(), + Collections.emptyMap(), + null); + return new AppendOnlyFileStoreTable( + Mockito.mock(FileIO.class), new Path("memory://" + name), schema, CatalogEnvironment.empty()); + } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java index 7790e49e8deeb6..a890c59d5dfa19 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonReaderOptionsTest.java @@ -20,8 +20,11 @@ import org.apache.doris.datasource.property.metastore.AbstractPaimonProperties; import com.google.common.collect.ImmutableMap; +import org.apache.paimon.catalog.Identifier; import org.apache.paimon.fs.FileIO; import org.apache.paimon.fs.Path; +import org.apache.paimon.privilege.PrivilegeChecker; +import org.apache.paimon.privilege.PrivilegedFileStoreTable; import org.apache.paimon.schema.TableSchema; import org.apache.paimon.table.AppendOnlyFileStoreTable; import org.apache.paimon.table.CatalogEnvironment; @@ -111,6 +114,21 @@ void testSafeRelationOptionOverridesUnsafeHiddenFallbackTable() { fallbackReadTable, ImmutableMap.of("scan.manifest.parallelism", "1"))); } + @Test + void testRejectUnsafeFallbackHiddenByPrivilegeDelegate() { + FileStoreTable main = newFileStoreTable("privileged_main", Collections.emptyMap()); + FileStoreTable fallback = newFileStoreTable( + "privileged_fallback", ImmutableMap.of("scan.manifest.parallelism", "0")); + FileStoreTable fallbackReadTable = new FallbackReadFileStoreTable(main, fallback); + FileStoreTable privilegedTable = PrivilegedFileStoreTable.wrap( + fallbackReadTable, + Mockito.mock(PrivilegeChecker.class), + Identifier.create("db", "table")); + + Assertions.assertThrows(IllegalArgumentException.class, + () -> PaimonReaderOptions.validateEffectiveTable(privilegedTable)); + } + private FileStoreTable newFileStoreTable(String name, Map options) { TableSchema schema = new TableSchema( 0, From 0f07b707b698b7229d4f2826298e6790e5d19802 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Thu, 30 Jul 2026 12:59:42 +0800 Subject: [PATCH 6/6] [fix](paimon) Address late reader guardrail reviews ### What problem does this PR solve? Issue Number: None Related PR: #66247 Problem Summary: Relation-safe Paimon overrides could be rejected later when system-table descriptor serialization rebuilt schema from the unsafe physical handle or when memoized partition metadata revalidated reader-only physical settings. The shared V2 JNI path also encoded Paimon-only schema payloads for unrelated connectors, and the P0 success paths did not assert returned values. Keep schema serialization non-planning, restrict metadata validation to manifest planning settings, publish encoded schema only for Paimon, and add result-based coverage including the partitioned safe-override composition. ### Release note Paimon relation overrides remain effective through metadata and descriptor phases, while unrelated V2 JNI connectors avoid Paimon schema-encoding work. ### Check List (For Author) - Test: PaimonJniScannerTest (15 passed), FE checkstyle, regression framework compile, C++ format check, and diff checks passed. FE/BE unit and external Paimon regression coverage were added but not run locally because the required third-party toolchain is absent. - Behavior changed: Yes. Metadata-only paths no longer reject reader-only physical values after a safe relation override, and encoded schema publication is Paimon-specific. - Does this need documentation: No --- be/src/format_v2/jni/jni_table_reader.cpp | 25 ++++-- be/src/format_v2/jni/jni_table_reader.h | 1 + be/src/format_v2/jni/paimon_jni_reader.h | 1 + .../format_v2/jni/jni_table_reader_test.cpp | 13 +++ .../format_v2/jni/paimon_jni_reader_test.cpp | 14 ++++ .../paimon/PaimonPartitionInfoLoader.java | 2 +- .../paimon/PaimonReaderOptions.java | 12 +++ .../paimon/PaimonSysExternalTable.java | 4 +- .../paimon/PaimonExternalMetaCacheTest.java | 15 ++++ .../paimon/PaimonExternalTableTest.java | 5 ++ .../test_paimon_jni_reader_guardrails.out | 44 ++++++++++ .../test_paimon_jni_reader_guardrails.groovy | 83 +++++++++---------- 12 files changed, 165 insertions(+), 54 deletions(-) create mode 100644 regression-test/data/external_table_p0/paimon/test_paimon_jni_reader_guardrails.out diff --git a/be/src/format_v2/jni/jni_table_reader.cpp b/be/src/format_v2/jni/jni_table_reader.cpp index c41d9075bb3fab..b58696fe4ad895 100644 --- a/be/src/format_v2/jni/jni_table_reader.cpp +++ b/be/src/format_v2/jni/jni_table_reader.cpp @@ -496,13 +496,16 @@ Status JniTableReader::_set_open_scanner_batch_size(size_t batch_size) { } void JniTableReader::_prepare_jni_scanner_schema() { + const bool publish_encoded_schema = publishes_encoded_schema(); std::vector required_fields; std::vector column_types; std::vector encoded_column_types; std::vector replace_types; required_fields.reserve(_jni_columns.size()); column_types.reserve(_jni_columns.size()); - encoded_column_types.reserve(_jni_columns.size()); + if (publish_encoded_schema) { + encoded_column_types.reserve(_jni_columns.size()); + } replace_types.reserve(_jni_columns.size()); _jni_block_template.clear(); _jni_block_template.reserve(_jni_columns.size()); @@ -513,21 +516,25 @@ void JniTableReader::_prepare_jni_scanner_schema() { required_fields.push_back(column.java_name); column_types.push_back( JniDataBridge::get_jni_type_with_different_string(column.transfer_type)); - encoded_column_types.push_back( - JniDataBridge::get_jni_type_with_encoded_struct_fields(column.transfer_type)); + if (publish_encoded_schema) { + encoded_column_types.push_back( + JniDataBridge::get_jni_type_with_encoded_struct_fields(column.transfer_type)); + } replace_types.push_back(column.replace_type); has_replace_type = has_replace_type || column.replace_type != "not_replace"; _jni_block_template.insert( {column.transfer_type->create_column(), column.transfer_type, column.java_name}); } _scanner_params["required_fields"] = join(required_fields, ","); - // Preserve the legacy field during rolling upgrades, while new scanners use independently - // encoded identifiers so commas and other legal identifier characters cannot alter framing. - _scanner_params["required_fields_base64"] = - JniDataBridge::encode_schema_values(required_fields); _scanner_params["columns_types"] = join(column_types, "#"); - _scanner_params["columns_types_base64"] = - JniDataBridge::encode_schema_values(encoded_column_types); + if (publish_encoded_schema) { + // Only Paimon consumes the paired payload. Keeping it capability-gated avoids recursively + // encoding nested types for every split of unrelated V2 JNI connectors. + _scanner_params["required_fields_base64"] = + JniDataBridge::encode_schema_values(required_fields); + _scanner_params["columns_types_base64"] = + JniDataBridge::encode_schema_values(encoded_column_types); + } if (has_replace_type) { _scanner_params["replace_string"] = join(replace_types, ","); } diff --git a/be/src/format_v2/jni/jni_table_reader.h b/be/src/format_v2/jni/jni_table_reader.h index 5e48270515f996..df7edc92f985fb 100644 --- a/be/src/format_v2/jni/jni_table_reader.h +++ b/be/src/format_v2/jni/jni_table_reader.h @@ -81,6 +81,7 @@ class JniTableReader : public TableReader { virtual Status _close_jni_scanner(); virtual Status _set_open_scanner_batch_size(size_t batch_size); virtual bool supports_batch_size_update_after_open() const { return true; } + virtual bool publishes_encoded_schema() const { return false; } virtual Status _open_jni_scanner(); bool _reserve_split_profile_publication(); const std::vector& jni_columns() const { return _jni_columns; } diff --git a/be/src/format_v2/jni/paimon_jni_reader.h b/be/src/format_v2/jni/paimon_jni_reader.h index 26767458b427db..04dd3f0be99337 100644 --- a/be/src/format_v2/jni/paimon_jni_reader.h +++ b/be/src/format_v2/jni/paimon_jni_reader.h @@ -51,6 +51,7 @@ class PaimonJniReader final : public format::JniTableReader { Status validate_scan_range(const TFileRangeDesc& range) const override; Status build_scanner_params(std::map* params) const override; bool supports_batch_size_update_after_open() const override { return false; } + bool publishes_encoded_schema() const override { return true; } private: static std::string build_default_io_manager_tmp_dirs(const std::vector& store_paths); diff --git a/be/test/format_v2/jni/jni_table_reader_test.cpp b/be/test/format_v2/jni/jni_table_reader_test.cpp index 68567675473ae5..03c4a99c584232 100644 --- a/be/test/format_v2/jni/jni_table_reader_test.cpp +++ b/be/test/format_v2/jni/jni_table_reader_test.cpp @@ -129,6 +129,19 @@ TEST(JniTableReaderTest, EncodedTypeDescriptorsPreserveNestedQuotedIdentifiers) "struct<$aGFzaCNuYW1l:string,$cmVnaW9uLGNvZGU:string,$Y29sb246bmFtZQ:string>"); } +TEST(JniTableReaderTest, GenericConnectorDoesNotPublishPaimonEncodedSchema) { + FakeJniTableReader reader; + reader._jni_columns = {JniTableReader::JniColumn { + .java_name = "region,code", + .transfer_type = std::make_shared(), + }}; + + reader._prepare_jni_scanner_schema(); + + EXPECT_FALSE(reader._scanner_params.contains("required_fields_base64")); + EXPECT_FALSE(reader._scanner_params.contains("columns_types_base64")); +} + TEST(JniTableReaderTest, CancellationStopsBeforeFetchingAnotherJavaBatch) { auto io_ctx = std::make_shared(); FakeJniTableReader reader; diff --git a/be/test/format_v2/jni/paimon_jni_reader_test.cpp b/be/test/format_v2/jni/paimon_jni_reader_test.cpp index 9c46aba05c76e1..4e27864e2cfd02 100644 --- a/be/test/format_v2/jni/paimon_jni_reader_test.cpp +++ b/be/test/format_v2/jni/paimon_jni_reader_test.cpp @@ -23,6 +23,7 @@ #include #include +#include "core/data_type/data_type_string.h" #include "format_v2/table_reader.h" #include "gen_cpp/PlanNodes_types.h" #include "runtime/runtime_state.h" @@ -161,6 +162,19 @@ TEST(PaimonJniReaderTest, ScanLevelOptionsOverrideLegacySplitFallbacks) { EXPECT_EQ(params["hadoop.source"], "scan"); } +TEST(PaimonJniReaderTest, PublishesEncodedSchemaForQuotedIdentifiers) { + PaimonJniReader reader; + reader._jni_columns = {JniTableReader::JniColumn { + .java_name = "region,code", + .transfer_type = std::make_shared(), + }}; + + reader._prepare_jni_scanner_schema(); + + EXPECT_EQ(reader._scanner_params.at("required_fields_base64"), "$cmVnaW9uLGNvZGU="); + EXPECT_TRUE(reader._scanner_params.contains("columns_types_base64")); +} + TEST(PaimonJniReaderTest, UsesStableRuntimeBatchSizeBeforeAndAfterOpen) { TQueryOptions query_options; query_options.__set_batch_size(8160); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/paimon/PaimonPartitionInfoLoader.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/paimon/PaimonPartitionInfoLoader.java index 766a9b62f536fd..9053aae1920841 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/paimon/PaimonPartitionInfoLoader.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/paimon/PaimonPartitionInfoLoader.java @@ -47,7 +47,7 @@ public PaimonPartitionInfo load(NameMapping nameMapping, Table paimonTable, List try { // Catalog.listPartitions reloads the raw physical table and loses Doris catalog/relation // copies. Enumerate the already merged handle so validation and planning see one table. - PaimonReaderOptions.validateEffectiveTable(paimonTable); + PaimonReaderOptions.validateEffectivePlanningTable(paimonTable); List paimonPartitions = CatalogUtils.listPartitionsFromFileSystem(paimonTable); boolean legacyPartitionName = PaimonUtil.isLegacyPartitionName(paimonTable); return PaimonUtil.generatePartitionInfo(partitionColumns, paimonPartitions, legacyPartitionName); diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java index b898f55f465957..60281dfa0292b0 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonReaderOptions.java @@ -126,6 +126,18 @@ public static void validateEffectiveTable(Table table) { } } + public static void validateEffectivePlanningTable(Table table) { + // Partition projection never opens a data reader. Revalidating batch/async values here + // would reject a raw handle even when the final relation copy safely overrides them. + validateManifestParallelism(table.options().get(CoreOptions.SCAN_MANIFEST_PARALLELISM.key())); + if (table instanceof FallbackReadFileStoreTable) { + validateEffectivePlanningTable(((FallbackReadFileStoreTable) table).fallback()); + } + if (table instanceof DelegatedFileStoreTable) { + validateEffectivePlanningTable(((DelegatedFileStoreTable) table).wrapped()); + } + } + private static void validateManifestParallelism(String value) { if (value == null) { return; diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java index df36b23fce0504..8c6fa89807fec6 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSysExternalTable.java @@ -315,7 +315,9 @@ private SchemaCacheValue getOrCreateSchemaCacheValue() { } private List buildFullSchema() { - Table sysTable = getSysPaimonTable(); + // Schema and descriptor construction do not plan a scan. Using the raw wrapper keeps a + // relation-scoped safe override from being replaced by validation of the physical handle. + Table sysTable = getRawSysPaimonTable(); List fields = sysTable.rowType().getFields(); List columns = Lists.newArrayListWithCapacity(fields.size()); diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalMetaCacheTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalMetaCacheTest.java index 6a018f8e06742f..06b32ea9d07815 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalMetaCacheTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalMetaCacheTest.java @@ -114,6 +114,21 @@ public void testPartitionProjectionUsesSafeCopiedTableInsteadOfRawCatalogReload( Assert.assertTrue(partitionInfo.getNameToPartition().isEmpty()); } + @Test + public void testPartitionProjectionIgnoresReaderOnlyPhysicalOptions() throws Exception { + FileStoreTable table = newPartitionedTable( + "reader_only", Collections.singletonMap("read.batch-size", "0")); + PaimonPartitionInfoLoader loader = new PaimonPartitionInfoLoader(); + + // Partition metadata does not use the data-reader batch size. The final relation copy is + // validated separately before scanning, so a safe override can reuse this projection. + PaimonPartitionInfo partitionInfo = loader.load( + new NameMapping(1L, "db", "table", "db", "table"), table, + Collections.singletonList(new Column("part", Type.INT))); + + Assert.assertTrue(partitionInfo.getNameToPartition().isEmpty()); + } + private FileStoreTable newPartitionedTable(String name, Map options) throws Exception { TableSchema schema = new TableSchema( 0, diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java index 42ed98324f97b6..ba5257cd24e256 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/PaimonExternalTableTest.java @@ -224,6 +224,9 @@ public void testPartitionsTableValidatesHiddenDataTableWithOverridePrecedence() Collections.emptyList()); Assert.assertTrue(systemTable.getSysPaimonTable(safeOverride) instanceof PartitionsTable); Assert.assertFalse(systemTable.isDataTable()); + // Descriptor serialization happens after relation binding and must not fall back to + // validating the unsafe physical handle that the relation override already replaced. + Assert.assertNotNull(systemTable.toThrift()); } @Test @@ -279,6 +282,8 @@ private PaimonSysExternalTable newSystemTable(FileStoreTable dataTable) { Mockito.when(sourceTable.getRemoteName()).thenReturn("source"); Mockito.when(sourceTable.getCatalog()).thenReturn(catalog); Mockito.when(sourceTable.getDatabase()).thenReturn(database); + Mockito.when(sourceTable.getPaimonCatalogType()) + .thenReturn(PaimonExternalCatalog.PAIMON_FILESYSTEM); Mockito.doReturn(dataTable).when(sourceTable).getBasePaimonTable(); return new PaimonSysExternalTable(sourceTable, "partitions"); } diff --git a/regression-test/data/external_table_p0/paimon/test_paimon_jni_reader_guardrails.out b/regression-test/data/external_table_p0/paimon/test_paimon_jni_reader_guardrails.out new file mode 100644 index 00000000000000..b149c4535db0e6 --- /dev/null +++ b/regression-test/data/external_table_p0/paimon/test_paimon_jni_reader_guardrails.out @@ -0,0 +1,44 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !scanner_v2_all_identifiers -- +1 east,01 hash-one first row 华东 nested-one east,01 a:1 +2 west,02 hash-two second row 华西 nested-two west,02 b:2 + +-- !scanner_v1_quoted_nested -- +1 east,01 nested-one east,01 a:1 +2 west,02 nested-two west,02 b:2 + +-- !scanner_v1_empty_identifier -- +empty-name + +-- !scanner_v2_quoted_nested -- +1 east,01 nested-one east,01 a:1 +2 west,02 nested-two west,02 b:2 + +-- !scanner_v2_empty_identifier -- +empty-name + +-- !catalog_override_physical_batch -- +1 + +-- !relation_reader_options -- +1 +2 + +-- !relation_option_isolation -- +1 1 +2 2 + +-- !failed_alter_preserves_catalog -- +2 + +-- !relation_override_physical_batch -- +1 + +-- !partitioned_relation_override_physical_batch -- +1 20 + +-- !relation_override_physical_manifest -- +1 10 + +-- !system_table_descriptor_with_safe_override -- +1 diff --git a/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy b/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy index e5a9deabee17e1..87bd23a60edd76 100644 --- a/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy +++ b/regression-test/suites/external_table_p0/paimon/test_paimon_jni_reader_guardrails.groovy @@ -88,6 +88,11 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { create table paimon.${dbName}.unsafe_physical_batch (id int) using paimon tblproperties ('read.batch-size'='0'); insert into paimon.${dbName}.unsafe_physical_batch values (1); + drop table if exists paimon.${dbName}.unsafe_partitioned_batch; + create table paimon.${dbName}.unsafe_partitioned_batch (id int, part int) using paimon + partitioned by (part) + tblproperties ('read.batch-size'='0'); + insert into paimon.${dbName}.unsafe_partitioned_batch values (1, 20); drop table if exists paimon.${dbName}.unsafe_physical_manifest; create table paimon.${dbName}.unsafe_physical_manifest (id int, part int) using paimon partitioned by (part) @@ -101,61 +106,50 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { sql "switch ${catalogName}" sql "use ${dbName}" sql "set force_jni_scanner=true" + sql "set enable_file_scanner_v2=true" - test { - sql """ - select `region,code`, `hash#name`, `display name`, `地区 名`, `nested#value` + order_qt_scanner_v2_all_identifiers """ + select id, `region,code`, `hash#name`, `display name`, `地区 名`, + `nested#value`.`hash#name`, `nested#value`.`region,code`, + `nested#value`.`colon:name` from quoted_reader_options where `region,code` in ('east,01', 'west,02') order by id - """ - } + """ sql "set enable_file_scanner_v2=false" - test { - sql """ - select `region,code`, `nested#value` + order_qt_scanner_v1_quoted_nested """ + select id, `region,code`, `nested#value`.`hash#name`, + `nested#value`.`region,code`, `nested#value`.`colon:name` from quoted_reader_options order by id - """ - } - test { - sql "select * from empty_identifier" - } + """ + qt_scanner_v1_empty_identifier "select * from empty_identifier" sql "set enable_file_scanner_v2=true" - test { - sql """ - select `region,code`, `nested#value` + order_qt_scanner_v2_quoted_nested """ + select id, `region,code`, `nested#value`.`hash#name`, + `nested#value`.`region,code`, `nested#value`.`colon:name` from quoted_reader_options order by id - """ - } - test { - sql "select * from empty_identifier" - } + """ + qt_scanner_v2_empty_identifier "select * from empty_identifier" // The safe catalog value must override the physical read.batch-size=0 value. - test { - sql "select * from unsafe_physical_batch order by id" - } + order_qt_catalog_override_physical_batch "select * from unsafe_physical_batch order by id" - test { - sql """ + order_qt_relation_reader_options """ select id from quoted_reader_options@options( 'read.batch-size'='4096', 'file-reader-async-threshold'='32 MB') order by id - """ - } - test { - sql """ + """ + order_qt_relation_option_isolation """ select small.id, large.id from quoted_reader_options@options('read.batch-size'='1') small join quoted_reader_options@options('read.batch-size'='8192') large on small.id = large.id order by small.id - """ - } + """ for (def invalidOption : [ ["read.batch-size", "0"], @@ -183,9 +177,7 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { } // A failed ALTER must not leave read.batch-size=0 behind; on the old path this follow-up // JNI scan can stop making progress instead of completing. - test { - sql "select count(*) from quoted_reader_options" - } + qt_failed_alter_preserves_catalog "select count(*) from quoted_reader_options" sql "drop catalog if exists ${physicalCatalogName}" sql(catalogDdl(physicalCatalogName, "")) @@ -195,26 +187,31 @@ suite("test_paimon_jni_reader_guardrails", "p0,external,paimon") { sql "select * from unsafe_physical_batch" exception "read.batch-size" } + order_qt_relation_override_physical_batch """ + select * from unsafe_physical_batch@options('read.batch-size'='4096') order by id + """ test { - sql "select * from unsafe_physical_batch@options('read.batch-size'='4096') order by id" + sql "select * from unsafe_partitioned_batch" + exception "read.batch-size" } + order_qt_partitioned_relation_override_physical_batch """ + select * from unsafe_partitioned_batch@options('read.batch-size'='4096') order by id + """ test { sql "select * from unsafe_physical_manifest" exception "scan.manifest.parallelism" } - test { - sql "select * from unsafe_physical_manifest@options('scan.manifest.parallelism'='1') order by id" - } + order_qt_relation_override_physical_manifest """ + select * from unsafe_physical_manifest@options('scan.manifest.parallelism'='1') order by id + """ test { sql "select count(*) from unsafe_physical_manifest\$partitions" exception "scan.manifest.parallelism" } - test { - sql """ + qt_system_table_descriptor_with_safe_override """ select count(*) from unsafe_physical_manifest\$partitions @options('scan.manifest.parallelism'='1') - """ - } + """ } finally { sql "set force_jni_scanner=false" sql "set enable_file_scanner_v2=${originalScannerV2}"