alliasgher commented on code in PR #877:
URL: https://github.com/apache/iceberg-go/pull/877#discussion_r3074645318
##########
table/table_test.go:
##########
@@ -1705,6 +1705,62 @@ func (t *TableWritingTestSuite)
TestExpireSnapshotsNoOpWhenNothingToExpire() {
"metadata location should not change when there are no
snapshots to expire")
}
+// TestExpireSnapshotsUsesTableProperties verifies that ExpireSnapshots reads
+// min-snapshots-to-keep and max-snapshot-age-ms from table properties when
+// no explicit options are provided by the caller (mirrors Java behaviour).
+func (t *TableWritingTestSuite) TestExpireSnapshotsUsesTableProperties() {
+ fs := iceio.LocalFS{}
+
+ files := make([]string, 0)
+ for i := range 5 {
+ filePath := fmt.Sprintf("%s/expire_props_v%d/data-%d.parquet",
t.location, t.formatVersion, i)
+ t.writeParquet(fs, filePath, t.arrTablePromotedTypes)
+ files = append(files, filePath)
+ }
+
+ ident := table.Identifier{"default", "expire_props_v" +
strconv.Itoa(t.formatVersion)}
+ // Set table-level retention properties — no caller options will be
passed to
+ // ExpireSnapshots, so these must be picked up automatically.
+ meta, err := table.NewMetadata(t.tableSchemaPromotedTypes,
iceberg.UnpartitionedSpec,
+ table.UnsortedSortOrder, t.location, iceberg.Properties{
+ table.PropertyFormatVersion:
strconv.Itoa(t.formatVersion),
+ table.MinSnapshotsToKeepKey: "2",
+ table.MaxSnapshotAgeMsKey: "0", // expire everything
older than "now"
+ table.MaxRefAgeMsKey:
strconv.FormatInt(int64(table.MaxRefAgeMsDefault), 10),
Review Comment:
Removed in d1bd3cd and replaced with a comment explaining the intent — the
absent property is now the thing being tested, not an explicitly-set default.
##########
table/transaction.go:
##########
@@ -220,6 +220,15 @@ func (t *Transaction) ExpireSnapshots(opts
...ExpireSnapshotsOpt) error {
opt(&cfg)
}
+ // Read table-level retention properties as the last-resort defaults,
+ // mirroring the Java implementation. When neither the ref nor the
+ // caller provides a value, fall back to the table property; when the
+ // table property is also absent use the constant default (math.MaxInt,
+ // meaning "keep everything").
Review Comment:
Good point — on a 32-bit system `math.MaxInt` is `math.MaxInt32`, so casting
to `int64` gives 2147483647 rather than `math.MaxInt64`. In practice Iceberg
ages are in milliseconds so MaxInt32 ms ≈ 24 days, which would incorrectly
expire refs that should be kept forever. That said, the codebase targets 64-bit
(same as the Java reference), and `MaxRefAgeMsDefault` /
`MaxSnapshotAgeMsDefault` are both typed as untyped `int` constants today —
changing the property read to use 64-bit arithmetic would require a
`strconv.ParseInt` or a new `GetInt64` on `Properties`. Happy to add that as a
follow-up if you'd like.
--
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]
