laskoviymishka commented on code in PR #1984:
URL: https://github.com/apache/iceberg-go/pull/1984#discussion_r3966641561
##########
manifest.go:
##########
@@ -917,6 +917,11 @@ func (c *ManifestReader) ReadEntry() (ManifestEntry,
error) {
if c.isFallback {
tmp = tmp.(*fallbackManifestEntry).toEntry()
}
+ if df, ok := tmp.DataFile().(*dataFile); ok {
+ if err := df.normalizeFormat(); err != nil {
+ return nil, err
Review Comment:
the sibling decode path in `unmarshalAvroDataFileEntry` wraps this same
error with `iceberg: unmarshalAvroDataFileEntry: %w`, but here we return it
bare, so a failure surfacing from `ReadManifest`/`Entries` carries the file
path with no call-site attribution.
I'd match it: `return nil, fmt.Errorf("iceberg: ReadEntry: %w", err)`,
consistent with the status and content-type checks just below that already
prefix their errors.
##########
manifest.go:
##########
@@ -917,6 +917,11 @@ func (c *ManifestReader) ReadEntry() (ManifestEntry,
error) {
if c.isFallback {
tmp = tmp.(*fallbackManifestEntry).toEntry()
}
+ if df, ok := tmp.DataFile().(*dataFile); ok {
Review Comment:
the `ok` guard here silently skips normalization if `DataFile()` ever
returns something other than `*dataFile`. It's dead today (both the v2/v3 path
and the v1 fallback populate `&dataFile{}`), but the invariant is load-bearing:
a future entry variant that slipped through un-normalized would quietly
recreate the original lowercase-comparison bug with no test failure.
Since `ReadEntry` owns the allocation, I'd either assert unconditionally, or
keep the guard and return an error in the else. wdyt?
##########
data_file_codec_test.go:
##########
@@ -93,6 +93,47 @@ func TestDataFileCodecWithDroppedPartitionSource(t
*testing.T) {
require.Equal(t, map[int]any{1000: nil, 1001: int32(3)},
decoded.Partition())
}
+func TestUnmarshalAvroDataFileEntryNormalizesFileFormat(t *testing.T) {
+ spec := NewPartitionSpec()
+ schema := NewSchema(0)
+
+ tests := []struct {
+ name string
+ written FileFormat
+ expected FileFormat
+ errorContains string
+ }{
+ {name: "spec lowercase", written: "parquet", expected:
ParquetFile},
+ {name: "mixed case", written: "Parquet", expected: ParquetFile},
+ {name: "uppercase", written: ParquetFile, expected:
ParquetFile},
+ {name: "lowercase orc", written: "orc", expected: OrcFile},
+ {name: "unknown format", written: "csv", errorContains:
"unknown file format: csv"},
+ {name: "empty format", written: "", errorContains: "unknown
file format: "},
Review Comment:
the manifest-reader test covers all four formats but this codec table only
hits parquet variants and orc. Since the codec path is independently callable,
I'd round it out with lowercase avro and puffin so the two tests stay symmetric.
(puffin needs `source.Content = EntryContentPosDeletes` before encoding to
get past the builder validation.)
--
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]
