zeroshade commented on code in PR #1321:
URL: https://github.com/apache/iceberg-go/pull/1321#discussion_r3482956971
##########
manifest.go:
##########
@@ -1274,6 +1274,13 @@ func WithManifestFileContent(content ManifestContent)
ManifestFileOption {
}
}
+// WithManifestFileFirstRowID sets the first_row_id on a v3+ data manifest.
+func WithManifestFileFirstRowID(firstRowID int64) ManifestFileOption {
+ return func(mf *manifestFile) {
+ mf.FirstRowIDValue = &firstRowID
Review Comment:
nit: `mf.version` is already set before options are applied, so this could
self-gate to v3+ where `first_row_id` actually exists:
```go
if mf.version >= 3 {
mf.FirstRowIDValue = &firstRowID
}
```
As written, calling this against a v1/v2 writer returns a `ManifestFile`
whose `FirstRowID()` is non-nil but gets silently dropped on serialize.
`WithManifestFileContent` has the same no-guard pattern so I'm fine either way,
but the v3 exclusivity is a bit stronger here.
##########
manifest.go:
##########
@@ -1628,6 +1635,7 @@ func WriteManifest(
schema *Schema,
snapshotID int64,
entries []ManifestEntry,
+ opts ...ManifestFileOption,
Review Comment:
nit (pre-existing): `WriteManifest` is exported but has no doc comment.
Since you're already touching the signature, a one-liner noting that `opts` set
v3-specific descriptor fields (e.g. `WithManifestFileFirstRowID`) would be a
welcome add.
##########
manifest_test.go:
##########
@@ -1158,6 +1158,66 @@ func (m *ManifestTestSuite)
TestV3DataManifestFirstRowIDInheritanceSkipsDeletedE
m.EqualValues(1000+liveCount, *read[2].DataFile().FirstRowID())
}
+func (m *ManifestTestSuite) TestWriteManifestWithFirstRowIDOption() {
Review Comment:
nit: consider adding (a) a v1/v2 + `WithManifestFileFirstRowID` case to pin
the intended behavior (no-op vs non-nil `FirstRowID()`), and (b) a
delete-manifest case, since the spec restricts `first_row_id` to data
manifests. The read side already gates inheritance on `formatVersion >= 3 &&
content == data`, so this just locks down the write side.
--
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]
