github-actions[bot] commented on code in PR #66007:
URL: https://github.com/apache/doris/pull/66007#discussion_r3649634255
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/FileQueryScanNode.java:
##########
@@ -718,6 +732,29 @@ public TableScanParams getScanParams() {
return this.scanParams;
}
+ /**
+ * Return metadata pinned for this scan relation.
+ */
+ protected Optional<MvccSnapshot> getRelationSnapshot() {
+ if (relationSnapshotInitialized) {
+ return relationSnapshot;
+ }
+ relationSnapshotInitialized = true;
+ TableIf targetTable = desc.getTable();
+ if (!(targetTable instanceof MvccTable)) {
+ return Optional.empty();
+ }
+ if (tableSnapshot != null || scanParams != null) {
Review Comment:
[P1] Reuse the snapshot pinned while binding this relation
For a branch/ref, this calls `loadSnapshot` again during physical scan
initialization. The logical output and tuple slots were built from branch head
S0, but if the branch advances before this call, schema params and the
Paimon/Iceberg scan source come from S1. A rename can fail column mapping;
without a rename the query can read S1 under a plan analyzed at S0, contrary to
the `MvccTable` whole-query snapshot contract. Please carry the bound
`MvccSnapshot` through logical-to-physical translation instead of re-resolving
raw qualifiers, and test successive mocked loads returning different heads.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalTable.java:
##########
@@ -178,6 +178,10 @@ public List<Column> getFullSchema() {
return schemaCacheValue.map(SchemaCacheValue::getSchema).orElse(null);
}
+ public List<Column> getFullSchema(Optional<MvccSnapshot> snapshot) {
Review Comment:
[P1] Honor the relation snapshot for HMS Iceberg schemas
`HMSExternalTable` implements `MvccTable` and returns an
`IcebergMvccSnapshot`, but it inherits this overload, which discards that
snapshot and calls the context-sensitive no-argument accessor.
`LogicalFileScan.captureRelationSchema` also handles only the direct
Iceberg/Paimon table classes. As a result, two HMS-Iceberg relations at
different versions still derive both schemas from the last table-wide entry;
across a rename, one side fails column mapping depending on relation order.
Please implement snapshot-aware HMS-Iceberg schema access and select
relation-local capture by MVCC capability, with a dual-version HMS test in both
orders.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -1467,7 +1464,9 @@ public static IcebergTableQueryInfo getQuerySpecSnapshot(
return new IcebergTableQueryInfo(
snapshotRef.snapshotId(),
refName,
- SnapshotUtil.schemaFor(table, refName).schemaId());
+ // Iceberg maps a branch name to the table's latest schema, so
resolve the branch
+ // head snapshot directly to keep historical branch columns
isolated.
+ SnapshotUtil.schemaFor(table,
snapshotRef.snapshotId()).schemaId());
Review Comment:
[P2] Plan historical Iceberg filters with the historical schema
This correctly selects the branch-head snapshot schema, but downstream
`IcebergScanNode` paths still convert conjuncts with `icebergTable.schema()`;
the manifest-cache path also constructs metrics/residual evaluators from that
current schema. A predicate on a renamed-away historical column therefore
cannot be pushed, and a drop/re-add of the same name binds the new field ID
instead of the queried historical ID, defeating field-ID-aware pruning. Please
use the pinned snapshot ID and its schema consistently for filter conversion,
evaluators, and the table scan, and add a rename or drop/re-add pruning test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java:
##########
@@ -813,6 +813,8 @@ private Plan
bindIcebergTableSink(MatchingContext<UnboundIcebergTableSink<Plan>>
}
}
+ // Iceberg branches share the table metadata schema, so writes use the
latest schema
Review Comment:
[P1] Bind branch-write targets from target-specific latest metadata
The comment states the required contract, but every target lookup below
(`getBaseSchema`, `getColumn`, and `getFullSchema`) is still backed by the
statement's table-scoped snapshot. For `IcebergSink t@branch(b)(id,new_name) ->
Project(id,old_name) -> Scan t FOR VERSION AS OF old`, binding the child
overwrites that entry with the old source schema, so the sink rejects valid
`new_name` or derives old target coercions. This affects both INSERT and
OVERWRITE SELECT. Please use the target schema already collected before child
binding (or another target-specific latest snapshot), and cover this plan shape.
##########
fe/fe-type/src/main/java/org/apache/doris/catalog/Type.java:
##########
@@ -482,18 +486,27 @@ public String hideVersionForVersionColumn(Boolean
isToSql) {
}
return typeStr.toString();
} else if (isArrayType()) {
- String nestedDesc = ((ArrayType)
this).getItemType().hideVersionForVersionColumn(isToSql);
+ String nestedDesc = ((ArrayType) this).getItemType()
+ .hideVersionForVersionColumn(isToSql, showNestedComment);
return "array<" + nestedDesc + ">";
} else if (isMapType()) {
- String keyDesc = ((MapType)
this).getKeyType().hideVersionForVersionColumn(isToSql);
- String valueDesc = ((MapType)
this).getValueType().hideVersionForVersionColumn(isToSql);
+ String keyDesc = ((MapType) this).getKeyType()
+ .hideVersionForVersionColumn(isToSql, showNestedComment);
+ String valueDesc = ((MapType) this).getValueType()
+ .hideVersionForVersionColumn(isToSql, showNestedComment);
return "map<" + keyDesc + "," + valueDesc + ">";
} else if (isStructType()) {
List<String> fieldDesc = new ArrayList<>();
StructType structType = (StructType) this;
for (int i = 0; i < structType.getFields().size(); i++) {
StructField field = structType.getFields().get(i);
- fieldDesc.add(field.getName() + ":" +
field.getType().hideVersionForVersionColumn(isToSql));
+ StringBuilder desc = new
StringBuilder(field.getName()).append(":")
Review Comment:
[P2] Preserve nested requiredness in DESCRIBE output
This new manual struct rendering adds comments but omits the ` not null`
suffix that `StructField.toSql` emits when `getContainsNull()` is false. The
same patch now maps Iceberg nested fields with `x.isOptional()`, so
`required_child` is preserved internally yet DESCRIBE renders it
indistinguishably from an optional child. The new tests construct only optional
nested fields. Please append ` not null` before the comment for required fields
and add adjacent required/optional coverage with nested-comment display both
enabled and disabled.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/StatementContext.java:
##########
@@ -986,7 +986,12 @@ public void loadSnapshots(TableIf specificTable,
Optional<TableSnapshot> tableSn
Optional<TableScanParams> scanParams) {
if (specificTable instanceof MvccTable) {
MvccTableInfo mvccTableInfo = new MvccTableInfo(specificTable);
- if (!snapshots.containsKey(mvccTableInfo)) {
+ if (tableSnapshot.isPresent() || scanParams.isPresent()) {
Review Comment:
[P1] Keep latest and historical snapshots relation-scoped
Overwriting the sole `MvccTableInfo` key still breaks a statement that mixes
an unqualified/latest relation with a historical relation. In latest-first
order, the historical relation replaces the map entry and the latest scan later
falls back to the old snapshot. In historical-first order, the unqualified
relation deliberately does not restore latest and captures the old schema
immediately. After schema evolution this can reject the latest columns or pair
them with historical data in both Iceberg and Paimon. Please store a concrete
snapshot per relation, including the unqualified latest relation, and add both
relation orders to the dual-relation tests.
--
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]
