zeroshade commented on code in PR #1321:
URL: https://github.com/apache/iceberg-go/pull/1321#discussion_r3493439912
##########
manifest.go:
##########
@@ -1306,6 +1306,10 @@ func (w *ManifestWriter) ToManifestFile(location string,
length int64, opts ...M
apply(&mf)
}
+ if mf.Content == ManifestContentDeletes && mf.FirstRowIDValue != nil {
Review Comment:
This guard is unreachable from the public API: no `ManifestFileOption` sets
`FirstRowIDValue` (only `WithManifestFileContent` exists) and `ToManifestFile`
inits it to nil. The PR description mentions a `WithManifestFileFirstRowID`
option + variadic `WriteManifest`, but neither is in the diff — only this check
landed. Either add the missing option/variadic, or drop the dead guard for now.
##########
manifest.go:
##########
@@ -1651,6 +1657,52 @@ func WriteManifest(
return w.ToManifestFile(filename, cnt.Count)
}
+// WriteManifestV3 writes a v3 data manifest and assigns first_row_id.
+// The returned ManifestFile has FirstRowID() set to firstRowID.
+// nextFirstRowID is firstRowID + AddedRowsCount + ExistingRowsCount
+// for the caller to chain to the next manifest. Use this only for
+// reconstruction — manifests committed through a v3 list writer get
+// first_row_id from AddManifests; pre-setting collides with that allocation.
+func WriteManifestV3(
+ filename string,
+ out io.Writer,
+ firstRowID int64,
+ spec PartitionSpec,
+ schema *Schema,
+ snapshotID int64,
+ entries []ManifestEntry,
+) (mf ManifestFile, nextFirstRowID int64, err error) {
+ cnt := &internal.CountingWriter{W: out}
+
+ w, err := NewManifestWriter(3, cnt, spec, schema, snapshotID)
+ if err != nil {
+ return nil, 0, err
+ }
+ defer internal.CheckedClose(w, &err)
+
+ for _, entry := range entries {
+ if err := w.addEntry(entry.(*manifestEntry)); err != nil {
+ return nil, 0, err
+ }
+ }
+
+ if err := w.Close(); err != nil {
Review Comment:
`w.Close()` ends up called three times on success (here, again inside
`ToManifestFile`, and via the deferred `CheckedClose`). Idempotent so safe, but
this explicit close is redundant — `ToManifestFile` already closes to finalize
`cnt.Count`. Consider dropping it to mirror `WriteManifest`.
##########
manifest_test.go:
##########
@@ -1158,6 +1159,129 @@ func (m *ManifestTestSuite)
TestV3DataManifestFirstRowIDInheritanceSkipsDeletedE
m.EqualValues(1000+liveCount, *read[2].DataFile().FirstRowID())
}
+func (m *ManifestTestSuite) TestWriteManifestV3() {
Review Comment:
Coverage gap: only `EntryStatusADDED` entries are exercised. Add a mixed
ADDED/EXISTING/DELETED case asserting `nextFirstRowID == firstRowID +
AddedRowsCount + ExistingRowsCount` (and that DELETED is excluded) to lock the
documented contract.
##########
manifest.go:
##########
@@ -1651,6 +1657,52 @@ func WriteManifest(
return w.ToManifestFile(filename, cnt.Count)
}
+// WriteManifestV3 writes a v3 data manifest and assigns first_row_id.
+// The returned ManifestFile has FirstRowID() set to firstRowID.
+// nextFirstRowID is firstRowID + AddedRowsCount + ExistingRowsCount
+// for the caller to chain to the next manifest. Use this only for
+// reconstruction — manifests committed through a v3 list writer get
+// first_row_id from AddManifests; pre-setting collides with that allocation.
+func WriteManifestV3(
+ filename string,
+ out io.Writer,
+ firstRowID int64,
+ spec PartitionSpec,
+ schema *Schema,
+ snapshotID int64,
+ entries []ManifestEntry,
+) (mf ManifestFile, nextFirstRowID int64, err error) {
+ cnt := &internal.CountingWriter{W: out}
+
+ w, err := NewManifestWriter(3, cnt, spec, schema, snapshotID)
+ if err != nil {
+ return nil, 0, err
+ }
+ defer internal.CheckedClose(w, &err)
+
+ for _, entry := range entries {
+ if err := w.addEntry(entry.(*manifestEntry)); err != nil {
+ return nil, 0, err
+ }
+ }
+
+ if err := w.Close(); err != nil {
+ return nil, 0, err
+ }
+
+ mf, err = w.ToManifestFile(filename, cnt.Count)
+ if err != nil {
+ return nil, 0, err
+ }
+
+ raw := mf.(*manifestFile)
+ v := firstRowID
+ raw.FirstRowIDValue = &v
Review Comment:
`FirstRowIDValue` is set post-finalize with no precondition on entry
content/status, so passing delete entries yields `Content=ManifestContentData`
+ a `FirstRowID` anyway — bypassing the guard above. Consider asserting
`Content == ManifestContentData` (or rejecting non-data entries) before
assigning.
--
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]
