github-actions[bot] commented on code in PR #65984:
URL: https://github.com/apache/doris/pull/65984#discussion_r3649822706
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalFileScan.java:
##########
@@ -206,16 +220,22 @@ public List<Slot> computeOutput() {
if (table instanceof IcebergExternalTable) {
// iceberg v3 need append row lineage columns
- return computeIcebergOutput((IcebergExternalTable) table);
+ return computeIcebergOutput();
+ } else if (scanParams.isPresent() && scanParams.get().isOptions()
+ && (table instanceof PaimonExternalTable || table instanceof
PaimonSysExternalTable)) {
+ List<Column> schema = table instanceof PaimonSysExternalTable
+ ? ((PaimonSysExternalTable)
table).getFullSchema(scanParams.get())
+ : ((PaimonExternalTable)
table).getFullSchema(scanParams.get());
Review Comment:
[P1] Preload selector-free OPTIONS before locking
A relation such as `p@options('scan.plan-sort-partition'='true')` is still
marked non-latest merely because `scanParams` is present. For a Paimon table
referenced only this way, `PreloadExternalMetadata` therefore skips
latest-snapshot and schema warmup. `BindRelation` then calls
`StatementContext.loadSnapshots` only after internal table locks are acquired;
for this selector-free form, `PaimonExternalTable.loadSnapshot` still fetches
the latest Paimon snapshot, so cold catalog/filesystem metadata I/O occurs
inside the lock interval before this output is built. The execution path
already treats this option as statement-pinned latest state, so the preload
classification should use the same selector/startup distinction (or preload the
exact relation state before locking). Please cover a mixed internal/Paimon
query with selector-free OPTIONS and cold metadata.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalFileScan.java:
##########
@@ -206,16 +220,22 @@ public List<Slot> computeOutput() {
if (table instanceof IcebergExternalTable) {
// iceberg v3 need append row lineage columns
- return computeIcebergOutput((IcebergExternalTable) table);
+ return computeIcebergOutput();
+ } else if (scanParams.isPresent() && scanParams.get().isOptions()
Review Comment:
[P1] Exclude parameterized scans from MTMV rewrite
This historical relation still looks identical to an ordinary latest
`LogicalFileScan` to async-MV rewrite: `CollectRelation` collects MTMVs by the
underlying table, `TableQueryOperatorChecker` rejects only file scans with
`TABLESAMPLE`, and `RelationMapping` keys relations only by table identity.
Thus an aggregate over `t@options('scan.snapshot-id'='1')` can be replaced by
an MTMV refreshed from latest `t`, returning latest rows before a Paimon scan
ever sees the requested snapshot. Please treat non-empty scan params as a table
query operator (unless the MV proves identical semantics), and add a Paimon
MTMV regression whose historical and latest aggregates differ.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java:
##########
@@ -0,0 +1,175 @@
+// 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.analysis.TableScanParams;
+
+import com.google.common.collect.ImmutableSet;
+import org.apache.paimon.CoreOptions;
+import org.apache.paimon.table.Table;
+
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+/**
+ * Validation and application rules for relation-scoped Paimon scan parameters.
+ */
+public final class PaimonScanParams {
+ private static final Set<String> QUERY_OPTION_KEYS =
CoreOptions.getOptions().stream()
+ .map(option -> option.key())
+ .filter(key -> key.startsWith("scan."))
+ .collect(Collectors.toSet());
+
+ private static final Set<String> STARTUP_POSITION_KEYS = ImmutableSet.of(
+ 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());
+
+ private static final Set<String> INHERITED_READ_STATE_KEYS =
ImmutableSet.<String>builder()
+ .addAll(STARTUP_POSITION_KEYS)
+ .add(CoreOptions.SCAN_MODE.key())
+ .add(CoreOptions.SCAN_BOUNDED_WATERMARK.key())
+ .add(CoreOptions.INCREMENTAL_BETWEEN.key())
+ .add(CoreOptions.INCREMENTAL_BETWEEN_TIMESTAMP.key())
+ .add(CoreOptions.INCREMENTAL_BETWEEN_SCAN_MODE.key())
+ .add(CoreOptions.INCREMENTAL_TO_AUTO_TAG.key())
+ .build();
+
+ private static final Set<String> INCREMENTAL_SYSTEM_TABLES =
ImmutableSet.of(
+ "audit_log", "binlog", "row_tracking");
+
+ private static final Set<String> OPTIONS_SYSTEM_TABLES = ImmutableSet.of(
+ "audit_log", "binlog", "files", "manifests", "partitions", "ro",
"row_tracking");
+
+ private PaimonScanParams() {
+ }
+
+ public static void validateOptions(Map<String, String> options) {
+ if (options.containsKey(CoreOptions.SCAN_FALLBACK_BRANCH.key())) {
+ throw new IllegalArgumentException("Paimon query option '"
+ + CoreOptions.SCAN_FALLBACK_BRANCH.key()
+ + "' is not supported because it requires rebuilding the
table through the catalog factory.");
+ }
+
+ Set<String> unsupported = options.keySet().stream()
+ .filter(key -> !QUERY_OPTION_KEYS.contains(key))
+ .collect(Collectors.toSet());
+ if (!unsupported.isEmpty()) {
+ throw new IllegalArgumentException("Unsupported Paimon query
option(s): " + unsupported);
+ }
+
+ long positionCount =
options.keySet().stream().filter(STARTUP_POSITION_KEYS::contains).count();
+ if (positionCount > 1) {
+ throw new IllegalArgumentException(
+ "Only one Paimon startup position can be specified: " +
STARTUP_POSITION_KEYS);
+ }
+
+ if (options.containsKey(CoreOptions.SCAN_MODE.key()) && positionCount
== 1) {
+ String position = options.keySet().stream()
+ .filter(STARTUP_POSITION_KEYS::contains)
+ .findFirst()
+ .get();
+ String mode =
options.get(CoreOptions.SCAN_MODE.key()).toLowerCase(Locale.ROOT);
+ if (!isCompatibleStartupMode(position, mode)) {
+ throw new IllegalArgumentException("Paimon scan mode '" + mode
+ + "' is incompatible with startup position '" +
position + "'.");
+ }
+ }
+ }
+
+ public static Table applyOptions(Table table, Map<String, String> options)
{
+ validateOptions(options);
+ Map<String, String> isolatedOptions = new HashMap<>(options);
+ if (hasStartupOptions(options)) {
+ // Startup mode, position, and range form one inherited state
family in Paimon. Clear
+ // absent members so one relation cannot accidentally reuse
another relation's read state.
+ INHERITED_READ_STATE_KEYS.stream()
Review Comment:
[P2] Clear Paimon's fallback scan keys too
This isolation removes only canonical option names, but Paimon 1.3.1 still
resolves `SCAN_MODE` from `log.scan` and `SCAN_TIMESTAMP_MILLIS` from
`log.scan.timestamp-millis` when their canonical keys are absent. `Table.copy`
removes exact string keys only, so a table or
`paimon.table-default.log.scan=latest` survives this cleanup and makes
`@options('scan.snapshot-id'='1')` conflict with `LATEST`; the legacy timestamp
alias can similarly survive a different selector or an INCR isolation. Please
clear each relevant `ConfigOption`'s fallback keys as part of the same state
family and add legacy-default tests for ordinary OPTIONS and system-table INCR.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java:
##########
@@ -106,6 +106,26 @@ public Table getPaimonTable(Optional<MvccSnapshot>
snapshot) {
}
}
+ public Table getPaimonTable(TableScanParams scanParams) {
+ if (scanParams != null && scanParams.isOptions()) {
+ Map<String, String> options = scanParams.getMapParams();
+ // Behavioral options must retain the statement-pinned handle;
only an explicit startup
+ // choice is allowed to replace that MVCC position with
relation-local read state.
+ Table table = PaimonScanParams.hasStartupOptions(options)
Review Comment:
[P1] Pin live-resolved startup options across both phases
`hasStartupOptions()` sends every mode or position to this live base handle.
In Paimon 1.3.1 batch planning, mode-only `latest`, `default`, and `full` scan
whatever snapshot is latest when `newScan()` runs, so an alias can read S+1
while another alias remains on statement-pinned S. A fixed future
`scan.timestamp-millis=T` is also resolved separately during output binding and
scan initialization; an intervening S+1 commit below T makes the unchanged
option bind S but read S+1. Creation-time forms likewise remain live until
planning. Please keep mode-only latest state on statement MVCC and resolve
every time-relative relation to one immutable snapshot/schema carried through
execution, including system-table wrappers. Add commit-between-phases tests for
mode-only latest and a fixed future timestamp.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java:
##########
@@ -0,0 +1,175 @@
+// 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.analysis.TableScanParams;
+
+import com.google.common.collect.ImmutableSet;
+import org.apache.paimon.CoreOptions;
+import org.apache.paimon.table.Table;
+
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+/**
+ * Validation and application rules for relation-scoped Paimon scan parameters.
+ */
+public final class PaimonScanParams {
+ private static final Set<String> QUERY_OPTION_KEYS =
CoreOptions.getOptions().stream()
+ .map(option -> option.key())
+ .filter(key -> key.startsWith("scan."))
+ .collect(Collectors.toSet());
+
+ private static final Set<String> STARTUP_POSITION_KEYS = ImmutableSet.of(
+ 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());
+
+ private static final Set<String> INHERITED_READ_STATE_KEYS =
ImmutableSet.<String>builder()
+ .addAll(STARTUP_POSITION_KEYS)
+ .add(CoreOptions.SCAN_MODE.key())
+ .add(CoreOptions.SCAN_BOUNDED_WATERMARK.key())
+ .add(CoreOptions.INCREMENTAL_BETWEEN.key())
+ .add(CoreOptions.INCREMENTAL_BETWEEN_TIMESTAMP.key())
+ .add(CoreOptions.INCREMENTAL_BETWEEN_SCAN_MODE.key())
+ .add(CoreOptions.INCREMENTAL_TO_AUTO_TAG.key())
+ .build();
+
+ private static final Set<String> INCREMENTAL_SYSTEM_TABLES =
ImmutableSet.of(
Review Comment:
[P2] Retain INCR for range-sensitive system tables
This allowlist now rejects `$files`, `$partitions`, and `$ro`, although
their Paimon 1.3.1 implementations copy the wrapped data table and consume its
scan state. `FilesRead` replans the copied table, `PartitionsRead` calls its
`newScan().listPartitionEntries()`, and `ReadOptimizedTable.newScan()` builds a
batch scan from the copied options, so `incremental-between` changes each
result. This also prevents the processed-table fix requested in the existing
`$files@incr` thread from ever being exercised. Please derive capability from
wrapper behavior or include these three types, and add exact range-result tests.
--
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]
