zeroshade commented on code in PR #1137:
URL: https://github.com/apache/arrow-go/pull/1137#discussion_r3752613003
##########
parquet/metadata/column_chunk.go:
##########
@@ -87,9 +87,10 @@ type ColumnChunkMetaData struct {
// this is primarily used internally or between the subpackages.
ColumnChunkMetaDataBuilder should
// be used by consumers instead of using this directly.
func NewColumnChunkMetaData(column *format.ColumnChunk, descr *schema.Column,
writerVersion *AppVersion, rowGroupOrdinal, columnOrdinal int16, fileDecryptor
encryption.FileDecryptor) (*ColumnChunkMetaData, error) {
+ columnMeta := column.GetMetaData()
Review Comment:
Cosmetic: this local doesn't earn its keep. It's used once, on line 93, and
the new check on line 121 reads `c.columnMeta` rather than this variable — so
it never diverges from the field, but a reader still has to track it to confirm
that.
Inlining it back to `columnMeta: column.GetMetaData()` in the struct literal
would leave the diff as purely the nil check.
My guess is it's a leftover from a draft where the check sat before the
crypto block — which is exactly the placement that would have broken encrypted
chunks, so the final version is right.
##########
parquet/metadata/file_internal_test.go:
##########
@@ -36,3 +36,8 @@ func TestNewFileMetaDataReturnsSchemaErrors(t *testing.T) {
require.Error(t, err)
require.Nil(t, meta)
}
+
+func TestNewColumnChunkMetaDataReturnsErrorForMissingMetadata(t *testing.T) {
+ _, err := NewColumnChunkMetaData(&format.ColumnChunk{}, nil, nil, 0, 0,
nil)
Review Comment:
Cosmetic: the sibling test just above
(`TestNewFileMetaDataReturnsSchemaErrors`) asserts `require.Nil(t, meta)`
alongside the error, while this one discards the first return with `_`.
Matching it would confirm no partially-constructed `ColumnChunkMetaData`
escapes next to the error — which is the more interesting half of the
guarantee, since the struct is fully populated by line 121 before the check
rejects it.
`require.EqualError` is fine and does pin the message; just worth noting
it'll need updating if a prefix is ever added, where the sibling's
`require.Error` wouldn't.
--
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]
