laskoviymishka commented on code in PR #16859:
URL: https://github.com/apache/iceberg/pull/16859#discussion_r3961172200
##########
core/src/main/java/org/apache/iceberg/MetadataLogEntriesTable.java:
##########
@@ -19,20 +19,43 @@
package org.apache.iceberg;
import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.exceptions.NotFoundException;
import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.SnapshotUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+/**
+ * A {@link Table} implementation that exposes a table's metadata log as rows.
+ *
+ * <p>Each row represents a historical or current metadata file and includes
the snapshot details
+ * and table properties recorded in that file. The current metadata is
included as the latest row.
+ *
+ * <p>Queries that reference {@code properties} read each retained historical
metadata file. These
+ * additional reads are skipped when {@code properties} is not referenced, and
the already loaded
+ * current metadata is reused.
+ */
public class MetadataLogEntriesTable extends BaseMetadataTable {
+ private static final int PROPERTIES_FIELD_ID = 6;
Review Comment:
`PROPERTIES_FIELD_ID = 6` drives both the schema declaration and the
`findField(PROPERTIES_FIELD_ID) == null` skip, but nothing checks the two agree
— if a field ever gets inserted before `properties`, the constant silently
points elsewhere and the skip mis-fires. A small static assertion that
`METADATA_LOG_ENTRIES_SCHEMA.findField(PROPERTIES_FIELD_ID)` is non-null and
named `properties` would keep them honest.
Tiny thing while we're here: `LOG` usually sits at the top of the constant
block by convention.
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -644,19 +644,22 @@ public void testMetadataLogEntries() throws Exception {
metadataLogEntries.get(0).file(),
null,
null,
- null),
+ null,
+ tableMetadata.properties()),
Review Comment:
these new `properties` assertions all compare against
`tableMetadata.properties()`, which is the current (final) map — and since
`testMetadataLogEntries` never changes a property, every historical row happens
to equal current. So this stays green even if `loadTableProperties` just
returned `current.properties()` for every entry.
The real history coverage now lives in
`testMetadataLogEntriesPropertyHistory`, so I wouldn't block on this. I'd
either assert the concrete expected map per row here, or drop a one-line
comment noting the properties are identical across entries in this test so it's
clear the divergence check lives elsewhere. Same in the Flink
`testMetadataLogEntries` copies. wdyt?
##########
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:
we only catch `NotFoundException` here, but `TableMetadataParser.read`
rethrows other IO problems as `RuntimeIOException` — a present-but-unreadable
file (permissions, a truncated/corrupt JSON, a transient read error) escapes
and fails the whole query instead of degrading to null+warn like the
missing-file case.
Since this is already best-effort, I'd broaden the catch
(`RuntimeException`, or at least `RuntimeIOException`) and log the
class+message. If we'd rather keep it narrow, worth a comment saying only
missing files degrade gracefully. wdyt?
##########
flink/v2.3/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkMetaDataTable.java:
##########
@@ -667,6 +667,23 @@ public void testMetadataLogEntries() {
}
}
+ @TestTemplate
Review Comment:
v2.3 only picked up the new `testMetadataLogEntriesPropertyHistory` — the
four `getField("properties")` assertions that landed in v1.20/2.1/2.2's
`testMetadataLogEntries` didn't make it into this module. Same base commit, so
it looks like a copy that got missed rather than a real version difference. I'd
add the same four here so the base-case coverage matches across Flink versions.
--
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]
