laskoviymishka commented on code in PR #1582:
URL: https://github.com/apache/iceberg-go/pull/1582#discussion_r3680745323
##########
table/snapshots_internal_test.go:
##########
@@ -217,3 +217,18 @@ func TestUpdateSnapshotSummariesUnsupportedOperation(t
*testing.T) {
_, err := updateSnapshotSummaries(Summary{Operation:
Operation("scan")}, nil)
assert.ErrorIs(t, err, iceberg.ErrNotImplemented)
}
+
+func TestUpdateSnapshotSummariesPreservesLargeTotals(t *testing.T) {
+ t.Parallel()
Review Comment:
small thing — this is the only test in the file that opts into
`t.Parallel()`; the siblings (`TestMergeSnapshotSummaries`,
`TestSnapshotSummaryCollector`, ...) all run sequentially. It's a pure-function
test so there's no race concern either way, but the lone opt-in is a little
inconsistent. I'd just drop it to match the file, wdyt?
##########
table/snapshots_internal_test.go:
##########
@@ -217,3 +217,18 @@ func TestUpdateSnapshotSummariesUnsupportedOperation(t
*testing.T) {
_, err := updateSnapshotSummaries(Summary{Operation:
Operation("scan")}, nil)
assert.ErrorIs(t, err, iceberg.ErrNotImplemented)
}
+
+func TestUpdateSnapshotSummariesPreservesLargeTotals(t *testing.T) {
+ t.Parallel()
+
+ result, err := updateSnapshotSummaries(Summary{Operation: OpAppend,
Properties: iceberg.Properties{
+ addedRecordsKey: "1000000000",
+ addedFileSizeKey: "1000000000",
+ }}, iceberg.Properties{
+ totalRecordsKey: "3000000000",
+ totalFileSizeKey: "8000000000",
+ })
+ require.NoError(t, err)
+ assert.Equal(t, "4000000000", result.Properties[totalRecordsKey])
Review Comment:
the fix is right, but this only exercises 2 of the 6 total paths (records +
file-size, addition only) and asserts two individual keys. I'd add a case that
removes a large value from a large starting total, and cover one of the
file-count totals too — the three `GetInt64` reads are independent, so a
partial reversion of just the `removed` one wouldn't be caught here. Asserting
the full result `Properties` map (the way `TestMergeSnapshotSummaries` does)
would also catch a regression that clobbers the other totals.
Worth knowing too: this proves the int64 math is correct, but it can't guard
against a revert to `GetInt` — the truncation only happens on a 32-bit host,
and CI only runs tests on amd64/arm64 (s390x just cross-compiles). So on every
runner we actually have, the old `GetInt` path would pass this test too. A
one-line comment saying so, or a build-tagged (386||arm) assertion, would make
the intent clear.
--
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]
