laskoviymishka commented on code in PR #877:
URL: https://github.com/apache/iceberg-go/pull/877#discussion_r3074235195
##########
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:
This sets max-ref-age-ms to math.MaxInt, which is exactly what GetInt
returns when the property is absent.
This line has no effect and is test noise.
Remove it, the test should prove that missing properties fall back to
defaults correctly, not that explicitly-set-to-default properties work.
##########
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:
The defaults here are math.MaxInt — meaning "keep everything" when no
property is set. That's correct and matches Java.
One thing: GetInt parses via strconv.ParseInt(..., 10, 64) and returns int.
Then you cast to int64. On 64-bit systems int == int64 so this is fine, but on
32-bit (if that's ever a target) math.MaxInt as int would be math.MaxInt32,
then cast to int64 would be 2147483647 — not math.MaxInt64. Probably doesn't
matter, but worth a note.
--
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]
