fix(manifest): normalize file_format casing when reading manifest entries

[wroever]

Closes #1983, which has the full write-up and reproduction.

Problem

The Avro manifest decoders assign the wire file_format string directly into the typed FileFormat field, whose constants are uppercase:

// manifest.go
Format FileFormat `avro:"file_format"`

GetFile (table/internal/interfaces.go) then matches the decoded value against iceberg.ParquetFile ("PARQUET") to pick a reader. A manifest written with lowercase parquet never matches, so the scan fails on the first data file — with a message that renders both spellings identically:

not implemented: only parquet format is implemented, got parquet

Two decode paths are affected: ManifestReader.ReadEntry, and unmarshalAvroDataFileEntry (data_file_codec.go), which is reachable publicly through codec.DecodeDataFile. Beyond GetFile, the un-normalized value also mis-compares at table/scan_splits.go, table/scanner.go (IsDeletionVector), and table/dv/deletion_vector.go.

Nothing unusual is needed to hit this: LoadTable followed by a bare tbl.Scan(). DuckDB's iceberg extension writes lowercase, so no table it has written is readable. It also isn't limited to explicit scans — Transaction.Delete runs a copy-on-write rewrite through the same path, so appends and deletes against such a table fail too.

Fix

Factor the normalization into dataFile.normalizeFormat, which routes the decoded value through the existing, already case-insensitive FileFormatFromString, and call it from both decode paths — ManifestReader.ReadEntry, before the existing status/content validation and covering the normal and fallback entry paths (they share the same *dataFile), and unmarshalAvroDataFileEntry.

This mirrors what the other implementations do on this exact path — Java's BaseFile.internalSet → FileFormat.fromString, and PyIceberg's FileFormat._missing_ — so it brings Go in line rather than introducing new leniency.

It also makes the manifest reader consistent with the rest of this library, which already normalizes everywhere else this field crosses the wire:

• catalog/rest/scan_task_decoder.go — the REST scan-planning decoder calls FileFormatFromString before building the data file.
• table/writer.go and table/rolling_data_writer.go — both writers normalize.

The Avro manifest reader was the only place treating wire casing as authoritative.

Correction: this was wrong, as @zeroshade found in review. unmarshalAvroDataFileEntry (data_file_codec.go), the other manifest-entry Avro decoder and the one behind the public codec.DecodeDataFile, had the identical bug.

Behavior changes

Two user-visible consequences of normalizing on read, both flagged by @zeroshade in review:

1. Metadata tables report the canonical spelling. table/inspect_files.go appends string(file.FileFormat()) directly, so files, data_files, delete_files and the all_* variants now show PARQUET for a manifest written with lowercase parquet.
2. Rewriting a manifest persists the normalized spelling. Reading a DuckDB-written table and running a delete writes PARQUET back to disk. Normalization is idempotent, so this is a one-time upgrade and not manifest churn.

Note for reviewers

An unrecognized file_format is now a hard error at decode time, naming the offending file path, rather than surfacing later as a confusing reader-selection failure. This matches Java, where FileFormat.fromString throws. The lenient alternative — pass unknown values through untouched and let file-open fail — is a one-line change if reviewers prefer it.

The blast radius of that hard error is wider than scan. Because it fires at decode time, it also reaches paths that never open the data file — inspect_files, expire-snapshots and orphan cleanup — and it aborts the entire manifest read even for entries iterManifest(discardDeleted=true) would have discarded. That is unreachable with any format this library writes, and Java fails identically, but it is worth stating explicitly.

Testing

TestManifestReaderNormalizesFileFormat writes a manifest through the Avro writer and reads it back, asserting the canonical constant comes out: lowercase/mixed/canonical parquet, lowercase avro and orc, puffin in a delete manifest, and the v1 fallback entry path; unrecognized csv and an empty file_format error.

TestUnmarshalAvroDataFileEntryNormalizesFileFormat is the equivalent table for the codec decoder, round-tripping through MarshalAvroEntry → unmarshalAvroDataFileEntry.

make test and make lint are clean.

[zeroshade]

Thanks for this — the fix is correct, minimal, and genuinely spec-justified. I verified it rather than taking the description's word for it, and the substance holds up. One real gap and a couple of description corrections before merge.

What I verified

• Reverting only the 7-line manifest.go hunk makes 4 of the 5 new subcases fail (spec_lowercase got "parquet", mixed_case got "Parquet", lowercase_avro got "avro", unknown_format "An error is expected but got nil"). It is a real regression test, not a tautology.
• Spec grounding: format/spec.md names the values in lowercase (avro, orc, parquet, puffin) while Java writes uppercase via FileFormat.name(). Case-insensitive read is the only portable behaviour, so the framing in #1983 is right and this isn't a DuckDB quirk.
• Java parity by reading the source, not assuming: FileFormat.fromString is valueOf(toUpperCase(Locale.ROOT)) and throws on unknown, and BaseFile calls it. Both the normalization and the new hard error match.
• Probes: the v1 fallback decode path normalizes; lowercase orc → ORC; lowercase puffin in a delete manifest → PUFFIN; and a round-trip through ManifestWriter.Existing followed by a raw non-normalizing decode shows "PARQUET" on disk — normalization is idempotent and doesn't churn manifests.
• Perf is a non-issue: FileFormatFromString("PARQUET") is 10–19 ns/op with 0 allocs (strings.ToUpper returns all-uppercase ASCII unchanged). A 5000-entry ReadEntry loop measures 295,588 allocs/op both with and without the fix — identical.
• go test . -count=1 → 1396 subtests pass; -race clean; golangci-lint run ./... → 0 issues; CI 15/15.

