ARROW-12104: [Go][Parquet] Second chunk of Ported Go Parquet code #9817

zeroshade · 2021-03-26T20:58:06Z

Following up from #9671 this is the next chunk of ported code consisting of the generated Thrift Code and the utilities for supporting Encryption, Compression and Reader/Writer Property handling.

Thankfully this is much smaller than the previous chunk, and so should be much easier to review and read.

github-actions · 2021-03-26T20:58:25Z

https://issues.apache.org/jira/browse/ARROW-12104

WilliamWhispell

LGTM

WilliamWhispell · 2021-03-26T22:10:49Z

go/parquet/compress/brotli.go

+			sofar += n
+		}
+		if err != nil && err != io.EOF {
+			panic(err)


Why panic and not return an error?

simplifies the Encode/Decode interface, and isn't recoverable from if it fails anyways.

WilliamWhispell · 2021-03-26T22:20:22Z

go/parquet/compress/compress.go

+func GetCodec(typ Compression) Codec {
+	ret, ok := codecs[typ]
+	if !ok {
+		// return codecs[Codecs.Uncompressed]


does this require further thought on defaults? or should the comment go?

For now i liked the idea of erroring when trying to retrieve a codec if we haven't implemented it rather than silently returning the uncompressed one.

The alternative here to panicing would be to change this to return (Codec, error) and have it return nil and an unimplemented error if it can't find the desired codec.

I think we should use panics when absolutely necessary. If a codec can't be found, the way to do that in Go is to return an error. This is how I implemented it:

https://github.com/nickpoorman/arrow/blob/ARROW-7905-go-parquet/go/parquet/parquet/compress/compress.go#L31

@nickpoorman

So, between the ideas of just defaulting down to returning Uncompressed vs modifying this to return (Codec, error) and returning an error, what do you think?

i've updated this to return (Codec, error) now instead of panic'ing.

👍 I think returning the error is the right move

go/parquet/compress/compress_test.go

WilliamWhispell · 2021-03-26T22:21:51Z

go/parquet/compress/compress_test.go

+
+			wr := codec.NewWriter(&buf)
+
+			const chunkSize = 1111


pulled this test from the C++ implementation tests. Ultimately it's because it's a number that is small enough to make sure we'll have multiple chunks but large enough that it'll have some compression it can do :)

I'm surprised C++ doesn't use a power of 2 (1024?)

I think the intent was to ensure that there is a chunk at the end which is not a full chunk to make sure we test that handling properly.

WilliamWhispell · 2021-03-30T16:37:01Z

go/parquet/compress/compress.go

+func (nocodec) CompressBound(len int64) int64 { return len }
+
+func init() {
+	codecs[Codecs.Uncompressed] = nocodec{}


I like the name, but any thoughts on Codecs.Identity - for example, https://grpc.github.io/grpc-java/javadoc/io/grpc/Codec.Identity.html

I think i prefer leaving it as Uncompressed for the name.

emkornfield · 2021-03-31T04:29:42Z

sorry this week is particularly bad. I will try to review on the weekend/next week.

go/parquet/compress/compress_test.go

emkornfield · 2021-03-31T15:37:41Z

I removed the "@" mentions in the description. It appears I get notified everytime someone clones the commit in master. Please tag people as a first comment instead.

zeroshade · 2021-03-31T16:00:34Z

@emkornfield Did not realize that, will keep that in mind for future PRs. Sorry!

zeroshade · 2021-04-04T18:22:48Z

rebased from master

some docs / comments update go.sum

zeroshade · 2021-04-07T15:06:51Z

@emkornfield @sbinet @nickpoorman Bump on getting reviews here! thanks!

emkornfield · 2021-04-07T15:54:56Z

will try to do a first pass today or tomorrow.

emkornfield

Took a pass through and looks reasonable. I'm not to familiar with encryption stuff. @ggershinsky to see if he wants to take a look.

go/parquet/compress/brotli.go

go/parquet/compress/compress.go

emkornfield · 2021-04-08T15:31:57Z

go/parquet/compress/compress.go

+	// NewWriter provides a wrapper around a write stream to compress data before writing it.
+	NewWriter(io.Writer) io.WriteCloser
+	// NewWriterLevel is like NewWrapper but allows specifying the compression level
+	NewWriterLevel(io.Writer, int) (io.WriteCloser, error)


API comment. It is worth exposing compression level in the API, an laternative would have be a member used at construction time?

So the way I built this was to minimize object creation, so the map contains objects which are stateless to ensure we have no race conditions, the objects themselves know how to construct the appropriate encoders and decoders, that's why I exposed it like this since not all of the libraries allow configuring the compression level after constructing the encoder. Most of them take the compression level as an argument to their NewWriter functions etc. which is what I modeled this after.

go/parquet/compress/compress.go

go/parquet/compress/zstd.go

go/parquet/reader_properties.go

emkornfield · 2021-04-08T15:50:02Z

go/parquet/types.go

+		Required  Repetition
+		Optional  Repetition
+		Repeated  Repetition
+		Undefined Repetition // convenience value


it might pay to distinguish beween undefined and not set. An issue was raised recently that at least in C++ we write Repetition required for the root of the schema when according to the spec we probably shouldn't.

in this case Undefined == not set it explicitly only exists to have an available option for "Not Set". Since "undefined" is not a legitimate value for the parquet spec. If you'd prefer I can change the name of this to be NotSet rather than Undefined.

emkornfield · 2021-04-08T15:52:08Z

go/parquet/writer_properties.go

+}
+
+// WithMaxRowGroupLength specifies the number of rows as the maximum number of rows for a given row group in the writer.
+func WithMaxRowGroupLength(nrows int64) WriterProperty {


not sure if this was mirrored on the C++ implementation intentionally, just wanted to check that this pattern is idiomatic go?

The C++ implementation uses a Builder pattern that constructs an object with a bunch of "Set...." functions on it and then eventually calls "Build" to generate the resulting object. This follows the idiomatic Go pattern for taking options, which is the same pattern used in the Arrow Go library for things like the ipc.NewWriter etc.

Essentially it ends up looking like:

parquet.NewWriterProperties(parquet.WithMaxRowGroupLength(nrows), parquet.With.........)

where NewWriterProperties can take any number of these WriterProperty options including none which would just produce the default WriterProperties.

ggershinsky · 2021-04-08T18:14:03Z

Took a pass through and looks reasonable. I'm not to familiar with encryption stuff. @ggershinsky to see if he wants to take a look.

will be glad to

ggershinsky · 2021-04-12T07:54:26Z

had a quick look at the encryption part, no specific comments, seems to be well structured and coded; as long as it interoperates with its C++ counterpart (can read/decrypt the files written/encrypted by the other), it should be ok - but this is applicable to all other parquet features as well :)

this pull request covers the so-called "low level" encryption layer. On top of it, parquet also has a "high-level" encryption layer, please see the explanation of differences and reasons. The high level layer is already released in Java parquet (v1.12.0), that went into Spark master for the next Spark release. This layer is also implemented in C++, and merged in Arrow (#8023). You might consider adding this layer to the Go implementation as well, so it will be able to interop with Spark and PyArrow, and will benefit from the additional security checks/features.

zeroshade · 2021-04-12T14:19:13Z

@ggershinsky So the completed library does use the files in the parquet-test-data github submodule in order to confirm that I'm able to read/decrypt the files there in addition to the files written by itself, so that ensures the interoperability.

I'll take a look at the "high-level" encryption layer and see how difficult it would be to implement / add. Depending on that I may add it as a separate PR rather than adding it to this one, if that's ok?

ggershinsky · 2021-04-12T14:31:21Z

Sounds good. My intention was to suggest the high-level layer as a future separate pull request, I should have been more explicit about this.

zeroshade · 2021-04-13T13:53:12Z

After looking at it a bit, I agree that it's definitely a good idea to add the high-level layer as a future enhancement after I finish getting the rest of the parquet impl merged :) Thanks for the suggestion @ggershinsky

@emkornfield @sbinet @nickpoorman any other comments on this one or am i good to go? :)

I've got about 4 more of these, so I'm getting antsy! haha 😝

Following up from #9817 this is the next chunk of code for the Go Parquet port consisting of the Schema package, implementing the Converted and Logical types along with handling schema creation, manipulation, and printing. Closes #10071 from zeroshade/arrow-12424 Authored-by: Matthew Topol <[email protected]> Signed-off-by: Micah Kornfield <[email protected]>

Following up from apache#9671 this is the next chunk of ported code consisting of the generated Thrift Code and the utilities for supporting Encryption, Compression and Reader/Writer Property handling. Thankfully this is much smaller than the previous chunk, and so should be much easier to review and read. Closes apache#9817 from zeroshade/arrow-12104 Authored-by: Matthew Topol <[email protected]> Signed-off-by: Micah Kornfield <[email protected]>

Following up from apache#9817 this is the next chunk of code for the Go Parquet port consisting of the Schema package, implementing the Converted and Logical types along with handling schema creation, manipulation, and printing. Closes apache#10071 from zeroshade/arrow-12424 Authored-by: Matthew Topol <[email protected]> Signed-off-by: Micah Kornfield <[email protected]>

github-actions bot added the Component: Go label Mar 26, 2021

WilliamWhispell reviewed Mar 30, 2021

View reviewed changes

emkornfield reviewed Mar 31, 2021

View reviewed changes

go/parquet/compress/compress_test.go Outdated Show resolved Hide resolved

zeroshade force-pushed the arrow-12104 branch from 8577e9d to 90bf7ee Compare April 4, 2021 18:22

Matthew Topol added 11 commits April 4, 2021 14:25

add more utils and basic properties

7322d0a

some docs / comments update go.sum

add apache license and exclude generated thrift

941ce5e

unit test for compress package

9aa9a4e

more comments and docs

75170f6

some cleanup as per go vet

b532763

make go lint happier

38c240d

suggestions from staticcheck linting

8dc288e

add package comment for thrift helpers

fd2712c

add clean target in makefile

6c8e463

add license to staticcheck.conf

5e933c5

removing support for LZ4 as per feedback

90bf7ee

emkornfield reviewed Apr 8, 2021

View reviewed changes

update based on feedback for assert and comments.

72946d0

swap panic for return error

7228c39

remove lz4 from go.mod

ba8c4ac

nickpoorman approved these changes Apr 14, 2021

View reviewed changes

emkornfield closed this in b2ceb8f Apr 16, 2021

zeroshade mentioned this pull request Apr 16, 2021

ARROW-12424: [Go][Parquet] Adding Schema Package for Go Parquet #10071

Closed

zeroshade deleted the arrow-12104 branch September 12, 2021 19:34

asfimport mentioned this pull request Apr 18, 2021

Next Chunk of ported Code #27928

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-12104: [Go][Parquet] Second chunk of Ported Go Parquet code #9817

ARROW-12104: [Go][Parquet] Second chunk of Ported Go Parquet code #9817

zeroshade commented Mar 26, 2021 •

edited by emkornfield

Loading

github-actions bot commented Mar 26, 2021

WilliamWhispell left a comment

WilliamWhispell Mar 26, 2021

zeroshade Mar 30, 2021

WilliamWhispell Mar 26, 2021

zeroshade Mar 30, 2021

nickpoorman Apr 13, 2021

zeroshade Apr 14, 2021

zeroshade Apr 14, 2021

nickpoorman Apr 14, 2021

WilliamWhispell Mar 26, 2021

zeroshade Mar 30, 2021

emkornfield Mar 31, 2021

zeroshade Apr 5, 2021

WilliamWhispell Mar 30, 2021

zeroshade Mar 30, 2021

emkornfield commented Mar 31, 2021

emkornfield commented Mar 31, 2021

zeroshade commented Mar 31, 2021

zeroshade commented Apr 4, 2021

zeroshade commented Apr 7, 2021

emkornfield commented Apr 7, 2021

emkornfield left a comment

emkornfield Apr 8, 2021

zeroshade Apr 9, 2021

emkornfield Apr 8, 2021

zeroshade Apr 9, 2021

emkornfield Apr 8, 2021

zeroshade Apr 9, 2021

ggershinsky commented Apr 8, 2021

ggershinsky commented Apr 12, 2021

zeroshade commented Apr 12, 2021

ggershinsky commented Apr 12, 2021

zeroshade commented Apr 13, 2021 •

edited

Loading

ARROW-12104: [Go][Parquet] Second chunk of Ported Go Parquet code #9817

ARROW-12104: [Go][Parquet] Second chunk of Ported Go Parquet code #9817

Conversation

zeroshade commented Mar 26, 2021 • edited by emkornfield Loading

github-actions bot commented Mar 26, 2021

WilliamWhispell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emkornfield commented Mar 31, 2021

emkornfield commented Mar 31, 2021

zeroshade commented Mar 31, 2021

zeroshade commented Apr 4, 2021

zeroshade commented Apr 7, 2021

emkornfield commented Apr 7, 2021

emkornfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggershinsky commented Apr 8, 2021

ggershinsky commented Apr 12, 2021

zeroshade commented Apr 12, 2021

ggershinsky commented Apr 12, 2021

zeroshade commented Apr 13, 2021 • edited Loading

zeroshade commented Mar 26, 2021 •

edited by emkornfield

Loading

zeroshade commented Apr 13, 2021 •

edited

Loading