You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@iceberg.apache.org by "the-onewho-knocks (via GitHub)" <gi...@apache.org> on 2026/06/26 12:57:05 UTC

[PR] feat: let WriteManifest set and return v3 first_row_id [iceberg-go]

the-onewho-knocks opened a new pull request, #1321:
URL: https://github.com/apache/iceberg-go/pull/1321

   **Summary**
   
   * add `WithManifestFileFirstRowID`, a `ManifestFileOption` that sets `first_row_id` on a v3+ data manifest
   * forward variadic `ManifestFileOption` args through `WriteManifest` to `ToManifestFile`
   
   **Why**
   
   `WriteManifest` predates the v3 work in #735 and forwarded no options to `ToManifestFile`, so `ManifestFile.FirstRowID()` was always `nil` for manifests written through this public helper, even though `ManifestWriter`/`ToManifestFile` already supported setting it via `opts`. Library consumers had no way to set or observe `first_row_id` through `WriteManifest`.
   
   `WriteManifest`'s signature change is purely additive (`opts` is the last, variadic parameter), so existing call sites are unaffected.
   
   **Testing**
   
   * `go test ./... -run '^TestWriteManifestWithFirstRowIDOption$' -count=1`
   * `go test ./... -count=1`
   * `gofmt -l .`
   * `golangci-lint run`
   
   Fixes #1274


-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: let WriteManifest set and return v3 first_row_id [iceberg-go]

Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.

laskoviymishka merged PR #1321:
URL: https://github.com/apache/iceberg-go/pull/1321


-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: let WriteManifest set and return v3 first_row_id [iceberg-go]

Posted by "zeroshade (via GitHub)" <gi...@apache.org>.

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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org