zeroshade commented on code in PR #560:
URL: https://github.com/apache/iceberg-go/pull/560#discussion_r2353571031
##########
manifest.go:
##########
@@ -1349,6 +1400,12 @@ func WriteManifestList(version int, out io.Writer,
snapshotID int64, parentSnaps
return errors.New("sequence number is required for V2
tables")
}
writer, err = NewManifestListWriterV2(out, snapshotID,
*sequenceNumber, parentSnapshotID)
+ case 3:
+ if sequenceNumber == nil {
+ return errors.New("sequence number is required for V3
tables")
+ }
+ // TODO
+ writer, err = NewManifestListWriterV3()
Review Comment:
return a not-implemented-error for now
##########
manifest.go:
##########
@@ -1246,6 +1258,11 @@ func NewManifestListWriterV2(out io.Writer, snapshotID,
sequenceNumber int64, pa
})
}
+func NewManifestListWriterV3() (*ManifestListWriter, error) {
+ // TODO: Implement v3 writer
+ return nil, errors.New("not implemented manifest list writer for v3
yet")
Review Comment:
use fmt.Errorf("%w: manifest list writer for v3", iceberg.ErrNotImplemented)
##########
manifest.go:
##########
@@ -1327,6 +1344,40 @@ func (m *ManifestListWriter) AddManifests(files
[]ManifestFile) error {
return err
}
}
+ case 3:
+ for _, file := range files {
+ if file.Version() != 3 {
+ return fmt.Errorf("%w: ManifestListWriter only
supports version 3 manifest files", ErrInvalidArgument)
+ }
+ wrapped := *(file.(*manifestFile))
+
+ // Ref:
https://github.com/apache/iceberg/blob/ea2071568dc66148b483a82eefedcd2992b435f7/core/src/main/java/org/apache/iceberg/ManifestListWriter.java#L157-L168
+ if wrapped.Content == ManifestContentData &&
wrapped.FirstRowId == nil {
+ if m.nextRowID != nil {
+ wrapped.FirstRowId = m.nextRowID
+ *m.nextRowID +=
wrapped.ExistingRowsCount + wrapped.AddedRowsCount
+ }
+ }
+
+ // Ref:
https://github.com/apache/iceberg/blob/ea2071568dc66148b483a82eefedcd2992b435f7/core/src/main/java/org/apache/iceberg/V3Metadata.java#L98-L122
+ if wrapped.SeqNumber == -1 {
+ if m.commitSnapshotID !=
wrapped.AddedSnapshotID {
+ return fmt.Errorf("found unassigned
sequence number for a manifest from snapshot %d != %d",
+ m.commitSnapshotID,
wrapped.AddedSnapshotID)
+ }
+ wrapped.SeqNumber = m.sequenceNumber
+ }
+
+ if wrapped.MinSeqNumber == -1 {
+ if m.commitSnapshotID !=
wrapped.AddedSnapshotID {
+ return fmt.Errorf("found unassigned
sequence number for a manifest from snapshot: %d", wrapped.AddedSnapshotID)
+ }
+ wrapped.MinSeqNumber = m.sequenceNumber
+ }
+ if err := m.writer.Encode(wrapped); err != nil {
+ return err
+ }
Review Comment:
since this logic is identical between v2 and v3, is there any way we can
consolidate the logic and reduce the duplication?
--
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]
