github-actions[bot] commented on code in PR #65984:
URL: https://github.com/apache/doris/pull/65984#discussion_r3649196510
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -924,8 +930,14 @@ 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());
}
+ if (theScanParams != null && theScanParams.isOptions()) {
+ // Apply per-query options to a copy so they cannot leak through
cached table handles.
+ return baseTable.copy(theScanParams.getMapParams());
Review Comment:
[P1] Bind the ordinary-table schema selected by OPTIONS
`PaimonExternalTable.loadSnapshot` treats an OPTIONS relation as the latest
snapshot, so analysis, tuple slots, and predicates are bound to the latest
schema before this copy runs. For `scan.snapshot-id`, Paimon then swaps the
copied table to that snapshot's historical schema: FE filters latest-only slots
out of its projection, while JNI still requests them and fails (`RequiredField
... not found`); native/C++ readers keep the same
latest-descriptor/historical-split mismatch. The existing schema thread covers
system-table wrappers; this is the separate ordinary-table MVCC path. Please
resolve schema-selecting OPTIONS before relation binding and add an
evolved-schema data-table regression.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -924,8 +930,14 @@ 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());
}
+ if (theScanParams != null && theScanParams.isOptions()) {
+ // Apply per-query options to a copy so they cannot leak through
cached table handles.
+ return baseTable.copy(theScanParams.getMapParams());
Review Comment:
[P1] Resolve alternate snapshot selectors before this copy
The ordinary-table handle reaching this branch is already pinned by
`PaimonLatestSnapshotProjectionLoader` with `scan.snapshot-id=<latest>`.
Replacing that same key works, but valid Paimon selectors such as
`scan.tag-name`, `scan.timestamp`, `scan.timestamp-millis`, or `scan.watermark`
are merged beside the hidden snapshot ID; Paimon 1.3.1 requires exactly one
selector and `newScan()` fails with its one-parameter error. Please resolve
OPTIONS while building the per-relation Paimon snapshot (or remove the mutually
exclusive cached selector before copying), and cover at least one non-ID
selector on a real MVCC-backed table.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -924,8 +930,14 @@ 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());
}
+ if (theScanParams != null && theScanParams.isOptions()) {
+ // Apply per-query options to a copy so they cannot leak through
cached table handles.
+ return baseTable.copy(theScanParams.getMapParams());
Review Comment:
[P2] Validate option names before relying on Table.copy
Paimon 1.3.1 is not the key-validation boundary assumed here:
`AbstractFileStoreTable.copy` merges arbitrary keys, `checkAlterTableOption`
only rejects immutable/bucket changes, and `SchemaValidation` explicitly does
not validate every option name. Consequently `@options('scan.snapsh0t-id'='1')`
succeeds, the typo is never consumed, and Doris returns the latest/default rows
instead of the requested historical data. Please validate the supported
query-option key space (including any intentional extension namespaces) and add
a negative typo case.
##########
regression-test/suites/external_table_p0/paimon/paimon_incr_read.groovy:
##########
@@ -43,6 +43,55 @@ suite("test_paimon_incr_read", "p0,external") {
def test_incr_read = { String force ->
sql """ set force_jni_scanner=${force} """
+
+ // Query-level OPTIONS params apply to ordinary data tables for
both native and JNI readers.
+ List<List<Object>> latestRows = sql """
+ select id, name, age from paimon_incr order by id
+ """
+ List<List<Object>> snapshot1Rows = sql """
+ select id, name, age from paimon_incr
+ @options('scan.snapshot-id'='1')
+ order by id
+ """
+ List<List<Object>> snapshot2Rows = sql """
+ select id, name, age from paimon_incr
+ @options('scan.snapshot-id'='2')
+ order by id
+ """
+ List<List<Object>> latestSnapshotRows = sql """
+ select id, name, age from paimon_incr
+ @options('scan.snapshot-id'='3')
+ order by id
+ """
+ assertEquals(1, snapshot1Rows.size())
+ assertEquals(2, snapshot2Rows.size())
+ assertEquals(3, latestRows.size())
+ assertEquals(latestRows, latestSnapshotRows)
+
+ // A dynamic option must not leak into the cached table used by
the next query.
+ assertEquals(latestRows, sql("select id, name, age from
paimon_incr order by id"))
+
+ List<List<Object>> relationScopedRows = sql """
+ select left_orders.id, right_orders.id
+ from paimon_incr
+ @options('scan.snapshot-id'='1') left_orders
+ join paimon_incr
+ @options('scan.snapshot-id'='2') right_orders
+ on left_orders.id = right_orders.id
+ order by left_orders.id
+ """
+ assertEquals(1, relationScopedRows.size())
Review Comment:
[P2] Make the right-hand snapshot affect this oracle
Snapshot 1 contains only ID 1, while snapshot 2 and the latest snapshot also
contain ID 1. Because the left input is fixed at snapshot 1 and this is an
inner equality join, the result is still exactly `(1,1)` if the right-hand
option is lost, replaced with snapshot 1, or executed at latest, so all three
assertions pass without proving relation-scoped execution. Please add a result
whose value depends on ID 2 being present only on the right (for example, drive
an anti/outer check from the snapshot-2 relation), and record it with a
generated `.out`, so dropping the second option fails the regression.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/TableScanParams.java:
##########
@@ -29,12 +29,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:
[P2] Reject OPTIONS on unsupported table types
Adding `OPTIONS` to this global set makes the generic relation grammar
accept it for every catalog, but only `PaimonScanNode` consumes `isOptions()`.
For example, a Hive relation is carried through `LogicalFileScan` and
`PhysicalPlanTranslator`, then `HiveScanNode` never reads the field, so `SELECT
* FROM hive_catalog.db.t@options('scan.snapshot-id'='1')` succeeds against the
normal table and silently discards the request; several other file-scan types
behave the same way. Please validate OPTIONS against the bound table type (and
add representative negative tests) so unsupported catalogs fail instead of
returning misleading data.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -924,8 +930,14 @@ 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());
}
+ if (theScanParams != null && theScanParams.isOptions()) {
+ // Apply per-query options to a copy so they cannot leak through
cached table handles.
+ return baseTable.copy(theScanParams.getMapParams());
Review Comment:
[P2] Reject OPTIONS forms that this branch cannot consume
The generic scan-parameter grammar also accepts `@options()` and positional
identifier forms such as `@options(foo, bar)`. Those arguments are stored in
`listParams`, while this branch forwards only the empty `mapParams`, so the
query succeeds as a latest/default scan instead of reporting malformed syntax.
Please require OPTIONS to be a non-empty key/value map and add negative
parser/analyzer coverage for both empty and positional forms.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -912,8 +918,8 @@ private Table getProcessedTable() throws UserException {
Table baseTable = source.getPaimonTable();
TableScanParams theScanParams = getScanParams();
if (source.getExternalTable() instanceof PaimonSysExternalTable) {
Review Comment:
[P1] Keep row_tracking incremental reads on the Paimon reader
This broadened acceptance also enables `t$row_tracking@incr(...)`. For an
append-only row-tracking table, its `DataSplit` is raw-convertible, and
`shouldForceJniForSystemTable()` forces only audit/binlog, so
`force_jni_scanner=false` sends it through `PAIMON_NATIVE`. Paimon's
row-tracking reader fills lazily stored `_ROW_ID` and `_SEQUENCE_NUMBER` from
`DataFileMeta.firstRowId/maxSequenceNumber`, but Doris's raw-file split carries
neither value and has no equivalent row-tracking special-field mapping.
First-write rows can therefore return missing/wrong hidden values. Please force
this system table to JNI (or carry and materialize the metadata) and add a
native-mode first-snapshot regression.
##########
regression-test/suites/external_table_p0/paimon/paimon_system_table.groovy:
##########
@@ -151,7 +151,56 @@ suite("paimon_system_table", "p0,external") {
desc ${tableName}\$snapshots
"""
- // 2.6 system table does not support time travel
+ // 2.6 system table supports dynamic scan options but not time travel
+ // Quote Paimon's partition column because PARTITION is reserved in
Doris SQL.
+ List<List<Object>> filesAtSnapshotResult = sql """
+ SELECT `partition`,
+ bucket,
+ file_path,
+ file_format,
+ schema_id,
+ level,
+ record_count,
+ file_size_in_bytes,
+ min_sequence_number,
+ max_sequence_number,
+ creation_time
+ FROM ${tableName}\$files
+ @options('scan.snapshot-id'='${direct_query_snapshot_id}')
+ """
+ assertTrue(filesAtSnapshotResult.size() > 0,
+ "Files system table should return data for snapshot
${direct_query_snapshot_id}")
+
+ String multiSnapshotTable = "test_paimon_incr_read_db.paimon_incr"
+ List<List<Object>> snapshotRows = sql """
+ select snapshot_id from ${multiSnapshotTable}\$snapshots order
by snapshot_id
+ """
+ assertTrue(snapshotRows.size() > 1, "The regression table should
contain multiple snapshots")
+ String firstSnapshotId = String.valueOf(snapshotRows.first()[0])
+ String latestSnapshotId = String.valueOf(snapshotRows.last()[0])
+ List<List<Object>> filesWithoutOptions = sql """
+ select file_path from ${multiSnapshotTable}\$files order by
file_path
+ """
+ List<List<Object>> filesAtFirstSnapshot = sql """
+ select file_path from ${multiSnapshotTable}\$files
+ @options('scan.snapshot-id'='${firstSnapshotId}')
+ order by file_path
+ """
+ List<List<Object>> filesAtLatestSnapshot = sql """
+ select file_path from ${multiSnapshotTable}\$files
+ @options('scan.snapshot-id'='${latestSnapshotId}')
+ order by file_path
+ """
+ assertTrue(filesAtFirstSnapshot.size() < filesWithoutOptions.size(),
Review Comment:
[P2] Assert the requested historical snapshot exactly
The fixture has one, two, and three files at snapshots 1, 2, and 3, but this
condition accepts either of the first two: resolving the requested first
snapshot as snapshot 2 still satisfies `2 < 3`. The later assertion verifies
only the latest selector, so this regression can pass while the historical
option selects the wrong snapshot. Please assert the known count/file set for
snapshot 1 and record the deterministic result through the regression `.out`
path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
