hubgeter commented on code in PR #68259:
URL: https://github.com/apache/doris/pull/68259#discussion_r4069248369
##########
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:
Confirmed and fixed for scanner v2. The missing link was the classification,
not the split: `getCommonIdentityPartitionColumns()` intersects with the
current spec, so p was a REGULAR slot and the v2 column mapper skipped the
split constant (it requires `is_partition_key`). v2 now classifies the identity
columns of all specs, like master; files without a value for the column still
fall back to the physical field through the mapper's existing chain. Added a
regression case
(`test_iceberg_partition_evolution_identity_metadata_only.groovy`) that asserts
the scanner output for a metadata-only file and for a post-evolution file that
does store the column.
Not done for v1, deliberately: v1 has no per-file fallback — a partition key
it cannot fill from the split is not read from the file either — so widening
the list there would turn currently-correct reads into NULLs. v1 still returns
NULL for this case; fixing it needs BE changes and is out of scope here. Noted
in the PR description.
##########
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:
Acknowledged, but left out of this PR on purpose. Resolving identity names
through `icebergTable.schema()` is pre-existing behavior and master does
exactly the same after #68124, so this is not a regression introduced by the
pick; it only bites a time-travel read whose identity column was dropped from
the current schema. Folding a schema-resolution change into a backport would
widen the blast radius of a wrong-result fix on a maintenance branch. Worth a
separate issue against master first, so both branches stay aligned.
--
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]
