ARROW-13646: [Go][Parquet] adding the parquet metadata package #10951
Conversation
| return nil | ||
| } | ||
|
|
||
| // WriteTo will always return 0 as the int64 since the thrift writer library |
There was a problem hiding this comment.
Hmm, I wonder if we should keep track of the bytes or explicit return a -1. Is there precedent for this elsewhere?
There was a problem hiding this comment.
So, in internal/utils/write_utils.go I do have what i called a TellWrapper which wrapped an io.Writer in order to keep track of the bytes written so that it could add a Tell() function to tell the current position which I use elsewhere.
The main reason why I didn't do something like that here was that I didn't want every call to WriteTo to create an object wrapping the io.Writer in order to count the bytes written. It's just unfortunate that the current thrift interface for a TStruct has a Write function but doesn't return the number of bytes it wrote.
I don't remember there being anywhere else that I do this so you might be right that it makes sense to explicitly return a -1 here instead of a 0. Personally as long as the comment documents what is being returned, I don't much mind whether it's a 0 or a -1 as long as we're explicit about it.
|
Bump @emkornfield when you get a chance. Thanks! Also, @nickpoorman I've implemented the suggested comment, removed the old comment, and replied to your question when you have a chance. I'm curious your thoughts on my response. Thanks! |
|
Bump @emkornfield @nickpoorman Just trying to get eyes on this so i can get it merged and finally get the File package up here :) |
|
Sorry for the delay this is on my todo list for this week. |
| // LessThan compares the app versions and returns true if this version | ||
| // is "less than" the passed version. | ||
| // | ||
| // If the apps don't match, this always returns false. Otherwise it compares |
There was a problem hiding this comment.
should this be ternary? the contract seems strange?
There was a problem hiding this comment.
It's effectively a < operator, not a <=.
What seems strange about the contract?
There was a problem hiding this comment.
the comparison of apps, I would think we would want to sort those also just so ordering is well defined in all scenarios.
There was a problem hiding this comment.
i think the idea was to make it easier to compare the versions themselves rather than just full regular ordering. it's intended that the ordering is only well defined when the apps are the same i think.
| // | ||
| // Reference: parquet-cpp/src/parquet/metadata.cc | ||
| // | ||
| // PARQUET-686 has more discussion on statistics |
There was a problem hiding this comment.
We haven't done this elsewhere but it isn't the only statistics bug from he C++ perspective. Decimal logical types comparisons for C++ where broken until recently and null statistics for list types are still broken.
There was a problem hiding this comment.
added check for parquet-cpp-arrow version 4.0.0 to check decimal stats etc.
|
|
||
| thriftEncodings = append(thriftEncodings, format.Encoding(parquet.Encodings.RLE)) | ||
| // Only PLAIN encoding is supported for fallback in V1 | ||
| // TODO(zeroshade): Use user specified encoding for V2 |
There was a problem hiding this comment.
note: #11031 recently deprecated "V2" as a standalone concept and now reference minor versions.
There was a problem hiding this comment.
So looking at that PR, it looks like the actual Parquet Format still only has the two values for version being 1 and 2 (with 2 now referring to "most recent version 2") and there's no way to actually write the minor version to the parquet file? Do I have that right? So the "fine grained" version handling is only for specific feature selection of writing, but they all still just write "2" to the parquet file metadata, right?
There was a problem hiding this comment.
Yes, that is my understanding.
go/parquet/metadata/file.go
Outdated
| createdBy := f.props.CreatedBy() | ||
| f.metadata.CreatedBy = &createdBy | ||
|
|
||
| // Users cannot set the `ColumnOrder` since we donot not have user defined sort order |
There was a problem hiding this comment.
| // Users cannot set the `ColumnOrder` since we donot not have user defined sort order | |
| // Users cannot set the `ColumnOrder` since we do not not have user defined sort order |
There was a problem hiding this comment.
is this comment copied from somewhere?
There was a problem hiding this comment.
go/parquet/metadata/file.go
Outdated
| } | ||
|
|
||
| fileEncProps := f.props.FileEncryptionProperties() | ||
| if fileEncProps != nil && !fileEncProps.EncryptedFooter() { |
There was a problem hiding this comment.
is there a reason to allow nil fileEncProps (can defaults always indicate no encryption?)
There was a problem hiding this comment.
so, Go doesn't allow default values for function params like C++. The standard pattern would be to have a separate function for the case with encryption or not or to use the options pattern. I'll take a look and see which pattern i like better for this.
There was a problem hiding this comment.
Ah, i misunderstood the question here.
The defaults are that the FileEncryptionProperties() are nil which indicates no encryption.
go/parquet/metadata/file.go
Outdated
| fileEncProps := f.props.FileEncryptionProperties() | ||
| if fileEncProps != nil && !fileEncProps.EncryptedFooter() { | ||
| var signingAlgo parquet.Algorithm | ||
| algo := fileEncProps.Algorithm() |
There was a problem hiding this comment.
i find fileEncProps a little too short because there is both encoding and encryption in parquet
There was a problem hiding this comment.
renamed to encryptProps
|
|
||
| out := &FileMetaData{ | ||
| FileMetaData: f.metadata, | ||
| version: NewAppVersion(f.metadata.GetCreatedBy()), |
There was a problem hiding this comment.
just want to make sure this references go someplace by default?
There was a problem hiding this comment.
at the top of go/parquet/writer_properties.go there is a constant DefaultCreatedBy which references parquet-go which is the default value for the CreatedBy string in the metadata unless a user overrides it in their writer properties.
|
|
||
| out.RowGroups = make([]*format.RowGroup, 0, len(rowGroups)) | ||
| for _, selected := range rowGroups { | ||
| out.RowGroups = append(out.RowGroups, f.RowGroups[selected]) |
There was a problem hiding this comment.
does it pay to make this a map for upstream consumers (I don't think it is done in other implementations just wondering if it is important).
There was a problem hiding this comment.
a map instead of a slice? Not sure what benefit a map would provide here?
There was a problem hiding this comment.
it would keep the indices of the RowGroups selected.
There was a problem hiding this comment.
Ah gotcha. So, in this particular case, it's creating a new FileMetaData with the selected subset of RowGroups, I think that it would be surprising to a consumer if the RowGroups in the subset weren't in the order that was provided as the argument (allowing for the rowgroups in the subset to be in a different order than the parent). Thus RowGroup(0) of the new FileMetaData that is returned, should be the row group at index rowGroups[0] of the parent File Metadata. It might make sense to keep a map in addition to the slice, but right now I don't believe it keeps any reference to the parent FileMetaData that it is a subset of, so I'm fine with seeing if that actually gets requested as a feature, rather than doing it now.
| panic("decryption not set propertly, cannot verify signature") | ||
| } | ||
|
|
||
| serializer := thrift.NewThriftSerializer() |
There was a problem hiding this comment.
@ggershinsky not sure if you want to look at cryptographic related functions here at all.
There was a problem hiding this comment.
A massive one, but overall looks well structured and in line with the Java and C++ implementations. The only way to verify the crypto code is to try to read encrypted files produced by the Java implementation, and to make the latter read encrypted files produced by this implementation.
(C++ version might be also ok for this purpose, but the Java one is more actively maintained).
Thanks @zeroshade and @emkornfield .
There was a problem hiding this comment.
@ggershinsky I used the files here: https://github.com/apache/parquet-testing to test the encryption stuff the same way that I believe the C++ does its testing of the encrypted reading/writing. After this gets merged and I add the file module which has all of the direct file handling, I can add back the tests which did this verification.
There was a problem hiding this comment.
yep, makes sense. these files were written by C++ encryption code, but were tested against the Java version too, so should be fine.
| assert.NoError(t, err) | ||
| assertStatsSet(t, rg1Col1) | ||
| assertStatsSet(t, rg1Col2) | ||
| assert.Equal(t, statsInt.Min, assertStats(t, rg1Col1).EncodeMin()) |
There was a problem hiding this comment.
its a little hard to grok/review if all of these tests are correct. I'm glad there are tests, but I'm wondering if there might be a better way to express the tests so that is it easier to verify the assertions are what we would expect.
There was a problem hiding this comment.
I actually copied these test cases from the C++ implementation (though that was back in january). I'll see if i can make the tests more obvious as to what they are testing.
|
@emkornfield Okay, i've either addressed or asked for clarification on pretty much all your comments I think. Let me know if I missed anything :) |
|
bump @emkornfield |
|
I think a few more doc update and we can merge and come back to any remaining issues. |
|
updated comments as discussed. |
Here's the next chunk of code following the merging of apache#10716 Thankfully the metadata package is actually relatively small compared to everything else so far. @emkornfield @sbinet @nickpoorman for visibility Closes apache#10951 from zeroshade/parquet-metadata-package Authored-by: Matthew Topol <mtopol@factset.com> Signed-off-by: Matthew Topol <mtopol@factset.com>
Here's the next chunk of code following the merging of #10716
Thankfully the metadata package is actually relatively small compared to everything else so far.
@emkornfield @sbinet @nickpoorman for visibility