github-actions[bot] commented on code in PR #67904:
URL: https://github.com/apache/doris/pull/67904#discussion_r4004218095
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadata.java:
##########
@@ -483,33 +488,65 @@ public ConnectorTableSchema getTableSchema(
return getTableSchema(session, handle);
}
Table table = loadTable(session, iceHandle);
- Schema schema;
- if (table.currentSnapshot() == null) {
- // Empty table: legacy getSchema falls back to the latest schema
(NEWEST_SCHEMA_ID path).
- schema = table.schema();
- } else {
- schema = table.schemas().get((int) snapshot.getSchemaId());
- if (schema == null) {
- // Defensive: a pinned id absent from table.schemas() (legacy
would NPE) -> latest.
- // INVARIANT: this SLOT-schema fallback MUST stay identical to
the DICT-schema fallback in
- // IcebergScanPlanProvider.pinnedSchema (same getSchemaId()
lookup + same silent -> table.schema()).
- // If the two diverge, the field-id dict names and the BE
scan-slot names resolve DIFFERENT
- // schemas -> BE children.at() std::out_of_range-SIGABRT on a
schema-evolved time-travel read
- // (reverify #65185 L16). Do not harden ONE side to throw
without the other.
- schema = table.schema();
+ validateSnapshotTable(iceHandle, table, snapshot);
+ Schema schema = resolvePinnedSchema(table, snapshot);
+ String specId =
snapshot.getProperties().get(PARTITION_SPEC_ID_PROPERTY);
+ PartitionSpec spec = specId == null ? table.spec() :
table.specs().get(Integer.parseInt(specId));
+ if (spec == null) {
+ // Keep the legacy missing-history fallback after checking the
table identity.
+ spec = table.spec();
+ }
+ return buildTableSchema(iceHandle.getTableName(), table, schema, spec,
true);
+ }
+
+ private void validateSnapshotTable(IcebergTableHandle handle, Table table,
ConnectorMvccSnapshot snapshot) {
+ String identity =
snapshot.getProperties().get(TABLE_IDENTITY_PROPERTY);
+ if (identity != null && !identity.equals(tableIdentity(table))) {
+ // Numeric schema/spec IDs can be reused after recreation. Reject
the entire old pin;
+ // replacing only its schema or spec would still mix the new table
with an old data fence.
+ if (latestSnapshotCache != null) {
+
latestSnapshotCache.invalidate(TableIdentifier.of(handle.getDbName(),
handle.getTableName()));
}
+ throw new DorisConnectorException("Iceberg table " +
handle.getDbName() + "." + handle.getTableName()
+ + " identity changed after its snapshot was cached; retry
the statement");
}
- return buildTableSchema(iceHandle.getTableName(), table, schema, true);
+ }
+
+ private static String tableIdentity(Table table) {
+ if (table instanceof HasTableOperations) {
+ TableMetadata metadata = ((HasTableOperations)
table).operations().current();
+ if (metadata.uuid() != null) {
+ return metadata.uuid();
+ }
+ // Legacy V1 metadata may lack a UUID. Only the exact metadata
file can safely reuse its IDs.
+ return "metadata:" +
Objects.requireNonNull(metadata.metadataFileLocation(),
Review Comment:
[P1] Do not equate a UUID-less table's identity with its current metadata
file. An ordinary Iceberg commit writes a new metadata file for the same table.
With REST vended credentials, `latestSnapshotCache` stays enabled while
`tableCache` is disabled, so the next statement can receive cached M1
coordinates, freshly load the same table at M2, and fail this identity check;
active V1 tables then require a retry after every commit.
`IcebergWriteSchemaContext` already handles this case by accepting a retained
metadata ancestor. Please disable latest-pin caching for UUID-less tables or
validate ancestry, and test a normal same-table commit under the
warm-pin/fresh-table cache combination while retaining recreate protection.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnectorMetadata.java:
##########
@@ -577,7 +581,38 @@ public Optional<ConnectorMvccSnapshot> beginQuerySnapshot(
Identifier identifier =
Identifier.create(paimonHandle.getDatabaseName(), paimonHandle.getTableName());
long id = latestSnapshotCache.getOrLoad(identifier,
() ->
catalogOps.latestSnapshotId(resolveTable(paimonHandle)).orElse(-1L));
- return
Optional.of(ConnectorMvccSnapshot.builder().snapshotId(id).build());
+ return Optional.of(ConnectorMvccSnapshot.builder().snapshotId(id)
+ .schemaId(statementSchemaId(paimonHandle,
resolveTable(paimonHandle))).build());
+ }
+
+ private <T> T readSchemaAuthenticated(Supplier<T> read) {
+ // Cached tables do not cache schema files: exact/latest schema reads
still need plugin UGI and TCCL.
+ try {
+ return context.executeAuthenticated(read::get);
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException("Failed to read Paimon schema", e);
+ }
+ }
+
+ private long statementSchemaId(PaimonTableHandle handle, Table table) {
+ return statementSchemas.computeIfAbsent(handle,
Review Comment:
[P1] Fence the physical Paimon generation across one statement.
`statementSchemas` is keyed by `PaimonTableHandle` equality, which omits the
transient `Table`/generation. `materializeLatest` can capture generation A, but
`PluginDrivenScanNode.create` later resolves the handle again; if an external
DROP/CREATE occurs between them and B reuses the snapshot/schema IDs,
`schemaForPin` returns A while `withScanOptions` preserves B and restoration
skips because the IDs match. The FE guard therefore passes on A/A while
native/JNI planning reads B's rows. This is distinct from the existing
cross-statement memo thread because it occurs after the current statement
pinned A. Please freeze or validate the physical generation through scan
resolution, and cover recreation between pinning and scan-handle resolution in
one metadata instance.
--
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]
