github-actions[bot] commented on code in PR #66247:
URL: https://github.com/apache/doris/pull/66247#discussion_r3679531506
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -995,18 +996,27 @@ private Table getProcessedTable() throws UserException {
throw new UserException("Can not specify scan params and table
snapshot at same time.");
}
+ Table finalTable;
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()) {
+ finalTable = baseTable.copy(getIncrReadParams());
+ } else if (theScanParams != null && theScanParams.isOptions()) {
try {
- return source.getPaimonTable(theScanParams);
+ finalTable = source.getPaimonTable(theScanParams);
} catch (IllegalArgumentException e) {
throw new UserException(e.getMessage(), e);
}
+ } else {
+ finalTable = baseTable;
+ }
+ try {
+ // This is the last common boundary before planning and
serialization, including scans
+ // with no relation copy and incremental/system-table paths that
bypass applyOptions.
Review Comment:
[P1] Do not scan raw physical partitions during snapshot binding
Before this method runs, statement snapshot loading follows
`PaimonLatestSnapshotProjectionLoader` -> `PaimonPartitionInfoLoader` ->
`PaimonExternalCatalog.getPaimonPartitions`, which calls raw
`catalog.listPartitions`. Paimon 1.3.1 implements that by reloading the
physical table and running `newScan().listPartitionEntries()`. A partitioned
table with physical `scan.manifest.parallelism=0` can therefore block before
this final guard rejects it, and a safe relation override cannot take effect
because snapshot loading runs first; an oversized value can mutate the global
pool. The new fixture is unpartitioned, so the loader returns `EMPTY` and
misses this path. Please validate before enumerating the physical table (or
skip this preload when relation options require it) and add a partitioned
fixture. This is distinct pre-ScanNode metadata planning.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -995,18 +996,27 @@ private Table getProcessedTable() throws UserException {
throw new UserException("Can not specify scan params and table
snapshot at same time.");
}
+ Table finalTable;
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()) {
+ finalTable = baseTable.copy(getIncrReadParams());
+ } else if (theScanParams != null && theScanParams.isOptions()) {
try {
- return source.getPaimonTable(theScanParams);
+ finalTable = source.getPaimonTable(theScanParams);
} catch (IllegalArgumentException e) {
throw new UserException(e.getMessage(), e);
}
+ } else {
+ finalTable = baseTable;
+ }
+ try {
+ // This is the last common boundary before planning and
serialization, including scans
Review Comment:
[P1] Guard row-count plans before this boundary
This is not the last Paimon planning boundary.
`PaimonExternalTable.fetchRowCount()` directly runs
`getBasePaimonTable().newReadBuilder().newScan().plan()`, and is reached by
`ExternalRowCountCache` (synchronously by default) and directly by
`AnalysisManager`. With physical `scan.manifest.parallelism=0`,
optimization/statistics can therefore block before
`getProcessedTable()`—including a query whose relation override would make the
final scan safe; an above-CPU value can replace the JVM-static pool first.
Please validate the effective base handle at each row-count plan and cover
cache/manual-analysis paths. This is distinct from the existing foreground-scan
thread.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonScanParams.java:
##########
@@ -140,7 +144,11 @@ public static Table applyOptions(Table table, Map<String,
String> options) {
.filter(key -> !tableOptions.containsKey(key))
.forEach(key -> isolatedOptions.put(key, null));
}
- return table.copy(isolatedOptions);
+ Table effectiveTable = table.copy(isolatedOptions);
+ // Validate after every copy so relation options participate in the
documented
+ // relation > catalog > physical precedence before the effective value
is judged.
+
PaimonReaderOptions.validateEffectiveTableOptions(effectiveTable.options());
Review Comment:
[P1] Validate the table hidden by read-only wrappers
On a real `$partitions` table, `effectiveTable.options()` is always empty:
Paimon 1.3.1's `ReadonlyTable` supplies `{}`, while `PartitionsTable.copy(...)`
only updates its private `FileStoreTable`. `PartitionsRead.createReader` later
calls that hidden table's `newScan().listPartitionEntries()`. Thus a physical
`scan.manifest.parallelism=0` passes this check and the ScanNode check, then
can block on Paimon's zero-permit executor. The new tests use generic `Table`
mocks that expose the copied map, so they miss this contract. Please validate
the underlying effective data table and add real `$partitions` no-override and
safe-override coverage. This is the remaining wrapper case after discussion
`3679236492`, not the foreground data-table path.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -995,18 +996,27 @@ private Table getProcessedTable() throws UserException {
throw new UserException("Can not specify scan params and table
snapshot at same time.");
}
+ Table finalTable;
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()) {
+ finalTable = baseTable.copy(getIncrReadParams());
+ } else if (theScanParams != null && theScanParams.isOptions()) {
try {
- return source.getPaimonTable(theScanParams);
+ finalTable = source.getPaimonTable(theScanParams);
} catch (IllegalArgumentException e) {
throw new UserException(e.getMessage(), e);
}
+ } else {
+ finalTable = baseTable;
+ }
+ try {
+ // This is the last common boundary before planning and
serialization, including scans
+ // with no relation copy and incremental/system-table paths that
bypass applyOptions.
+
PaimonReaderOptions.validateEffectiveTableOptions(finalTable.options());
Review Comment:
[P1] Validate fallback branch options too
`FallbackReadFileStoreTable.options()` exposes only its wrapped main branch,
but `newScan()` creates both main and fallback scans and
`FallbackReadScan.plan()` calls `fallbackScan.listPartitions()` before planning
remaining fallback data. If the main branch is safe but the physical fallback
branch has `scan.manifest.parallelism=0`, this line validates only the safe
main map and planning can block; an above-CPU value can replace the shared
executor. Please validate every option-bearing child handle after applicable
copies and add a real fallback-branch fixture. This is distinct from the normal
foreground table because the hidden child has independent schema options.
--
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]
