[fallintoplace]

What

• Remove DataFileBuilder.BlockSizeInBytes.

Why

• The field is deprecated by the Iceberg spec.

Implementation

• Remove the setter and keep the legacy field for reading v1 manifests.

Refs #2062.

[badalprasadsingh]

Close to approval, but there are 2 things which definitely needs a look at.

[badalprasadsingh]

Please add it back.

[badalprasadsingh]

Don't the builders leave this at 0, causing v1 manifests to store 0? If so, this comment is incorrect.

As far as I checked last, Java writes 64 MiB, and our v1 schema declares the same default. Could you add omitzero to the tag so v1 writes that default? Also, TestManifestEntryProjectionWhitelistCoversDataFileSchema reads the raw tag, so it needs strings.Cut(name, ",").

BlockSizeInBytes int64 `avro:"block_size_in_bytes,omitzero"`

[badalprasadsingh]

nit: Please move the doc comment above dataFile onto BlockSizeInBytes field, check how DistinctCounts is written.

[badalprasadsingh]

nit: This line checks a field in memory, not something that gets written to the manifest. Same check running in all 5 cases, and calls Build() before the builder is done.

better to remove it.

If you take the omitzero suggestion, test this instead: write a v1 file without setting the field, then check that it reads back as 64mib.

[zeroshade]

+1 to @badalprasadsingh's review. I checked both main points with a throwaway test. Building a data file without the setter and round-tripping a v1 manifest reads back block_size_in_bytes = 0, although the v1 schema says "Always write default in v1" with a 64 MiB default (internal/avro_schemas.go:251). Since nothing in the repo calls the setter, that's already what every v1 manifest we write contains, and removing the setter takes away the only workaround. With the tag changed to avro:"block_size_in_bytes,omitzero", the same round trip reads back 67108864, and TestManifestEntryProjectionWhitelistCoversDataFileSchema fails until it strips the tag options with strings.Cut(name, ","), as predicted. So please take the omitzero change, fix the comment, swap the in-memory assert.Zero check for the write-v1-then-read-64MiB test, and restore the compatibility note from b7bbe05.

[zeroshade]

Thanks, this takes all of @badalprasadsingh's points: omitzero makes unset v1 block sizes write the 64 MiB schema default, the doc moved onto the field, the whitelist test strips tag options, and the round-trip test pins the default (it fails if omitzero is removed). I pushed a merge of main (bd1e323) that keeps both releases.md notes. go vet ./... (including -tags integration) and go test . pass. LGTM. @badalprasadsingh, could you take another look?

[laskoviymishka]

LGTM

[zeroshade]

Thanks, this takes all of @badalprasadsingh's points: omitzero makes unset v1 block sizes write the 64 MiB schema default, the doc moved onto the field, the whitelist test strips tag options, and the round-trip test pins the default (it fails if omitzero is removed). I pushed a merge of main (f6b6d31) that keeps both releases.md notes. go vet ./... (including -tags integration) and go test . pass. LGTM.

[badalprasadsingh]

lgtm :) all my points are addressed, thanks.