zeroshade commented on code in PR #217:
URL: https://github.com/apache/arrow-go/pull/217#discussion_r1878342245
##########
parquet/metadata/metadata_test.go:
##########
@@ -114,7 +114,7 @@ func TestBuildAccess(t *testing.T) {
require.NoError(t, err)
serialized, err := faccessor.SerializeString(context.Background())
assert.NoError(t, err)
- faccessorCopy, err := metadata.NewFileMetaData([]byte(serialized), nil)
+ faccessorCopy, err := metadata.NewFileMetaData([]byte(serialized), 0,
nil)
Review Comment:
Shouldn't it be invalid to use 0 for the footer offset?
##########
parquet/metadata/file.go:
##########
@@ -275,6 +278,9 @@ func NewFileMetaData(data []byte, fileDecryptor
encryption.FileDecryptor) (*File
// Size is the length of the raw serialized metadata bytes in the footer
func (f *FileMetaData) Size() int { return f.metadataLen }
+// SourceSz is the total size of the source file
+func (f *FileMetaData) SourceSz() int64 { return f.footerOffset }
Review Comment:
Because the FileMetadata can potentially be separate from the file itself,
it might make more sense to name this differently or at least use a different
description for the comment.
##########
parquet/metadata/file.go:
##########
@@ -243,11 +243,13 @@ type FileMetaData struct {
// size of the raw bytes of the metadata in the file which were
// decoded by thrift, Size() getter returns the value.
metadataLen int
+
+ footerOffset int64
}
// NewFileMetaData takes in the raw bytes of the serialized metadata to
deserialize
// and will attempt to decrypt the footer if a decryptor is provided.
-func NewFileMetaData(data []byte, fileDecryptor encryption.FileDecryptor)
(*FileMetaData, error) {
+func NewFileMetaData(data []byte, footerOffset int64, fileDecryptor
encryption.FileDecryptor) (*FileMetaData, error) {
Review Comment:
The only thing I'm not a fan of here is that this is a breaking change. Now,
I don't think there are many (if any) packages that are importing and using
this function directly (as creating new file metadata would primarily be done
via the Writer rather than done directly) but it's still a concern that I'd
rather not create a breaking change if we can avoid it.
Is there a way we can avoid this being a breaking change at all? If 0 is
valid, maybe we create a setter?
--
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]
