laskoviymishka commented on code in PR #1695:
URL: https://github.com/apache/iceberg-go/pull/1695#discussion_r3740290760
##########
table/inspect.go:
##########
@@ -247,6 +251,87 @@ func (i InspectTable) Snapshots(ctx context.Context)
(array.RecordReader, error)
return rr, nil
}
+// Refs returns one row per snapshot reference known to the table. Reference
+// names are sorted to make the result deterministic even though table metadata
+// stores refs in a map.
+//
+// Columns:
+// - name (string, required): the branch or tag name
+// - type (string, required): BRANCH or TAG
+// - snapshot_id (long, required): the referenced snapshot
+// - max_reference_age_in_ms (long, optional): tag/branch reference retention
+// - min_snapshots_to_keep (int, optional): branch snapshot retention
+// - max_snapshot_age_in_ms (long, optional): branch snapshot retention
+//
+// The returned reader holds a single record batch. The caller must Release it.
+func (i InspectTable) Refs(ctx context.Context) (array.RecordReader, error) {
+ arrowSchema, err := SchemaToArrowSchema(RefsSchema(), nil, true, false)
+ if err != nil {
+ return nil, fmt.Errorf("inspect refs: build arrow schema: %w",
err)
+ }
+
+ type refRow struct {
+ name string
+ ref SnapshotRef
+ }
Review 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.
##########
table/inspect.go:
##########
@@ -247,6 +251,87 @@ func (i InspectTable) Snapshots(ctx context.Context)
(array.RecordReader, error)
return rr, nil
}
+// Refs returns one row per snapshot reference known to the table. Reference
+// names are sorted to make the result deterministic even though table metadata
+// stores refs in a map.
+//
+// Columns:
+// - name (string, required): the branch or tag name
+// - type (string, required): BRANCH or TAG
+// - snapshot_id (long, required): the referenced snapshot
+// - max_reference_age_in_ms (long, optional): tag/branch reference retention
+// - min_snapshots_to_keep (int, optional): branch snapshot retention
+// - max_snapshot_age_in_ms (long, optional): branch snapshot retention
+//
+// The returned reader holds a single record batch. The caller must Release it.
+func (i InspectTable) Refs(ctx context.Context) (array.RecordReader, error) {
+ arrowSchema, err := SchemaToArrowSchema(RefsSchema(), nil, true, false)
+ if err != nil {
+ return nil, fmt.Errorf("inspect refs: build arrow schema: %w",
err)
+ }
+
+ type refRow struct {
+ name string
+ ref SnapshotRef
+ }
+ refs := make([]refRow, 0)
+ for name, ref := range i.tbl.metadata.Refs() {
+ refs = append(refs, refRow{name: name, ref: ref})
+ }
+ slices.SortFunc(refs, func(a, b refRow) int {
+ return cmp.Compare(a.name, b.name)
+ })
+
+ bldr := array.NewRecordBuilder(i.alloc, arrowSchema)
+ defer bldr.Release()
+
+ name := bldr.Field(0).(*array.StringBuilder)
+ refType := bldr.Field(1).(*array.StringBuilder)
+ snapshotID := bldr.Field(2).(*array.Int64Builder)
+ maxReferenceAge := bldr.Field(3).(*array.Int64Builder)
+ minSnapshotsToKeep := bldr.Field(4).(*array.Int32Builder)
+ maxSnapshotAge := bldr.Field(5).(*array.Int64Builder)
+
+ for _, row := range refs {
+ if err := ctx.Err(); err != nil {
+ return nil, err
+ }
+
+ name.Append(row.name)
+ refType.Append(strings.ToUpper(string(row.ref.SnapshotRefType)))
+ snapshotID.Append(row.ref.SnapshotID)
+
+ if row.ref.MaxRefAgeMs != nil {
+ maxReferenceAge.Append(*row.ref.MaxRefAgeMs)
+ } else {
+ maxReferenceAge.AppendNull()
+ }
+ if row.ref.MinSnapshotsToKeep != nil {
Review 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?
##########
table/inspect_internal_test.go:
##########
@@ -245,6 +247,152 @@ func TestInspectHistoryNoCurrentSnapshot(t *testing.T) {
require.False(t, isCurrentAncestor.Value(0), "no current snapshot means
no ancestors")
}
+func TestInspectRefsSchema(t *testing.T) {
+ sc := RefsSchema()
+
+ require.Equal(t, []string{
+ "name",
+ "type",
+ "snapshot_id",
+ "max_reference_age_in_ms",
+ "min_snapshots_to_keep",
+ "max_snapshot_age_in_ms",
+ }, testFieldNames(sc))
+
+ fields := sc.Fields()
+ require.Equal(t, []int{1, 2, 3, 4, 5, 6}, []int{
+ fields[0].ID,
+ fields[1].ID,
+ fields[2].ID,
+ fields[3].ID,
+ fields[4].ID,
+ fields[5].ID,
+ })
+ require.Equal(t, iceberg.PrimitiveTypes.String, fields[0].Type)
+ require.Equal(t, iceberg.PrimitiveTypes.String, fields[1].Type)
+ require.Equal(t, iceberg.PrimitiveTypes.Int64, fields[2].Type)
+ require.Equal(t, iceberg.PrimitiveTypes.Int64, fields[3].Type)
+ require.Equal(t, iceberg.PrimitiveTypes.Int32, fields[4].Type)
+ require.Equal(t, iceberg.PrimitiveTypes.Int64, fields[5].Type)
+ require.True(t, fields[0].Required)
+ require.True(t, fields[1].Required)
+ require.True(t, fields[2].Required)
+ require.False(t, fields[3].Required)
+ require.False(t, fields[4].Required)
+ require.False(t, fields[5].Required)
+}
+
+func TestInspectRefs(t *testing.T) {
+ tbl := historyTestTable()
+ minSnapshotsToKeep := 2
+ maxSnapshotAge := int64(3000)
Review 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?
##########
table/inspect_internal_test.go:
##########
@@ -245,6 +247,152 @@ func TestInspectHistoryNoCurrentSnapshot(t *testing.T) {
require.False(t, isCurrentAncestor.Value(0), "no current snapshot means
no ancestors")
}
+func TestInspectRefsSchema(t *testing.T) {
+ sc := RefsSchema()
+
+ require.Equal(t, []string{
+ "name",
+ "type",
+ "snapshot_id",
+ "max_reference_age_in_ms",
+ "min_snapshots_to_keep",
+ "max_snapshot_age_in_ms",
+ }, testFieldNames(sc))
+
+ fields := sc.Fields()
+ require.Equal(t, []int{1, 2, 3, 4, 5, 6}, []int{
+ fields[0].ID,
+ fields[1].ID,
+ fields[2].ID,
+ fields[3].ID,
+ fields[4].ID,
+ fields[5].ID,
+ })
+ require.Equal(t, iceberg.PrimitiveTypes.String, fields[0].Type)
+ require.Equal(t, iceberg.PrimitiveTypes.String, fields[1].Type)
+ require.Equal(t, iceberg.PrimitiveTypes.Int64, fields[2].Type)
+ require.Equal(t, iceberg.PrimitiveTypes.Int64, fields[3].Type)
+ require.Equal(t, iceberg.PrimitiveTypes.Int32, fields[4].Type)
+ require.Equal(t, iceberg.PrimitiveTypes.Int64, fields[5].Type)
+ require.True(t, fields[0].Required)
+ require.True(t, fields[1].Required)
+ require.True(t, fields[2].Required)
+ require.False(t, fields[3].Required)
+ require.False(t, fields[4].Required)
+ require.False(t, fields[5].Required)
+}
Review 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.
--
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]
