github-actions[bot] commented on code in PR #68259:
URL: https://github.com/apache/doris/pull/68259#discussion_r4068366747
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -2418,7 +2426,10 @@ private Split createIcebergSplit(FileScanTask
fileScanTask) throws UserException
}
split.setTableFormatType(TableFormatType.ICEBERG);
split.setTargetSplitSize(selectFeSplitSize(fileScanTask,
targetSplitSize));
- if (isPartitionedTable) {
+ // Gate on the table's spec HISTORY, not on the current default spec:
a file written under an older
Review Comment:
[P2] Resolve identity names against the selected schema
Explicit snapshot/tag reads intentionally use `getQuerySchema()` and project
the historical schema, but this path still asks
`getIdentityPartitionInfoMap(..., icebergTable, ...)` to resolve the old source
ID through `icebergTable.schema()`, i.e. the current schema. If `p` was removed
after the selected snapshot (after first removing its partition field),
`findColumnName(sourceId)` returns null and the manifest identity value is
discarded before either scanner sees it. That leaves metadata-only historical
files wrong even after the scanner-side issue is fixed. Please resolve identity
keys/names against the selected scan schema and add a snapshot/tag case where
the old identity column is absent from the current schema.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -2418,7 +2426,10 @@ private Split createIcebergSplit(FileScanTask
fileScanTask) throws UserException
}
split.setTableFormatType(TableFormatType.ICEBERG);
split.setTargetSplitSize(selectFeSplitSize(fileScanTask,
targetSplitSize));
- if (isPartitionedTable) {
+ // Gate on the table's spec HISTORY, not on the current default spec:
a file written under an older
+ // identity spec keeps its partition values in the manifest, and they
are the only source for an
+ // identity partition column that the physical file does not store.
+ if (hasPartitionedSpec) {
Review Comment:
[P1] Materialize the historical identity value
For `identity(p) -> unpartitioned`, the current spec makes
`getCommonIdentityPartitionColumns()` empty, so FE still marks `p` as a
regular/file slot. With scanner v1, `orderedPartitionMetadataKeys` is also
empty and this split value is never sent. With scanner v2 it is sent, but
`FileScannerV2` only sets `ColumnDefinition.is_partition_key` for non-file
slots, and `TableColumnMapper` consumes a partition constant only when that
flag is true; otherwise it reads the physical field. Consequently the
`add_files`/metadata-only case described by this PR still returns a missing
value (or a required-field error) in both modes. The new test stops at
`IcebergSplit`, so it cannot catch that downstream failure. Please make a
present per-split identity value authoritative while retaining physical-file
fallback for ranges without one, and cover the actual scanner output in both
modes.
--
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]
