CaptainAni187 commented on PR #2109: URL: https://github.com/apache/iceberg-go/pull/2109#issuecomment-6022126933
@laskoviymishka Thanks for the thorough review. I went with Build being repeatable, since that's what #2108 asks for, and made the contract explicit on both `Build` methods. - **Timestamp:** `lastUpdatedMS` is now computed into a local in `table.MetadataBuilder.Build` and no longer stored on the builder. Building, making more changes and building again gives a later last-updated time (`TestBuild_LastUpdatedIsNotFixedByAnEarlierBuild`, which fails on the previous commit). `TestLastUpdateIncreasedForPropertyOnlyUpdate` now reads the time from the built metadata instead of the builder field. - **View UUID:** the UUID generated for a new view without one is now kept on the builder and reused, so retried builds describe the same view. It's in its own field rather than `b.uuid`, so `SetUUID` still works after a build. `TestBuild_RepeatedBuildsAreStable` checks both. - **Pruning path:** `TestRemoveSnapshotsPrunesSnapshotLogHistory` builds twice after `RemoveSnapshots`, compares the logs, and checks `builder.snapshotLog` keeps all three entries. That last check fails on main. - **View expiry path:** `TestBuild_DoesNotGrowVersionLogWhenExpiringVersions` uses `VersionHistorySizeKey=1` with three versions, so `updateHistory` runs. - **Retry from existing metadata:** `TestBuild_FromBaseDoesNotGrowVersionLog` starts from metadata with a non-empty version log. Both tests also compare the UUID across three builds. - **Mutate-then-build-again:** each `Build` describes the builder's whole change set from its base, so after `SetCurrentVersionID` is called again the log records the version the builder now ends at. A version that was set in between but never committed isn't kept. That's also what happens when the current version is set twice before a single build, and as far as I can tell it matches Java's view builder. I've left it as is, but tell me if you'd rather keep intermediate entries. I merged `main` into the branch. `go test ./table/... ./view/...` and golangci-lint pass locally. -- 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]
