laskoviymishka commented on code in PR #2095:
URL: https://github.com/apache/iceberg-go/pull/2095#discussion_r4165106172
##########
table/metadata.go:
##########
@@ -1517,12 +1517,17 @@ func (b *MetadataBuilder) buildCommonMetadata()
(*commonMetadata, error) {
b.lastUpdatedMS = time.Now().UnixMilli()
}
+ // Build may run more than once on the same builder, so the log is
extended on a
+ // copy. Appending to b.metadataLog would add the previous file again
on every call.
+ metadataLog := b.metadataLog
if b.previousFileEntry != nil && b.HasChanges() {
maxMetadataLogEntries := max(1,
b.props.GetInt(
MetadataPreviousVersionsMaxKey,
MetadataPreviousVersionsMaxDefault))
- b.AppendMetadataLog(*b.previousFileEntry)
- b.TrimMetadataLogs(maxMetadataLogEntries)
+ metadataLog = append(slices.Clone(b.metadataLog),
*b.previousFileEntry)
+ if len(metadataLog) > maxMetadataLogEntries {
+ metadataLog =
metadataLog[len(metadataLog)-maxMetadataLogEntries:]
Review Comment:
This inline trim is now a copy of `TrimMetadataLogs`, just without the
receiver mutation. I'd pull the shared logic into a pure helper (something like
`trimMetadataLog(log, maxEntries)` that returns the last `maxEntries` without
touching its input) and call it from both here and `TrimMetadataLogs`.
Otherwise a future change to trim semantics has to land in two places and one
will drift.
##########
table/metadata.go:
##########
@@ -1517,12 +1517,17 @@ func (b *MetadataBuilder) buildCommonMetadata()
(*commonMetadata, error) {
b.lastUpdatedMS = time.Now().UnixMilli()
}
+ // Build may run more than once on the same builder, so the log is
extended on a
+ // copy. Appending to b.metadataLog would add the previous file again
on every call.
+ metadataLog := b.metadataLog
Review Comment:
I'd clone here too, `metadataLog := slices.Clone(b.metadataLog)`, so both
exits return a fresh slice. The changed branch below clones but this no-changes
path hands back `b.metadataLog` by alias; it's safe today given how `append`
behaves, but the asymmetry is the kind of thing that bites whoever edits this
next, and it's one alloc only on the no-change path.
##########
table/metadata_builder_internal_test.go:
##########
@@ -1489,6 +1489,48 @@ func TestExpireMetadataLog(t *testing.T) {
require.Len(t, meta.(*metadataV2).MetadataLog, 2)
}
+func TestBuildDoesNotGrowMetadataLog(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ require.NoError(t, builder.SetProperties(map[string]string{"test.prop":
"value"}))
+
+ for range 3 {
+ meta, err := builder.Build()
+ require.NoError(t, err)
+ require.Len(t, meta.(*metadataV2).MetadataLog, 1)
Review Comment:
Since this is an internal test, I'd assert on the builder's own slice
directly inside the loop, `require.Len(t, builder.metadataLog, 1, "builder log
must not grow across Build calls")`. Right now non-mutation is inferred from
the returned length staying at 1; asserting on `builder.metadataLog` pins the
exact invariant the fix is about.
##########
table/metadata.go:
##########
@@ -1517,12 +1517,17 @@ func (b *MetadataBuilder) buildCommonMetadata()
(*commonMetadata, error) {
b.lastUpdatedMS = time.Now().UnixMilli()
Review Comment:
Not blocking, but while we're making `Build()` side-effect-free on the log:
it still writes `b.lastUpdatedMS` on the first call, and `updateSnapshotLog()`
further down still writes back `b.snapshotLog`, so `Build()` is idempotent on
the metadata log now but not on those two. Harmless in the current CoW path
since both get overwritten from the snapshot timestamp before commit, but a
builder cloned after a `Build()` inherits a frozen `lastUpdatedMS`. Do we
extend the same copy-on-build treatment here, or document that `Build()` caches
these on purpose? wdyt?
##########
table/metadata_builder_internal_test.go:
##########
@@ -1489,6 +1489,48 @@ func TestExpireMetadataLog(t *testing.T) {
require.Len(t, meta.(*metadataV2).MetadataLog, 2)
}
+func TestBuildDoesNotGrowMetadataLog(t *testing.T) {
Review Comment:
Could we add a case for the other new branch, `previousFileEntry` set but
`HasChanges() == false`? That's the path where the fix returns `b.metadataLog`
untouched, and right now `builderWithoutChanges` immediately gets a
`SetProperties` so every test enters the active-changes branch. A quick
`Build()` with no updates asserting `PreviousFiles()` stays empty would cover
it.
##########
table/copy_on_write_metadata_log_test.go:
##########
@@ -0,0 +1,64 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table_test
+
+import (
+ "context"
+ "path/filepath"
+ "slices"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ iceio "github.com/apache/iceberg-go/io"
+ "github.com/apache/iceberg-go/table"
+ "github.com/stretchr/testify/require"
+)
+
+// A copy-on-write delete builds the transaction's metadata once per rewritten
Review Comment:
The comment says `Build` runs "once per rewritten file," but the duplicates
on `main` actually come from classification and `StagedTable()` building the
same builder too, not just the per-file rewrites, so the real count is higher
than the comment implies. I'd reword it so the regression stays legible if
those call sites ever move. While here, give the final `require.Len` a message
like the setup assertion has.
--
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]
