badalprasadsingh commented on code in PR #2325:
URL: https://github.com/apache/iceberg-go/pull/2325#discussion_r4377225600
##########
website/src/releases.md:
##########
@@ -42,6 +42,8 @@ Per-release notes (highlights, breaking changes,
contributors) are published on
### Unreleased compatibility notes
+- **Deprecated data-file block-size setter removed.**
`DataFileBuilder.BlockSizeInBytes` is no longer available. Reading the legacy
block-size field from v1 manifests is unchanged.
Review Comment:
Please add it back.
##########
manifest.go:
##########
Review Comment:
nit: Please move the doc comment above `dataFile` onto `BlockSizeInBytes`
field, check how `DistinctCounts` is written.
##########
manifest.go:
##########
@@ -2598,6 +2598,8 @@ func fitDecimalBytes(bytes []byte, size int) ([]byte,
error) {
return retained, nil
}
+// dataFile retains deprecated block_size_in_bytes for reading legacy v1
+// manifests. New builders leave it at the required default; v2/v3 omit it.
Review Comment:
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, ",")`.
```go
BlockSizeInBytes int64 `avro:"block_size_in_bytes,omitzero"`
```
##########
manifest_projection_test.go:
##########
@@ -213,8 +213,13 @@ func
TestManifestEntryProjectionSupportsManifestVersionsAndDeletes(t *testing.T)
128,
)
require.NoError(t, err)
+ assert.Zero(t,
builder.Build().(*dataFile).BlockSizeInBytes)
Review Comment:
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.
--
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]
