github-actions[bot] commented on code in PR #65984:
URL: https://github.com/apache/doris/pull/65984#discussion_r3642875194
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -912,8 +912,8 @@ private Table getProcessedTable() throws UserException {
Table baseTable = source.getPaimonTable();
TableScanParams theScanParams = getScanParams();
if (source.getExternalTable() instanceof PaimonSysExternalTable) {
Review Comment:
[P1] Keep incremental audit/binlog scans on JNI
The newly accepted `audit_log` and `binlog` ranges still reach
`setPaimonParams()` as `DataSplit`s. When `enable_paimon_cpp_reader=true`, that
method selects `PAIMON_CPP` without consulting
`shouldForceJniForSystemTable()`. The C++ path only receives the base table
path/split and builds a base `TableRead`; it has no audit/binlog wrapper to
materialize `rowkind` or binlog's before/after arrays. These queries can
therefore fail or return the wrong shape under the supported session option.
Please exclude these system-table types from the C++ branch (and cover the
enabled option) until it implements those transformations.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -912,8 +912,8 @@ private Table getProcessedTable() throws UserException {
Table baseTable = source.getPaimonTable();
TableScanParams theScanParams = getScanParams();
if (source.getExternalTable() instanceof PaimonSysExternalTable) {
- if (theScanParams != null) {
- throw new UserException("Paimon system tables do not support
scan params.");
+ if (theScanParams != null && !theScanParams.incrementalRead()) {
Review Comment:
[P1] Gate `@incr` by the system table's capability
This condition accepts every Paimon system-table type, but several 1.3.1
readers do not consume `incremental-between`. For example, `SnapshotsRead`
enumerates snapshot history using only pushed `snapshot_id` predicates, and
`ManifestsTable` reads latest/explicit time-travel state, so
`$snapshots@incr(1,2)` silently returns the same unbounded metadata as a normal
scan. Reusing the processed table on BE does not fix those types. Please reject
incremental params for option-insensitive system tables (or implement explicit
range semantics) and keep the positive path on a type such as `audit_log`.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -924,6 +924,8 @@ 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.
Review Comment:
[P1] Send the processed table to the JNI reader
This copy is only used by `getPaimonSplitFromAPI()`; `doInitialize()` has
already serialized `source.getPaimonTable()` for the BE. In Paimon 1.3.1,
metadata tables such as `files` and `partitions` emit placeholder splits and
their BE-side readers call the underlying table's `newScan()` again. That
second scan therefore uses the cached table's latest/default options, so (for
example) `$files@incr(1,2)` can return files from the full latest snapshot
instead of only the `(1,2]` changes. Please serialize/reuse the same processed
per-query table for execution as well as FE planning.
##########
regression-test/suites/external_table_p0/paimon/paimon_system_table.groovy:
##########
@@ -151,7 +151,12 @@ 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
+ List<List<Object>> incrementalSystemTableResult = sql """
+ select * from
${tableName}\$snapshots@incr('startSnapshotId'=1, 'endSnapshotId'=2)
+ """
+ assertNotNull(incrementalSystemTableResult, "System table incremental
query result should not be null")
Review Comment:
[P2] Make this assert the incremental result
This only proves that the query did not throw: `sql` returns a non-null list
for empty, full, and correctly ranged results. It also uses Paimon's
`snapshots` table, whose 1.3.1 reader enumerates snapshot history and does not
consume `incremental-between`, so this test still passes if the new options are
lost before execution. Please exercise an incremental-capable system table such
as `audit_log` with multiple known snapshots and check an ordered exact result
(and generated `.out`) that excludes at least one out-of-range change.
--
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]
