feat(table): add metadata log entries table - #1696
Conversation
b9635b0 to
471388a
Compare
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Nice addition — this lines up with Java's MetadataLogEntriesTable really well: the equal-timestamp tie-break, keeping the snapshot ID when the snapshot itself has expired, and the microsecond timestamp all match, and the tests hit the cases that matter.
The one thing I'd like to settle before merge is the documentation around latestSnapshotAt. The behavior is right, but the doc comment reads like point-in-time semantics when it actually resolves against the current snapshot log (so trimming can move the answer), and the equal-timestamp tie-break quietly diverges from PyIceberg. Both are the correct, Java-compatible calls — they just need a line or two of comment so nobody unwinds them later.
A few smaller things I left inline:
- the
int32(*snapshot.SchemaID)cast is unguarded (fine by spec, but it'll trip G115 and the sibling tables don't cast) - empty
metadataLocationwould write""into the requiredfilecolumn - worth a checked-allocator test for the new method, mirroring the Snapshots one
None of these block — happy to approve once the latestSnapshotAt semantics are documented. Nice work.
| var latestTimestamp int64 | ||
| found := false | ||
| for entry := range metadata.SnapshotLogs() { | ||
| if entry.TimestampMs <= timestampMs && (!found || entry.TimestampMs > latestTimestamp) { |
There was a problem hiding this comment.
The behavior here is right and matches Java's SnapshotUtil.snapshotIdAsOfTime, but I'd tighten the doc comment above so nobody unwinds it later.
Two things it should call out. First, this resolves against the current snapshot log, so if an older snapshot was trimmed by ExpireSnapshots the result can shift to a later entry (or none). The doc's "the snapshot-log entry at or before the metadata timestamp" reads like point-in-time semantics, which it isn't under trimming — it's the correct, Java-compatible choice, it just needs to say so.
Second, the > latestTimestamp tie-break keeps the first entry on equal-ms timestamps. That matches Java, but PyIceberg's snapshot_as_of_timestamp iterates in reverse and returns the last. One line on the > noting the divergence is intentional would keep someone from "correcting" it. wdyt?
| continue | ||
| } | ||
| if snapshot.SchemaID != nil { | ||
| latestSchemaID.Append(int32(*snapshot.SchemaID)) |
There was a problem hiding this comment.
SchemaID is spec-bounded to int32 so this never actually overflows, but the bare int32(*snapshot.SchemaID) is a narrowing cast that'll trip gosec G115 if we ever enable it, and the sibling tables don't cast (snapshots.go uses the value as a plain int). I'd add a short //nolint:gosec noting it's bounded by spec, or a guard — either's fine, just so the intent is explicit. wdyt?
| entries = append(entries, entry) | ||
| } | ||
| entries = append(entries, MetadataLogEntry{ | ||
| MetadataFile: i.tbl.metadataLocation, |
There was a problem hiding this comment.
If a table is ever built with an empty metadataLocation, this synthetic current entry writes "" into file, which is a required (non-null) column — non-null but semantically empty. TestInspectMetadataLogEntriesEmpty passes a real path so it doesn't cover this. Java skips the synthetic entry when the location isn't set; I'd either match that or document the assumption that metadataLocation is always populated here.
| } else { | ||
| latestSchemaID.AppendNull() | ||
| } | ||
| latestSequenceNumber.Append(snapshot.SequenceNumber) |
There was a problem hiding this comment.
For a V1 snapshot SequenceNumber is the Go zero value 0, and it's appended unconditionally to this optional column, so there's no way to tell "V1, no sequence numbers" from "V2, sequence 0". Java and PyIceberg both emit 0 here too, so this matches every client and I wouldn't block on it. If we wanted to be stricter than Java we could emit null for V1 (Version() < 2), but that's genuinely optional.
| TimestampMs: i.tbl.metadata.LastUpdatedMillis(), | ||
| }) | ||
|
|
||
| bldr := array.NewRecordBuilder(i.alloc, arrowSchema) |
There was a problem hiding this comment.
This builds five column builders and can early-return on ctx.Err() mid-build, so I'd like a checked-allocator test asserting everything's released. TestInspectAllocatorOption only exercises Snapshots today. Could we add a TestInspectMetadataLogEntriesAllocator mirroring it with memory.NewCheckedAllocator and asserting zero bytes after release? wdyt?
| SnapshotLog: []SnapshotLogEntry{ | ||
| {SnapshotID: firstSnapshot, TimestampMs: 2000}, | ||
| {SnapshotID: secondSnapshot, TimestampMs: 3000}, | ||
| {SnapshotID: lateSnapshot, TimestampMs: 2500}, |
There was a problem hiding this comment.
This fixture is out of chronological order by 500ms (2000, 3000, 2500), which passes today only because validateChronologicalSnapshotLogs allows a 1-minute skew tolerance. If that tolerance is ever tightened this becomes invalid metadata and the test fails for an unrelated reason. A one-line comment on whether this is meant to be genuinely out-of-order or just within-tolerance clock skew would make the intent clear.
| return nil, fmt.Errorf("inspect metadata log entries: build arrow schema: %w", err) | ||
| } | ||
|
|
||
| entries := make([]MetadataLogEntry, 0) |
There was a problem hiding this comment.
Small one: PreviousFiles() has a known length so this could be pre-sized, or the loop collapsed with slices.Collect (already used in metadata.go). Not load-bearing.
2 participants
Summary
Adds the
metadata_log_entriesmetadata table toTable.Inspect.Tests
go test ./tableScope
This adds metadata inspection only. It does not change metadata loading or snapshot selection behavior.