feat(table): add refs metadata table - #1695
Conversation
6032e64 to
1a92fec
Compare
laskoviymishka
left a comment
There was a problem hiding this comment.
Really clean clone of the existing inspect pattern: schema IDs, names, and flags match Java's RefsTable, the BRANCH/TAG casing matches Java's enum names, and sorting by ref name plus the empty-refs test are exactly right.
Almost there. The one thing I'd want before merge is a guard on the int32(*row.ref.MinSnapshotsToKeep) cast. MinSnapshotsToKeep is a *int, and with MinSnapshotsToKeepDefault being math.MaxInt, that default landing in a ref would silently append -1. Either bounds-check and return an error before the append, or move the field to *int32. Narrow trigger, but it's silent when it's wrong.
The rest is just matching the sibling tests: a checked-allocator test for Refs, and pulling a refsTestTable() helper instead of reaching into tbl.metadata.(*metadataV2). Both are inline.
Once the overflow guard's in, happy to take another pass and approve.
| type refRow struct { | ||
| name string | ||
| ref SnapshotRef | ||
| } |
There was a problem hiding this comment.
tiny thing while we're here: the sibling methods accumulate into a plain var refs []refRow rather than make([]refRow, 0), or size the make with len(...). Matching them keeps this consistent and skips the eager empty alloc.
| } else { | ||
| maxReferenceAge.AppendNull() | ||
| } | ||
| if row.ref.MinSnapshotsToKeep != nil { |
There was a problem hiding this comment.
MinSnapshotsToKeep is a *int, so on 64-bit platforms this cast silently truncates anything above math.MaxInt32. The concrete way it bites: MinSnapshotsToKeepDefault is math.MaxInt, so if that default ever lands in a stored ref this appends -1 rather than erroring.
I'd guard it before the append, either returning an error when the value is out of int32 range, or switching the field to *int32 if we want the wider change. wdyt?
| require.False(t, fields[3].Required) | ||
| require.False(t, fields[4].Required) | ||
| require.False(t, fields[5].Required) | ||
| } |
There was a problem hiding this comment.
one gap vs. the sibling coverage: TestInspectAllocatorOption runs Snapshots through a memory.CheckedAllocator so a builder leak surfaces at test exit, but there's no equivalent for Refs. Cheap to add, and it'd catch a leak in this new builder path. I'd add a TestInspectRefsAllocator following that pattern.
| func TestInspectRefs(t *testing.T) { | ||
| tbl := historyTestTable() | ||
| minSnapshotsToKeep := 2 | ||
| maxSnapshotAge := int64(3000) |
There was a problem hiding this comment.
the other inspect tests build a complete *metadataV2 in a helper (historyTestTable, snapshotsTestTable) and hand it to New(). Reaching in with tbl.metadata.(*metadataV2) to mutate SnapshotRefs is more fragile: it panics if metadata is ever wrapped.
I'd pull a refsTestTable() helper that sets the branch and tag refs up front and share it between TestInspectRefs and TestInspectRefsEmpty, matching the sibling pattern. wdyt?
Summary
Adds the
refsmetadata table toTable.Inspect.Tests
go test ./tablemin_snapshots_to_keep.Scope
This adds metadata inspection only. It does not change snapshot-reference creation or retention behavior.