tomtongue commented on code in PR #16859:
URL: https://github.com/apache/iceberg/pull/16859#discussion_r4006282857
##########
core/src/main/java/org/apache/iceberg/MetadataLogEntriesTable.java:
##########
@@ -119,6 +150,32 @@ private static StaticDataTask.Row metadataLogEntryToRow(
// latest snapshot in this file corresponding to the log entry
latestSnapshotId,
latestSnapshot != null ? latestSnapshot.schemaId() : null,
- latestSnapshot != null ? latestSnapshot.sequenceNumber() : null);
+ latestSnapshot != null ? latestSnapshot.sequenceNumber() : null,
+ properties);
+ }
+
+ private static Map<String, String> loadTableProperties(
+ TableMetadata.MetadataLogEntry metadataLogEntry,
+ FileIO io,
+ TableMetadata current,
+ boolean skipPropertiesLoad) {
+
+ // Avoid loading metadata file when properties are not projected.
+ if (skipPropertiesLoad) {
+ return null;
+ }
+
+ // Reuse the already loaded current metadata.
+ if (metadataLogEntry.file().equals(current.metadataFileLocation())) {
+ return current.properties();
+ }
+
+ try {
+ return TableMetadataParser.read(io,
metadataLogEntry.file()).properties();
+ } catch (NotFoundException e) {
Review Comment:
Thanks for raising this. I carefully considered your comment but I would
prefer to keep the fallback limited to missing historical metadata files. For
other read failures, returning `null` would let the query succeed with
incomplete property history. This potentially hides a storage or metadata
problem. Propagating the error makes the failure visible to the caller.
Catching `RuntimeIOException` also would not cover all unreadable-file
cases. For example, some storage errors propagate as SDK exceptions, and
invalid metadata can raise validation exceptions. Catching all
RuntimeExceptions would additionally risk suppressing programming errors.
So I decided to keep `NotFoundException` as the only exception that produces
`null` and a warning, and added a comment explaining that fallback. I also
updated the test to verify that `RuntimeIOException` propagates, while
retaining the missing-file test. I believe this keeps the behavior aligned with
the PR’s documented handling of missing historical metadata files.
--
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]
