zeroshade commented on PR #2325: URL: https://github.com/apache/iceberg-go/pull/2325#issuecomment-6425067759
+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`. -- 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]
