github-actions[bot] commented on code in PR #65955:
URL: https://github.com/apache/doris/pull/65955#discussion_r3639164831
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractPaimonProperties.java:
##########
@@ -121,6 +140,90 @@ public Map<String, String> getCatalogOptionsMap() {
}
}
+ public Map<String, String> getTableOptionsMap() {
+ return tableOptionsMap;
+ }
+
+ public static boolean isTableOptionProperty(String key) {
+ return key.toLowerCase(Locale.ROOT).startsWith(TABLE_OPTION_PREFIX);
+ }
+
+ private Map<String, String> extractTableOptions() {
+ Map<String, String> 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) {
+ 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);
Review Comment:
[P1] Reject a zero Paimon read batch before scanning
`Options.get()` checks only that this value is an integer, so
`paimon.table-option.read.batch-size=0` is accepted and the new `putIfAbsent`
path preserves it. In bundled Paimon 1.3.1, the Parquet reader then computes a
zero-row batch, returns success without advancing `rowsReturned`, and repeats
that result. Doris consumes the empty iterator and immediately calls
`readBatch()` again, so a nonempty Parquet scan spins indefinitely. Please
require `read.batch-size > 0` during CREATE/ALTER validation and cover
zero/non-positive values with a reader-progress regression.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractPaimonProperties.java:
##########
@@ -121,6 +140,90 @@ public Map<String, String> getCatalogOptionsMap() {
}
}
+ public Map<String, String> getTableOptionsMap() {
+ return tableOptionsMap;
+ }
+
+ public static boolean isTableOptionProperty(String key) {
+ return key.toLowerCase(Locale.ROOT).startsWith(TABLE_OPTION_PREFIX);
+ }
+
+ private Map<String, String> extractTableOptions() {
+ Map<String, String> 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) {
+ 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);
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException("Invalid value for Paimon table
option '" + key + "': "
+ + e.getMessage(), e);
+ }
+ }
+
+ private static final class SupportedTableOptions {
+ private final Map<String, ConfigOption<?>> exactOptions;
+ private final Map<String, ConfigOption<?>> prefixOptions;
+
+ private SupportedTableOptions(Map<String, ConfigOption<?>>
exactOptions,
+ Map<String, ConfigOption<?>> prefixOptions) {
+ this.exactOptions = exactOptions;
+ this.prefixOptions = prefixOptions;
+ }
+
+ private static SupportedTableOptions build() {
+ Map<String, ConfigOption<?>> exactOptions = new HashMap<>();
+ Map<String, ConfigOption<?>> prefixOptions = new HashMap<>();
+ for (ConfigOption<?> option : CoreOptions.getOptions()) {
Review Comment:
[P1] Only admit options legal for Paimon dynamic copies
`CoreOptions.getOptions()` includes `bucket` and other immutable table
options, so for example `paimon.table-option.bucket=4` passes this validator.
On first table load, `PaimonExternalCatalog` calls `table.copy(tableOptions)`;
bundled Paimon 1.3.1 rejects changed `bucket` in dynamic options and rejects
its immutable option set. Catalog creation can therefore succeed while a table
with a different stored value fails metadata/query access. Please build the
allow-list from options Paimon permits in `Table.copy` (canonicalizing
fallbacks), or reject immutable/bucket options during CREATE/ALTER, and add an
end-to-end negative test.
##########
be/src/exec/operator/file_scan_operator.cpp:
##########
@@ -116,14 +116,9 @@ bool FileScanLocalState::_should_use_file_scanner_v2(const
TQueryOptions& query_
const bool is_transactional_hive =
scan_params.__isset.table_format_params &&
scan_params.table_format_params.table_format_type ==
"transactional_hive";
- // JNI reader selection is stored per split, but this scan-level selector
cannot inspect the
- // split yet. Older FEs may omit both the scan-level Paimon marker and
split-level reader_type,
- // so keep JNI scans on V1 until scanner selection can distinguish every
compatibility shape.
return query_options.__isset.enable_file_scanner_v2 &&
query_options.enable_file_scanner_v2 &&
!is_load && scan_params.format_type != TFileFormatType::FORMAT_WAL
&&
- scan_params.format_type != TFileFormatType::FORMAT_ES_HTTP &&
- scan_params.format_type != TFileFormatType::FORMAT_LANCE &&
- scan_params.format_type != TFileFormatType::FORMAT_JNI &&
!is_transactional_hive;
+ scan_params.format_type != TFileFormatType::FORMAT_ES_HTTP &&
!is_transactional_hive;
Review Comment:
[P1] Keep JNI and Lance on V1 until V2 is semantically ready
This scan-level switch activates V2 for every eligible JNI/Lance scan, but
both capability and residual semantics are incomplete. V2 has no Lance case,
rejects LakeSoul, Paimon `PAIMON_CPP`, and legacy Paimon without `reader_type`,
then returns `NotSupported` with no V1 fallback; unchanged selector tests at
`file_scanner_v2_test.cpp:420-461` still require those shapes to stay on V1.
Even admitted JNI readers clone every scanner conjunct and filter it in
`finalize_jni_block()`, after which `Scanner::_filter_output_block()` evaluates
the original again, so a non-deterministic predicate such as `rand()` is
applied twice (unlike V1). Please keep JNI/Lance on V1 until selection is
capability-gated and the JNI residual path executes each predicate once, with
tests for unsupported shapes and non-deterministic predicates.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -93,9 +93,9 @@ public class PaimonScanNode extends FileQueryScanNode {
private static final String DORIS_INCREMENTAL_BETWEEN_SCAN_MODE =
"incrementalBetweenScanMode";
private static final String PAIMON_PROPERTY_PREFIX = "paimon.";
private static final String DORIS_ENABLE_FILE_READER_ASYNC =
"jni.enable_file_reader_async";
- private static final String DORIS_ENABLE_JNI_IO_MANAGER =
"doris.enable_jni_io_manager";
- private static final String DORIS_JNI_IO_MANAGER_TMP_DIR =
"doris.jni_io_manager.tmp_dir";
- private static final String DORIS_JNI_IO_MANAGER_IMPL_CLASS =
"doris.jni_io_manager.impl_class";
+ private static final String DORIS_ENABLE_JNI_IO_MANAGER =
"jni.enable_jni_io_manager";
Review Comment:
[P1] Preserve the legacy IO-manager keys across upgrades
These names are persisted catalog properties and cross-version protocol
inputs, not internal-only constants. Existing catalogs store `paimon.doris.*`,
but the new FE now searches only `paimon.jni.*`, so the option disappears and
the Java scanner silently defaults the IO manager to disabled. Old-FE/new-BE
and new-FE/old-BE pairs have the same inverse spelling mismatch. Please accept
both namespaces for a compatibility window, define new-key precedence,
normalize at FE/BE/Java boundaries, and retain old-catalog plus mixed-version
tests.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java:
##########
@@ -134,7 +134,11 @@ public org.apache.paimon.table.Table
getPaimonTable(NameMapping nameMapping, Str
} else {
identifier = new Identifier(nameMapping.getRemoteDbName(),
nameMapping.getRemoteTblName());
}
- return executionAuthenticator.execute(() ->
catalog.getTable(identifier));
+ return executionAuthenticator.execute(() -> {
+ Table table = catalog.getTable(identifier);
+ Map<String, String> tableOptions =
paimonProperties.getTableOptionsMap();
+ return tableOptions.isEmpty() ? table :
table.copy(tableOptions);
Review Comment:
[P1] Exclude context selectors from catalog table options
Reflecting every `CoreOptions` admits options that cannot be safely
persisted and applied after load. `branch=dev` reaches this copy after main's
`TableSchema` is loaded; Paimon retains that schema while branch managers/files
switch to dev, so independent branch schemas can mismatch. Likewise a catalog
`scan.tag-name` remains when explicit `FOR VERSION AS OF` later adds
`scan.snapshot-id`; `copy()` merges both and Paimon rejects the mutually
exclusive selectors (incremental overlays leave the same stale keys). Please
restrict this namespace to context-free read tuning options; keep branch
selection on the `Identifier` path and time-travel/incremental selectors
query-scoped, with cross-branch-schema and cross-kind selector tests.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractPaimonProperties.java:
##########
@@ -121,6 +140,90 @@ public Map<String, String> getCatalogOptionsMap() {
}
}
+ public Map<String, String> getTableOptionsMap() {
+ return tableOptionsMap;
+ }
+
+ public static boolean isTableOptionProperty(String key) {
+ return key.toLowerCase(Locale.ROOT).startsWith(TABLE_OPTION_PREFIX);
+ }
+
+ private Map<String, String> extractTableOptions() {
+ Map<String, String> 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) {
+ 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);
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException("Invalid value for Paimon table
option '" + key + "': "
Review Comment:
[P1] Roll back catalog state for validation exceptions
This new `IllegalArgumentException` escapes the existing ALTER rollback
contract. `CatalogMgr.replayAlterCatalogProps()` first calls
`tryModifyCatalogProps()` (which mutates the map and resets its caches), but
restores `oldProperties` only from `catch (DdlException)`. A rejected unknown
or malformed table option is therefore not edit-logged yet remains in the
running FE; an unknown key cannot be removed by another SET and can keep
property reconstruction failing until restart/recreation. Please translate
these failures to `DdlException` or roll back on every validation exception,
and add an ALTER-level test that verifies the prior catalog state remains
usable after rejection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
