zeroshade commented on code in PR #890:
URL: https://github.com/apache/iceberg-go/pull/890#discussion_r3089673911
##########
manifest.go:
##########
@@ -316,14 +243,14 @@ type manifestFile struct {
SeqNumber int64 `avro:"sequence_number"`
MinSeqNumber int64 `avro:"min_sequence_number"`
AddedSnapshotID int64 `avro:"added_snapshot_id"`
- AddedFilesCount int32 `avro:"added_files_count"`
- ExistingFilesCount int32 `avro:"existing_files_count"`
- DeletedFilesCount int32 `avro:"deleted_files_count"`
+ AddedFilesCount int32
`avro:"added_files_count,alias=added_data_files_count"`
+ ExistingFilesCount int32
`avro:"existing_files_count,alias=existing_data_files_count"`
+ DeletedFilesCount int32
`avro:"deleted_files_count,alias=deleted_data_files_count"`
AddedRowsCount int64 `avro:"added_rows_count"`
ExistingRowsCount int64 `avro:"existing_rows_count"`
DeletedRowsCount int64 `avro:"deleted_rows_count"`
- PartitionList *[]FieldSummary `avro:"partitions"`
- Key []byte `avro:"key_metadata"`
+ PartitionList *[]FieldSummary `avro:"partitions,type-alias=r508"`
+ Key *[]byte `avro:"key_metadata"`
Review Comment:
why this change?
##########
manifest.go:
##########
@@ -384,15 +311,30 @@ func (m *manifestFile) Length() int64
{ return m.Len }
func (m *manifestFile) PartitionSpecID() int32 { return m.SpecID }
func (m *manifestFile) ManifestContent() ManifestContent { return m.Content }
func (m *manifestFile) SnapshotID() int64 { return
m.AddedSnapshotID }
-func (m *manifestFile) AddedDataFiles() int32 { return
m.AddedFilesCount }
-func (m *manifestFile) ExistingDataFiles() int32 { return
m.ExistingFilesCount }
-func (m *manifestFile) DeletedDataFiles() int32 { return
m.DeletedFilesCount }
-func (m *manifestFile) AddedRows() int64 { return
m.AddedRowsCount }
-func (m *manifestFile) ExistingRows() int64 { return
m.ExistingRowsCount }
-func (m *manifestFile) DeletedRows() int64 { return
m.DeletedRowsCount }
-func (m *manifestFile) SequenceNum() int64 { return m.SeqNumber }
-func (m *manifestFile) MinSequenceNum() int64 { return
m.MinSeqNumber }
-func (m *manifestFile) KeyMetadata() []byte { return m.Key }
+func (m *manifestFile) AddedDataFiles() int32 {
+ return m.AddedFilesCount
+}
+
+func (m *manifestFile) ExistingDataFiles() int32 {
+ return m.ExistingFilesCount
+}
+
+func (m *manifestFile) DeletedDataFiles() int32 {
+ return m.DeletedFilesCount
+}
Review Comment:
why change this formatting?
##########
manifest.go:
##########
@@ -401,8 +343,14 @@ func (m *manifestFile) Partitions() []FieldSummary {
return *m.PartitionList
}
-func (m *manifestFile) HasAddedFiles() bool { return m.AddedFilesCount != 0
}
-func (m *manifestFile) HasExistingFiles() bool { return m.ExistingFilesCount
!= 0 }
+func (m *manifestFile) HasAddedFiles() bool {
+ return m.AddedFilesCount != 0
+}
+
+func (m *manifestFile) HasExistingFiles() bool {
+ return m.ExistingFilesCount != 0
+}
+
Review Comment:
why change this formatting?
##########
manifest.go:
##########
@@ -158,154 +158,81 @@ func (b *ManifestBuilder) Build() ManifestFile {
return b.m
}
-type fallbackManifestFileV1 struct {
- manifestFileV1
- AddedSnapshotID *int64 `avro:"added_snapshot_id"`
-}
-
-func (f *fallbackManifestFileV1) toFile() *manifestFile {
- if f.AddedSnapshotID == nil {
- f.manifestFileV1.AddedSnapshotID = -1
- }
-
- return f.manifestFileV1.toFile()
-}
-
+// manifestFileV1 is a read/write struct for V1 manifest list entries.
+// Pointer fields handle nullable Avro types (["null", T] unions).
+// The three count fields carry aliases for pre-1.4 Java Iceberg legacy names
+// (apache/iceberg#5338) so that avro.Resolve maps them transparently.
type manifestFileV1 struct {
- manifestFile
- AddedFilesCount *int32 `avro:"added_files_count"`
- ExistingFilesCount *int32 `avro:"existing_files_count"`
- DeletedFilesCount *int32 `avro:"deleted_files_count"`
- AddedRowsCount *int64 `avro:"added_rows_count"`
- ExistingRowsCount *int64 `avro:"existing_rows_count"`
- DeletedRowsCount *int64 `avro:"deleted_rows_count"`
+ Path string `avro:"manifest_path"`
+ Len int64 `avro:"manifest_length"`
+ SpecID int32 `avro:"partition_spec_id"`
+ AddedSnapshotID *int64 `avro:"added_snapshot_id"`
Review Comment:
Isn't `AddedSnapshotId` required in v1? Will this break if the avro schema
has it as required vs optional? Previously we relied on embedding the
`manifestFile` version that had it as required and then fell back if it was
optional. But we don't want this to break on v1 manifests that have it as
required.
##########
manifest.go:
##########
@@ -811,43 +753,46 @@ func ReadManifest(m ManifestFile, f io.Reader,
discardDeleted bool) ([]ManifestE
// "format-version" metadata key (only manifest files are). When the key is
// absent, version 1 is assumed.
func ReadManifestList(in io.Reader) ([]ManifestFile, error) {
- rd, err := ocf.NewReader(in)
- if err != nil {
- return nil, err
- }
+ var version int
+
+ rd, err := ocf.NewReader(in, ocf.WithReaderSchemaFunc(func(rd
*ocf.Reader) (*avro.Schema, error) {
+ version = 1
+ if raw := rd.Metadata()["format-version"]; len(raw) > 0 {
+ v, err := strconv.Atoi(string(raw))
+ if err != nil {
+ return nil, fmt.Errorf("invalid format-version:
%w", err)
+ }
- sc, err := avro.Parse(string(rd.Metadata()["avro.schema"]))
- if err != nil {
- return nil, err
- }
+ version = v
+ }
- version := 1
- if raw := rd.Metadata()["format-version"]; len(raw) > 0 {
- version, err = strconv.Atoi(string(raw))
- if err != nil {
- return nil, fmt.Errorf("invalid format-version: %w",
err)
+ if version == 1 {
+ return manifestFileV1Reader, nil
}
+
+ return manifestFileReader, nil
+ }))
+ if err != nil {
+ return nil, err
}
if version == 1 {
- root := sc.Root()
- for _, f := range root.Fields {
- if f.Name == "added_snapshot_id" {
- if f.Type.Type == "union" {
- return
decodeManifestsWithFallback[*fallbackManifestFileV1](rd)
+ results := make([]ManifestFile, 0)
+ for {
+ tmp := new(manifestFileV1)
+ if err := rd.Decode(tmp); err != nil {
+ if errors.Is(err, io.EOF) {
+ return results, nil
}
- break
+ return nil, err
}
+
+ results = append(results, tmp.toFile())
Review Comment:
Isn't this just `decodeManifestsWithFallback[*manifestFileV1](rd)`? Can't we
just leave the switch we had below?
##########
manifest.go:
##########
@@ -811,43 +753,46 @@ func ReadManifest(m ManifestFile, f io.Reader,
discardDeleted bool) ([]ManifestE
// "format-version" metadata key (only manifest files are). When the key is
// absent, version 1 is assumed.
func ReadManifestList(in io.Reader) ([]ManifestFile, error) {
- rd, err := ocf.NewReader(in)
- if err != nil {
- return nil, err
- }
+ var version int
+
+ rd, err := ocf.NewReader(in, ocf.WithReaderSchemaFunc(func(rd
*ocf.Reader) (*avro.Schema, error) {
+ version = 1
+ if raw := rd.Metadata()["format-version"]; len(raw) > 0 {
+ v, err := strconv.Atoi(string(raw))
+ if err != nil {
+ return nil, fmt.Errorf("invalid format-version:
%w", err)
+ }
- sc, err := avro.Parse(string(rd.Metadata()["avro.schema"]))
- if err != nil {
- return nil, err
- }
+ version = v
+ }
- version := 1
- if raw := rd.Metadata()["format-version"]; len(raw) > 0 {
- version, err = strconv.Atoi(string(raw))
- if err != nil {
- return nil, fmt.Errorf("invalid format-version: %w",
err)
+ if version == 1 {
+ return manifestFileV1Reader, nil
}
+
+ return manifestFileReader, nil
Review Comment:
are we going to have to do this for every version?
--
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]
