github-actions[bot] commented on code in PR #66297:
URL: https://github.com/apache/doris/pull/66297#discussion_r3683652240
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/StatementContext.java:
##########
@@ -940,7 +935,8 @@ public void addPlannerHook(PlannerHook plannerHook) {
public void loadSnapshots(TableIf specificTable, Optional<TableSnapshot>
tableSnapshot,
Optional<TableScanParams> scanParams) {
if (specificTable instanceof MvccTable) {
- MvccTableInfo mvccTableInfo = new MvccTableInfo(specificTable);
+ MvccTableInfo mvccTableInfo = new MvccTableInfo(specificTable,
+ versionKeyOf(tableSnapshot, scanParams));
if (!snapshots.containsKey(mvccTableInfo)) {
Review Comment:
[P1] Share the resolved startup state when aliases share this snapshot key.
Each textual alias owns a separate `TableScanParams`: the first identical
`OPTIONS` alias resolves and pins `scan.file-creation-time-millis` to snapshot
S while loading this entry, but the second hits `containsKey` and skips
`loadSnapshot`, leaving its own `resolvedMapParams` empty. If a commit lands
before that alias reaches `getPaimonTable`, it resolves from the live base
table and pins S+1, so one statement can plan the two identical aliases from
different snapshots. Please store the canonical resolved options with the
statement snapshot and seed every matching params instance (or otherwise force
scan planning to consume the pinned resolution), and test two identical
latest-dependent aliases while latest advances.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java:
##########
@@ -139,9 +140,12 @@ public Table getPaimonTable(NameMapping nameMapping,
String branch, String query
}
return executionAuthenticator.execute(() -> {
Table table = catalog.getTable(identifier);
- Map<String, String> tableOptions =
-
paimonProperties.getTableOptionsForCopy(table.options());
- return tableOptions.isEmpty() ? table :
table.copy(tableOptions);
+ Map<String, String> tableOptions =
paimonProperties.getTableOptionsForCopy();
+ // Relation options are applied after this cached handle is
returned. Defer final
+ // validation so a safe relation value can override an unsafe
physical value.
+ Map<String, String> runtimeOptions =
+ PaimonReaderOptions.runtimeSafeCopyOptions(table,
tableOptions);
Review Comment:
[P1] Defer physical manifest validation until relation options are
available. This method runs while building the neutral table-cache entry,
before any `TableScanParams` exists. With no catalog override,
`runtimeSafeCopyOptions` reads a physical `scan.manifest.parallelism=0` and
throws, so the base handle never reaches `PaimonExternalTable.loadSnapshot` /
`PaimonScanParams.applyOptions`; `@options('scan.manifest.parallelism'='1')`
still fails for unpartitioned, partitioned, and system-table relations. This is
distinct from the existing pre-lock partition-loader thread because it fails
earlier during base cache loading. Please keep this cached handle neutral and
normalize/validate after the final relation > catalog > physical copy, with a
real catalog/cache-path test rather than a mocked base handle.
##########
fe/be-java-extensions/paimon-connector/src/main/java/org/apache/doris/paimon/PaimonJniScanner.java:
##########
@@ -571,18 +665,27 @@ static Optional<Long> parseDataSizeBytes(String value) {
private void initTable() {
Preconditions.checkState(params.containsKey("serialized_table"));
table = PaimonUtils.deserialize(params.get("serialized_table"));
+ table = applyDefaultReadBatchSize(table, batchSize);
+ paimonAllFieldNames = PaimonUtils.getFieldNames(this.table.rowType());
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("paimonAllFieldNames:{}", paimonAllFieldNames);
+ }
+ }
+
+ static Table applyDefaultReadBatchSize(Table table, int dorisBatchSize) {
+ if (table.options().containsKey(CoreOptions.READ_BATCH_SIZE.key())) {
Review Comment:
[P1] Keep the old-FE fallback when trusting a serialized batch size. During
a rolling upgrade, an old FE has none of this PR's `PaimonReaderOptions`
validation and can serialize a physical `read.batch-size=0` (the regression
fixture creates exactly that table). The previous scanner always replaced it
with Doris's positive `batchSize`, but a new BE now takes this branch and hands
`0` to Paimon's reader, which the new FE-side guard itself says can report
success without advancing. This affects both V1 and V2 because both instantiate
this Java scanner and is distinct from the existing thread about current-FE
values being overwritten. Please tag validated payloads and retain the fallback
for older senders, or independently validate/fail fast here; add old-FE/new-BE
coverage for a serialized zero-batch table.
--
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]
