-
Notifications
You must be signed in to change notification settings - Fork 229
fix(manifest): normalize file_format casing when reading manifest entries #1984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,6 +93,46 @@ 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"}, | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit — empty-format row asserts a prefix common to every unknown-format error errorContains "unknown file format: " is a substring of "unknown file format: csv" and of every other unknown-format message, so the row cannot distinguish the empty value from any other rejected spelling. It still verifies that an empty file_format is not silently accepted, which is the point of the row, so this is purely about assertion precision. Same applies to manifest_test.go:2456. |
||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| builder, err := NewDataFileBuilder(spec, EntryContentData, | ||
| "s3://bucket/table/data.parquet", ParquetFile, nil, nil, nil, 1, 1024) | ||
| require.NoError(t, err) | ||
| source := builder.Build().(*dataFile) | ||
| source.Format = tt.written | ||
|
|
||
| encoded, err := source.MarshalAvroEntry(spec, schema, 2) | ||
| require.NoError(t, err) | ||
|
|
||
| decoded, err := unmarshalAvroDataFileEntry(encoded, spec, schema, 2) | ||
| if tt.errorContains != "" { | ||
| require.ErrorContains(t, err, tt.errorContains) | ||
|
|
||
| return | ||
| } | ||
| require.NoError(t, err) | ||
| require.Equal(t, tt.expected, decoded.FileFormat()) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TestManifestEntrySchemaForDistinguishesSameSpecIDDifferentPartitionTypes | ||
| // guards against the cache-key collision that would occur if the cache | ||
| // were keyed only by spec.ID(). Two tables can both use | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| } | ||
| } | ||
| switch tmp.Status() { | ||
| case EntryStatusEXISTING, EntryStatusADDED, EntryStatusDELETED: | ||
| default: | ||
|
|
@@ -2771,6 +2776,20 @@ func (d *dataFile) setFieldIDToDecimalScaleMap(m map[int]int) { | |
| d.fieldIDToDecimalScale = m | ||
| } | ||
|
|
||
| // normalizeFormat sets d.Format to the FileFormat constant matching its | ||
| // decoded spelling, or returns an error if it names no known format. | ||
| // Every Avro decode path must call it before the value is compared or | ||
| // exposed. | ||
| func (d *dataFile) normalizeFormat() error { | ||
| format, err := FileFormatFromString(string(d.Format)) | ||
| if err != nil { | ||
| return fmt.Errorf("data file %q has invalid file format: %w", d.FilePath(), err) | ||
| } | ||
|
wroever marked this conversation as resolved.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit — Corrected error message is pinned by no assertion The delta's whole payload is the wording of this format string, but no test asserts it. Reintroducing the redundant 'has invalid file format:' prefix keeps the suite green, so the improvement can silently regress. Tightening one row per table to assert the full 'data file "...": unknown file format: csv' would lock it in. Cosmetic-only, so not worth a round-trip on its own. |
||
| d.Format = format | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (d *dataFile) ContentType() ManifestEntryContent { return d.Content } | ||
| func (d *dataFile) FilePath() string { return d.Path } | ||
| func (d *dataFile) FileFormat() FileFormat { return d.Format } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.