Major — the sibling decoder has the identical bug

data_file_codec.go:132-150 — unmarshalAvroDataFileEntry, the other manifest-entry Avro decoder (public entry point codec.DecodeDataFile), still treats wire casing as authoritative. I built a dataFile with Format = "parquet", ran MarshalAvroEntry then unmarshalAvroDataFileEntry, and got file_format = "parquet" back. Every downstream comparison then mis-fires exactly as #1983 describes — table/internal/interfaces.go:38 (GetFile), table/scan_splits.go:34, table/scanner.go:433 (IsDeletionVector), table/dv/deletion_vector.go:331.

This matters because the codec package doc frames raw manifest bytes as in-contract input: "The bytes it produces are the same Avro bytes a manifest carries for the corresponding value" and "Legacy manifests that already carry the field on the wire still decode correctly through DecodeDataFile."

Fix: the same four lines after the s.Decode at data_file_codec.go:141, or better, factor the normalization into one helper both ReadEntry and unmarshalAvroDataFileEntry call — the shared newDecodeEntry/*dataFile plumbing already exists.

This also makes the description's claim "The Avro manifest reader was the only place treating wire casing as authoritative" false, which is worth correcting either way.

Description omissions

Two real, user-visible behaviour changes aren't mentioned. Both are improvements — I'm asking for disclosure, not changes:

1. table/inspect_files.go:665 does b.fileFormat.Append(string(file.FileFormat())), so the files / data_files / delete_files / all_* metadata tables now report PARQUET for a lowercase-written manifest.
2. Rewriting a manifest persists the normalized spelling, so reading a DuckDB-written table and running a delete silently upgrades the casing on disk.

Minor

• manifest.go:920-926 — an unrecognized format now aborts the entire manifest read even for entries iterManifest(discardDeleted=true) would discard (probe: vortex on a DELETED entry → 0 entries, hard error; previously it decoded and was skipped). Unreachable today and Java hard-fails identically, so I'm not asking for the lenient variant — but the blast radius is wider than scan (inspect_files, expire-snapshots, orphan cleanup never open the data file yet would now fail). Worth a sentence.
• manifest.go:923 uses df.Path where the adjacent first-row-id block at :960 uses df.FilePath(). Pick one.
• Pre-existing asymmetry this PR makes more visible: the reader is now lenient while NewDataFileBuilder (manifest.go:3082) and table/transaction.go:1099 stay exact-case — NewDataFileBuilder(..., FileFormat("parquet"), ...) still errors. Routing the builder through FileFormatFromString would be a coherent follow-up.
• Test coverage gaps (all four verified working by probe, so coverage not correctness): the v1 fallback path, orc, puffin-in-a-delete-manifest, and empty file_format. The v1 case is the one I'd add since the description makes an explicit claim about it — swapping in internal.NewManifestEntrySchema(partSchema, 1) and encoding a fallbackManifestEntry is about 3 lines in the existing table.

Not blocking on any of the above except that I'd like the data_file_codec.go decoder either fixed here or explicitly deferred to a follow-up issue, since it's the same bug the PR exists to fix.

---

This review was drafted by an AI-assisted tool and confirmed by an Iceberg Go maintainer. The findings cite the project's review criteria; if you think one is mis-applied, please reply and a maintainer will weigh in.

[wroever]

@zeroshade Thanks for the review. Have addressed the previously-unhandled codec path in 9a12014, happy to make further edits as needed.

[zeroshade]

The prior major (unnormalized sibling decoder in unmarshalAvroDataFileEntry) is genuinely fixed and pinned by a mutation-verified test, all four requested coverage gaps are now real non-vacuous subcases, and no un-normalized Avro decode path remains.

Re-review verification: 3 of 7 prior findings confirmed fixed at 9a12014 (each verified by mutating the fix and observing the suite go red, not by taking the claim on trust). Still open:

• not fixed — description omission: table/inspect_files.go:665 means the files/data_files/delete_files/all_* metadata tables now report PARQUET for a lowercase-written manifest
• not fixed — description omission: rewriting a manifest persists the normalized spelling, silently upgrading casing on disk
• partially fixed — minor: an unrecognized format now aborts the entire manifest read even for entries iterManifest(discardDeleted=true) would discard; blast radius wider than scan — worth a sentence

Verification performed

go build ./... (clean); go vet . ./codec (clean); go test . ./codec -count=1 (ok 0.481s / 0.436s); go test ./table/... -count=1 (all 5 packages ok); go test . -race -count=1 -run 'TestManifests|TestUnmarshal|TestDataFileCodec|TestEncodeDecode' (ok, race-clean); gofmt -l on all 4 changed files (clean). Mutation runs: removed manifest.go:917-922 -> 8/9 subcases FAIL; removed data_file_codec.go:143-145 -> 4/5 subcases FAIL. Throwaway probe pr1984_probe_test.go (isFallback assertion + DELETED-entry hard-error) written, run, and deleted. Local golangci-lint v2.12.2 panics under Go 1.26 (goanalysis loader crash, environment issue) — deferred to the green CI lint job.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.

More on how Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.