the-onewho-knocks commented on code in PR #1321:
URL: https://github.com/apache/iceberg-go/pull/1321#discussion_r3491551226
##########
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:
Will rework the tests once WriteManifestV3 lands splitting into focused
subtests (so an early Require failure doesn't silently skip later cases),
adding the delete-manifest rejection case, and an AddManifests case with one
pre-set and one nil-ID manifest to assert non-overlapping ranges. Will also
switch to EqualValues consistently for the int64 comparisons. Thanks for
flagging the test-isolation issue too clearly worth fixing regardless of the
API shape.
--
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]
