laskoviymishka commented on code in PR #1321:
URL: https://github.com/apache/iceberg-go/pull/1321#discussion_r3490780616
##########
manifest_test.go:
##########
@@ -1158,6 +1158,110 @@ func (m *ManifestTestSuite)
TestV3DataManifestFirstRowIDInheritanceSkipsDeletedE
m.EqualValues(1000+liveCount, *read[2].DataFile().FirstRowID())
}
+func (m *ManifestTestSuite) TestWriteManifestWithFirstRowIDOption() {
+ partitionSpec := NewPartitionSpecID(1,
+ PartitionField{FieldID: 1000, SourceIDs: []int{1}, Name: "x",
Transform: IdentityTransform{}})
+ count := int64(10)
+ entries := []ManifestEntry{
+ &manifestEntry{
+ EntryStatus: EntryStatusADDED,
+ Snapshot: &entrySnapshotID,
+ Data: &dataFile{
+ Content: EntryContentData,
+ Path: "/data/file1.parquet",
+ Format: ParquetFile,
+ PartitionData: map[string]any{"x": int(1)},
+ RecordCount: count,
+ FileSize: 1000,
+ BlockSizeInBytes: 64 * 1024,
+ FirstRowIDField: nil,
+ },
+ },
+ &manifestEntry{
+ EntryStatus: EntryStatusADDED,
+ Snapshot: &entrySnapshotID,
+ Data: &dataFile{
+ Content: EntryContentData,
+ Path: "/data/file2.parquet",
+ Format: ParquetFile,
+ PartitionData: map[string]any{"x": int(2)},
+ RecordCount: count,
+ FileSize: 2000,
+ BlockSizeInBytes: 64 * 1024,
+ FirstRowIDField: nil,
+ },
+ },
+ }
+
+ // Test 1: WriteManifest with WithManifestFileFirstRowID sets the field.
+ var bufWithID bytes.Buffer
+ firstRowID := int64(500)
+ mf, err := WriteManifest("/manifest.avro", &bufWithID, 3,
partitionSpec, testSchema, entrySnapshotID, entries,
+ WithManifestFileFirstRowID(firstRowID))
+ m.Require().NoError(err)
+ m.Require().NotNil(mf.FirstRowID())
+ m.Equal(firstRowID, *mf.FirstRowID())
+
+ // Reading back, entries inherit first_row_id from the manifest file.
+ read, err := ReadManifest(mf, bytes.NewReader(bufWithID.Bytes()), false)
+ m.Require().NoError(err)
+ m.Require().Len(read, 2)
+ m.Require().NotNil(read[0].DataFile().FirstRowID())
+ m.EqualValues(firstRowID, *read[0].DataFile().FirstRowID())
+ m.Require().NotNil(read[1].DataFile().FirstRowID())
+ m.EqualValues(firstRowID+count, *read[1].DataFile().FirstRowID())
+
+ // Test 2: WriteManifest without option leaves FirstRowID nil (backward
compat).
+ var bufNoID bytes.Buffer
+ mf2, err := WriteManifest("/manifest.avro", &bufNoID, 3, partitionSpec,
testSchema, entrySnapshotID, entries)
Review Comment:
Once the design and the guards settle, the coverage will want to follow.
Right now the two cases cover the happy path and backward-compat, but the no-op
behaviors the doc promises are exactly what's untested — v1/v2 dropping the
value, and a delete manifest rejecting it. I'd also add an `AddManifests` case
with one pre-set and one nil-ID manifest asserting non-overlapping ranges,
since that's the scenario most likely to bite.
Smaller things while you're here: these read more naturally as two focused
subtests than comment-delimited blocks in one method (if Test 1's `Require`
aborts, Test 2 never runs), and the block mixes `m.Equal` and `m.EqualValues`
on the same `int64` — neighboring tests use `EqualValues` throughout.
##########
manifest.go:
##########
@@ -1274,6 +1274,16 @@ func WithManifestFileContent(content ManifestContent)
ManifestFileOption {
}
}
Review Comment:
Assuming the option survives the design question above — the doc says "v3+
data manifest" but the closure enforces neither the version nor the content. On
v1/v2 we set `FirstRowIDValue` in memory and then `toV1()` silently drops it at
encode time, so what you read back diverges from what's persisted, with no
error. And nothing stops `WithManifestFileContent(ManifestContentDeletes)` +
this producing a delete manifest with a non-null `first_row_id`, which the spec
says is always null (Java's list writer asserts `content() != DATA ||
firstRowId() != null` rather than allowing it).
`ManifestFileOption` is `func(mf *manifestFile)`, so it can't return an
error or see the version at apply time — the in-closure guard (`if mf.version <
3 { return }`) is really the only place for the version check. The delete case
is trickier because option order matters, so I'd validate that one in
`ToManifestFile` after all opts are applied rather than in the closure.
##########
manifest.go:
##########
@@ -1274,6 +1274,16 @@ func WithManifestFileContent(content ManifestContent)
ManifestFileOption {
}
}
+// WithManifestFileFirstRowID sets the first_row_id on a v3+ data manifest.
+// Silently no-ops on v1/v2 manifests since the field does not exist in those
formats.
+func WithManifestFileFirstRowID(firstRowID int64) ManifestFileOption {
Review Comment:
Independent of the API-shape question — this stores the address of the
by-value parameter, so applying the same option instance to two manifests
shares one `*int64`. Single-call is fine, but reuse aliases the storage, and
any downstream mutation through the pointer would leak across manifests.
Copy into the closure so each application gets its own allocation, the way
`AddManifests` does (`firstRowID := *m.nextRowID` before taking its address):
```go
return func(mf *manifestFile) {
v := firstRowID
mf.FirstRowIDValue = &v
}
```
--
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]
