github-actions[bot] commented on code in PR #65984:
URL: https://github.com/apache/doris/pull/65984#discussion_r3649596579
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java:
##########
@@ -106,6 +106,22 @@ public Table getPaimonTable(Optional<MvccSnapshot>
snapshot) {
}
}
+ public Table getPaimonTable(TableScanParams scanParams) {
+ if (scanParams != null && scanParams.isOptions()) {
+ // Statement MVCC is keyed by table, while OPTIONS belongs to one
relation. Start
+ // from the catalog handle so its schema cannot inherit another
relation's selector.
+ return PaimonScanParams.applyOptions(getBasePaimonTable(),
scanParams.getMapParams());
Review Comment:
[P1] Preserve statement MVCC for selector-free OPTIONS
`BindRelation` has already stored one exact Paimon snapshot per table, but
this branch discards it even when the option is not a snapshot selector. If
snapshot S is captured and S+1 commits before scan initialization, a plain
alias keeps S through `PaimonSource`, while an alias with
`@options('scan.plan-sort-partition'='true')` calls `newScan()` on the unpinned
base handle and reads S+1. It can also inherit a catalog/table-default selector
because `applyOptions` clears selectors only when the request supplies one. A
behavioral query option can therefore mix snapshots inside one statement.
Please overlay selector-free options on the table stored in statement MVCC,
reserve a cleaned base handle for explicit relation-local selectors, and cover
a two-alias commit/default-selector case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java:
##########
@@ -725,6 +732,8 @@ private LogicalPlan getLogicalPlan(TableIf table,
UnboundRelation unboundRelatio
Utils.qualifiedNameWithBackquote(qualifiedTableName));
});
+ validateOptionsTarget(table, unboundRelation.getScanParams());
Review Comment:
[P2] Validate OPTIONS before every relation or command can discard it
This check runs only after a catalog-backed query relation is resolved. A
CTE returns `LogicalCTEConsumer` before reaching it, and a recursive CTE
similarly returns `LogicalWorkTableReference`, so `WITH c AS (...) SELECT *
FROM c@options('scan.snapshot-id'='1')` silently ignores the request. The
shared `baseTableRef` grammar also feeds commands such as SHOW REPLICA into
`TableRefInfo`; those consumers never read `scanParams`, so they silently
ignore OPTIONS too. Please reject OPTIONS at each early return and non-query
consumer, or move capability validation to a boundary shared by every
syntactically accepted `baseTableRef`, with CTE and representative command
negative tests.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java:
##########
@@ -0,0 +1,113 @@
+// 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.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> SNAPSHOT_SELECTOR_KEYS = ImmutableSet.of(
+ CoreOptions.SCAN_SNAPSHOT_ID.key(),
+ CoreOptions.SCAN_TAG_NAME.key(),
+ CoreOptions.SCAN_TIMESTAMP.key(),
+ CoreOptions.SCAN_TIMESTAMP_MILLIS.key(),
+ CoreOptions.SCAN_WATERMARK.key(),
+ CoreOptions.SCAN_VERSION.key());
+
+ 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) {
+ 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 selectorCount =
options.keySet().stream().filter(SNAPSHOT_SELECTOR_KEYS::contains).count();
+ if (selectorCount > 1) {
+ throw new IllegalArgumentException(
+ "Only one Paimon snapshot selector can be specified: " +
SNAPSHOT_SELECTOR_KEYS);
+ }
+ }
+
+ public static Table applyOptions(Table table, Map<String, String> options)
{
+ validateOptions(options);
+ Map<String, String> isolatedOptions = new HashMap<>(options);
+ if
(options.keySet().stream().anyMatch(SNAPSHOT_SELECTOR_KEYS::contains)) {
Review Comment:
[P2] Isolate scan mode and position in both directions
This removes competing selector keys, but it leaves a table/catalog-default
`scan.mode`. With default `scan.mode=latest`,
`@options('scan.snapshot-id'='1')` therefore fails Paimon validation under
`LATEST`. The reverse is broken too: if the table carries `scan.snapshot-id=7`,
local `@options('scan.mode'='latest')` has no selector key, so cleanup never
runs and the retained snapshot is rejected under `LATEST`. Please isolate mode
and startup position as one compatibility-aware state: clear incompatible
inherited counterparts, while still rejecting explicitly conflicting
query-local pairs, and add regressions for both directions.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java:
##########
@@ -106,6 +106,22 @@ public Table getPaimonTable(Optional<MvccSnapshot>
snapshot) {
}
}
+ public Table getPaimonTable(TableScanParams scanParams) {
+ if (scanParams != null && scanParams.isOptions()) {
+ // Statement MVCC is keyed by table, while OPTIONS belongs to one
relation. Start
+ // from the catalog handle so its schema cannot inherit another
relation's selector.
+ return PaimonScanParams.applyOptions(getBasePaimonTable(),
scanParams.getMapParams());
+ }
+ return getPaimonTable(MvccUtil.getSnapshotFromContext(this));
+ }
+
+ public List<Column> getFullSchema(TableScanParams scanParams) {
+ Table table = getPaimonTable(scanParams);
+ return PaimonUtil.parseSchema(table,
Review Comment:
[P1] Preserve nested Paimon field IDs in the OPTIONS schema
`parseSchema` assigns `field.id()` only to each top-level `Column`; all
STRUCT, MAP, and ARRAY descendants created by `Column.createChildrenColumn`
keep the default ID `-1`. The normal Paimon schema path calls
`updatePaimonColumnUniqueId` recursively. Here `ExternalUtil` serializes the
missing nested IDs, `AccessPathParser` substitutes zero-based child ordinals,
and the native reader treats those fake ordinals as real IDs under
`BY_FIELD_ID`. A nested child renamed, reordered, or dropped and re-added is
therefore materialized as missing/default, or can match the wrong sibling on an
ID collision. Please build this schema through the schema cache or recursively
populate nested IDs and TIMESTAMPTZ metadata, then cover OPTIONS reads across
nested evolution.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java:
##########
@@ -0,0 +1,113 @@
+// 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.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."))
Review Comment:
[P1] Do not accept fallback-branch through plain Table.copy
This prefix allowlist admits Paimon 1.3.1's `scan.fallback-branch`, but
`applyOptions` implements every accepted key with `table.copy(...)`. Paimon
creates `FallbackReadFileStoreTable` only in `FileStoreTableFactory.create`,
where it resolves the named branch. Copying a plain table merely stores the
option and remains a plain append-only or primary-key table; copying an
existing fallback wrapper keeps its already-resolved fallback object even if
the query names another branch. Thus
`@options('scan.fallback-branch'='archive')` can succeed while silently
omitting partitions that exist only in `archive`, or can read a previously
configured fallback branch. Please route this option through factory/catalog
resolution, or reject it, and add a two-branch result test that switches the
requested fallback.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -924,8 +951,17 @@ private Table getProcessedTable() throws UserException {
}
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());
Review Comment:
[P2] Clear all inherited startup state for system-table INCR
`getIncrReadParams()` removes only `scan.snapshot-id` and `scan.mode`, so
the newly supported wrappers retain other positions and range controls. With
default `scan.tag-name=tag1`, `$audit_log@incr(1,2)` is rejected under
incremental mode. An inherited watermark is silently worse: Paimon time-travels
the wrapped table to the watermark-era schema before validation, does not list
watermark as an INCREMENTAL conflict, then scans `(1,2)` through that old
schema, which can disagree with Doris's current bound columns after evolution.
Please clear every inherited startup position and alternate range control
through one isolation helper, and cover both a rejected tag and an
evolved-schema watermark case.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/TableScanParams.java:
##########
@@ -29,12 +30,14 @@ public class TableScanParams {
public static final String INCREMENTAL_READ = "incr";
public static final String BRANCH = "branch";
public static final String TAG = "tag";
+ public static final String OPTIONS = "options";
public static final String SNAPSHOT = "snapshot";
public static final String RESET = "reset";
private static final ImmutableSet<String> VALID_PARAM_TYPES =
ImmutableSet.of(
INCREMENTAL_READ,
BRANCH,
- TAG);
+ TAG,
+ OPTIONS);
Review Comment:
[P1] Preserve OPTIONS through CREATE VIEW's relation copy
Adding OPTIONS to global relation parameters also makes it valid in a view
query, but `LogicalPlanBuilderForCreateView` immediately calls
`UnboundRelation.withIndexInSql`, whose copy passes `null` for `scanParams`.
Creation-time analysis and `finalCols` therefore use the ordinary/latest table.
The SQL rewrite replaces only the table identifier, so the persisted definition
still contains `@options(...)`; later expansion parses it normally and binds
the selected snapshot schema. After evolution, `CREATE VIEW v AS SELECT * FROM
p@options('scan.snapshot-id'='1')` is declared from latest columns but expands
with historical slots, breaking the view contract. Please preserve `scanParams`
in every identity copy, or reject scan params in persisted views, and add an
evolved-schema CREATE/ALTER VIEW regression.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java:
##########
@@ -0,0 +1,113 @@
+// 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.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> SNAPSHOT_SELECTOR_KEYS = ImmutableSet.of(
Review Comment:
[P1] Isolate creation-time startup positions too
`QUERY_OPTION_KEYS` accepts `scan.creation-time-millis` and
`scan.file-creation-time-millis`, but this selector set omits both, so they
neither participate in explicit-conflict counting nor clear inherited
positions. For a table carrying default `scan.snapshot-id=7`,
`@options('scan.creation-time-millis'='T')` retains snapshot 7. Paimon 1.3.1
derives `scan.mode=from-snapshot`, selects snapshot 7, and does not validate
creation time as a conflict, so the query succeeds while silently ignoring T.
The file-creation variant fails conflict validation instead. Please include
every mutually exclusive startup-position key in one isolation/validation
model, clear inherited mode and positions when one is supplied, and add a
result test that distinguishes the default snapshot from the requested creation
time.
--
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]
