yangshangqing95 commented on code in PR #17762:
URL: https://github.com/apache/iceberg/pull/17762#discussion_r3844662899
##########
core/src/main/java/org/apache/iceberg/MetadataLogEntriesTable.java:
##########
@@ -59,18 +68,46 @@ public Schema schema() {
private DataTask task(TableScan scan) {
TableMetadata current = table().operations().current();
+
List<TableMetadata.MetadataLogEntry> metadataLogEntries =
Lists.newArrayList(current.previousFiles().listIterator());
+
metadataLogEntries.add(
new TableMetadata.MetadataLogEntry(
current.lastUpdatedMillis(), current.metadataFileLocation()));
+
+ Schema projectedSchema = scan.schema();
+ boolean shouldLoadSnapshotDetails =
+ projectedSchema.findField(LATEST_SNAPSHOT_ID.fieldId()) != null
+ || projectedSchema.findField(LATEST_SCHEMA_ID.fieldId()) != null
+ || projectedSchema.findField(LATEST_SEQUENCE_NUMBER.fieldId()) !=
null;
+
return StaticDataTask.of(
table().io().newInputFile(current.metadataFileLocation()),
schema(),
- scan.schema(),
+ projectedSchema,
metadataLogEntries,
metadataLogEntry ->
- MetadataLogEntriesTable.metadataLogEntryToRow(metadataLogEntry,
table()));
+ metadataLogEntryToRow(metadataLogEntry, current,
shouldLoadSnapshotDetails));
+ }
+
+ private Snapshot latestSnapshotForEntry(
+ TableMetadata.MetadataLogEntry metadataLogEntry, TableMetadata current) {
+ // Resolve snapshot details from the metadata file represented by this
entry because snapshots
+ // may have been removed from the current table history after snapshot
expiration.
+ TableMetadata metadata =
+ metadataLogEntry.file().equals(current.metadataFileLocation())
+ ? current
+ : TableMetadataParser.read(table().io(), metadataLogEntry.file());
Review Comment:
Hi @uros-b thanks for pointing this out.
I considered this trade-off when designing and left the read unguarded
intentionally. A metadata file that is still referenced by the metadata log is
expected to exist and be readable, so returning null would conflate an
inaccessible or corrupt metadata entry with a metadata version that genuinely
has no snapshot.
The projection check is intended to preserve timestamp/file-only scans
without reading historical metadata files. When latest_* fields are requested,
my preference is to fail rather than silently return potentially misleading
values.
--
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]
