ARROW-12045: [Go][Parquet] Initial Chunk of Parquet port to Go #9671

zeroshade · 2021-03-10T16:16:05Z

Based on the c++ implementation but tuned and optimized for Go, I spent the first couple months this year creating a Go implementation for Parquet with the goal of native/easy integration with the Arrow library while still being highly performant and at minimum reaching feature parity with the C++ implementation.

Based on the conversations on the JIRA card, rather than dumping a huge code bomb (there's a ton), I've chunked it up. This is the initial chunk of code comprising of an internal utils directory that is analogous to the cpp/arrow/utils/ bit readers/writers/bit run readers/etc. which were ultimately used by the go implementation, while using c2goasm to reach the performance necessary for certain areas.

This is part 1 of the implementation as I chunk it up and push it out. I'll wait for each chunk to get merged before pushing the next PR in order to make sure that everything stays in sync.

CC: @emkornfield @wesm @sbinet @nickpoorman

go/parquet/internal/testutils/random.go

github-actions · 2021-03-10T16:53:32Z

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

emkornfield · 2021-03-10T22:12:07Z

@zeroshade would it be possible to create a new JIRA and PR for the bump in version requirement. That should likely be separate from the core parquet work?

zeroshade · 2021-03-10T23:12:37Z

@emkornfield sure, that's easy. i didn't realize i was gonna have to do it until the actions were failing haha.

zeroshade · 2021-03-10T23:29:37Z

@emkornfield filed as #9675

@emkornfield

@emkornfield spun off from #9671 Closes #9675 from zeroshade/arrow-11931 Authored-by: Matthew Topol <mtopol@factset.com> Signed-off-by: Sebastien Binet <binet@cern.ch>

zeroshade · 2021-03-12T14:55:51Z

@sbinet @emkornfield I've rebased this to include the changes to bump to go1.15 so now this is ready for reviews. After this gets merged, i'll push the next chunk of code. Sorry for the size of this one but it was the smallest i could easily make it as a self contained piece that doesn't rely on other chunks. It's a foundational piece while the rest of the chunks will be a lot smaller.

go/parquet/go.sum

go/parquet/LICENSE.txt

emkornfield · 2021-03-16T03:55:48Z

go/parquet/internal/bmi/Makefile

+PERL_FIXUP_ROTATE=perl -i -pe 's/(ro[rl]\s+\w{2,3})$$/\1, 1/'
+C2GOASM=c2goasm -a -f
+CC=clang
+C_FLAGS=-masm=intel -mno-red-zone -mstackrealign -mllvm -inline-threshold=1000 \


does this limit what architectures/OS this can work for? For instance in c++ we are careful to allow compilation that don't have AVX2

In the actual Go code, if the architecture doesn't have AVX2 it just falls back to a pure go implementation. More explained in the reply to your main comment.

emkornfield

Thanks for breaking this up into smaller chunks. It is still quite large, so I will try to take a more detailed pass but some high level things stood out to me:

Documentation is pretty sparse. If this was an issue with original code, it would be good to break this anti-pattern.
From a build perspective, is there a way to automatically generate the ASM files so they don't need to be checked in?
From a code organization, would it make sense to separate out all the bit helpers into another package? A lot of the bit util code (not all of it) in C++ arrow is used in multiple places. This might be premature so I'm okay holding off on it.

go/parquet/internal/bmi/_lib/bitmap_bmi2.c

go/parquet/internal/bmi/_lib/bitmap_bmi2.s

go/parquet/internal/bmi/bitmap_bmi2.go

go/parquet/internal/bmi/bmi_init.go

emkornfield · 2021-03-16T04:05:15Z

go/parquet/internal/testutils/random_arrow.go

+	"golang.org/x/exp/rand"
+)
+
+func RandomNonNull(dt arrow.DataType, size int) array.Interface {


based on the name of the files, Arrow might be the better package here?

so these are actually test utilities that i end up using for generating random arrow arrays for use with the unit tests for writing arrow directly to parquet inside of a pqarrow package that isn't included here. As mentioned above, by putting it in the internal package path, these cannot be imported outside of the parquet packages. If i put this into the Arrow package, then I'd need to make them exported publicly in the arrow package which i was trying to avoid doing.

this seems OK for now. It seems like the best solution would be having a "test utility package" that both Arrow and Parquet could share for tests.

emkornfield · 2021-03-16T04:05:37Z

go/parquet/internal/testutils/random_arrow.go

+			bldr.Append(buf)
+		}
+		return bldr.NewArray()
+	// case arrow.DECIMAL:


why commented out?

See #9671 (comment) but long story short: these rely on a function that was exported in a later chunk of this change, so rather than delete them and then re-add it later, I just commented them out until i add that chunk. If you'd prefer these to be deleted for now and then added as part of that chunk, I can do that.

go/parquet/internal/testutils/random_arrow.go

go/parquet/internal/utils/math.go

go/parquet/internal/utils/rle.go

emkornfield · 2021-03-16T17:38:27Z

Regarding assembly, didn't realize Go was already doing this, if it is consistent with existing patterns makes sense to keep it.

zeroshade · 2021-03-17T17:35:43Z

rebased branch with master, hopefully that fixes the failing check :)

WilliamWhispell

About half way through

go/parquet/internal/bmi/bmi_noasm.go

go/parquet/internal/testutils/random.go

go/parquet/internal/utils/bit_block_counter.go

go/parquet/internal/utils/bit_reader.go

emkornfield · 2021-03-22T15:55:25Z

go/parquet/internal/utils/bit_reader_test.go

+	}
+}
+
+// func TestBitRleOverflow(t *testing.T) {


why commented out?

so this is an artifact of an earlier impl and a test i copied from the C++ impl.

Given the way I implemented the RLE by taking in an io.WriterAt, it's not the same as the c++ version which always expects a specifically sized buffer with a maximum length that it tracks. It just writes to the io.WriterAt and returns whether or not that was successful, and should fail if it can't write. So this test ends up being not worthwhile or relevant in this implementation, I had intended to potentially come back and work this is but never did. So i'll just delete it for now rather than leaving it commented out here.

emkornfield · 2021-03-22T15:56:35Z

go/parquet/internal/utils/bit_run_reader.go

+// BitRunReader is an interface that is usable by multiple callers to provide
+// multiple types of bit run readers such as a reverse reader and so on.
+//
+// It's a convenience interface for counting contiguous set/unset bits in a bitmap.


it might be worth commenting that in places where BitBlockCounter can be used that is apreferred method since it will be faster.

sure, i'll add that to the comment

emkornfield

Left a few more questions. I mostly skimmed the logic as this is a port of the C++ code. Overall, I think it looks ok. I'll wait a couple of days to see if @sbinet or @nickpoorman have any go specific comments.

emkornfield · 2021-03-22T16:07:24Z

One more small nit. Could you create a new sub-jira issue for this PR? We generally like to maintain 1-1 mapping from JIRA to PRs

zeroshade · 2021-03-22T16:54:29Z

@emkornfield When you say "sub-jira issue" do you mean a sub-issue on the linked issue? Or just a new JIRA issue in general? If it's a new JIRA issue altogether, should it be under the "PARQUET" jira instead of the ARROW one?

emkornfield · 2021-03-22T17:41:22Z

@emkornfield When you say "sub-jira issue" do you mean a sub-issue on the linked issue?

Yes, I think this will be the cleanest (it should give you a new ID).

zeroshade · 2021-03-22T18:04:17Z

@emkornfield done and updated the title of this PR. :) Also pushed changes based on your suggestions

github-actions · 2021-03-22T20:39:13Z

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

…heck

…taticcheck

zeroshade · 2021-03-25T16:15:56Z

@emkornfield Just checking in, if there are no comments by @sbinet or @nickpoorman would we be able to get this merged either today or tomorrow? I have the next chunk ready to go, it's also much smaller than this one haha.

emkornfield · 2021-03-25T16:19:29Z

Yes, I'll merge tomorrow if no further comments.

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. Closes #9817 from zeroshade/arrow-12104 Authored-by: Matthew Topol <mtopol@factset.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

@emkornfield

@emkornfield spun off from apache#9671 Closes apache#9675 from zeroshade/arrow-11931 Authored-by: Matthew Topol <mtopol@factset.com> Signed-off-by: Sebastien Binet <binet@cern.ch>

@emkornfield

Based on the c++ implementation but tuned and optimized for Go, I spent the first couple months this year creating a Go implementation for Parquet with the goal of native/easy integration with the Arrow library while still being highly performant and at minimum reaching feature parity with the C++ implementation. Based on the conversations on the JIRA card, rather than dumping a huge code bomb (there's a ton), I've chunked it up. This is the initial chunk of code comprising of an internal utils directory that is analogous to the cpp/arrow/utils/ bit readers/writers/bit run readers/etc. which were ultimately used by the go implementation, while using c2goasm to reach the performance necessary for certain areas. This is part 1 of the implementation as I chunk it up and push it out. I'll wait for each chunk to get merged before pushing the next PR in order to make sure that everything stays in sync. CC: @emkornfield @wesm @sbinet @nickpoorman Closes apache#9671 from zeroshade/arrow-7905-p1 Authored-by: Matthew Topol <mtopol@factset.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

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 <mtopol@factset.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

@emkornfield

Based on the c++ implementation but tuned and optimized for Go, I spent the first couple months this year creating a Go implementation for Parquet with the goal of native/easy integration with the Arrow library while still being highly performant and at minimum reaching feature parity with the C++ implementation. Based on the conversations on the JIRA card, rather than dumping a huge code bomb (there's a ton), I've chunked it up. This is the initial chunk of code comprising of an internal utils directory that is analogous to the cpp/arrow/utils/ bit readers/writers/bit run readers/etc. which were ultimately used by the go implementation, while using c2goasm to reach the performance necessary for certain areas. This is part 1 of the implementation as I chunk it up and push it out. I'll wait for each chunk to get merged before pushing the next PR in order to make sure that everything stays in sync. CC: @emkornfield @wesm @sbinet @nickpoorman Closes apache#9671 from zeroshade/arrow-7905-p1 Authored-by: Matthew Topol <mtopol@factset.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

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 <mtopol@factset.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

@emkornfield

@emkornfield spun off from apache#9671 Closes apache#9675 from zeroshade/arrow-11931 Authored-by: Matthew Topol <mtopol@factset.com> Signed-off-by: Sebastien Binet <binet@cern.ch>

@emkornfield

Based on the c++ implementation but tuned and optimized for Go, I spent the first couple months this year creating a Go implementation for Parquet with the goal of native/easy integration with the Arrow library while still being highly performant and at minimum reaching feature parity with the C++ implementation. Based on the conversations on the JIRA card, rather than dumping a huge code bomb (there's a ton), I've chunked it up. This is the initial chunk of code comprising of an internal utils directory that is analogous to the cpp/arrow/utils/ bit readers/writers/bit run readers/etc. which were ultimately used by the go implementation, while using c2goasm to reach the performance necessary for certain areas. This is part 1 of the implementation as I chunk it up and push it out. I'll wait for each chunk to get merged before pushing the next PR in order to make sure that everything stays in sync. CC: @emkornfield @wesm @sbinet @nickpoorman Closes apache#9671 from zeroshade/arrow-7905-p1 Authored-by: Matthew Topol <mtopol@factset.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

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 <mtopol@factset.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

zeroshade commented Mar 10, 2021

View reviewed changes

go/parquet/internal/testutils/random.go Show resolved Hide resolved

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

zeroshade mentioned this pull request Mar 10, 2021

ARROW-11931: [Go] bump to go1.15 #9675

Closed

sbinet pushed a commit that referenced this pull request Mar 12, 2021

ARROW-11931: [Go] bump to go1.15

ff331f2

@emkornfield spun off from #9671 Closes #9675 from zeroshade/arrow-11931 Authored-by: Matthew Topol <mtopol@factset.com> Signed-off-by: Sebastien Binet <binet@cern.ch>

zeroshade force-pushed the arrow-7905-p1 branch 2 times, most recently from f898486 to d4cb9cc Compare March 12, 2021 14:52

emkornfield reviewed Mar 16, 2021

View reviewed changes

go/parquet/go.sum Show resolved Hide resolved

emkornfield reviewed Mar 16, 2021

View reviewed changes

go/parquet/LICENSE.txt Show resolved Hide resolved

emkornfield reviewed Mar 16, 2021

View reviewed changes

emkornfield requested changes Mar 16, 2021

View reviewed changes

Matthew Topol added 10 commits March 17, 2021 13:32

initial shift to arrow repo, add and clean up utils

feb5bc8

updating assembly

d405f98

add go.mod remove chunk of types that slipped in early

7fcd06e

add parquet to go_build and go_test ci scripts

a0eb831

fix license for go.mod and add go.sum to exclude

c622b46

use return vals instead of output params

fe16a34

first group of adding more comments, more to come.

69c368e

next chunk of new comments and docs.

0775917

more comments

7d2d3f0

split up rle getspaced with helper funcs

545f7a8

zeroshade force-pushed the arrow-7905-p1 branch from df05d6d to 545f7a8 Compare March 17, 2021 17:32

zeroshade requested a review from emkornfield March 17, 2021 17:35

WilliamWhispell reviewed Mar 18, 2021

View reviewed changes

emkornfield reviewed Mar 22, 2021

View reviewed changes

adding comments and fixups from comments

51af5ed

zeroshade changed the title ~~ARROW-7905: [Go][Parquet] Initial Chunk of Parquet port to Go~~ ARROW-12045: [Go][Parquet] Initial Chunk of Parquet port to Go Mar 22, 2021

Matthew Topol added 3 commits March 23, 2021 12:13

update with comments and slight changes suggested by golint / staticc…

864ed7b

…heck

other suggestions of package docs and comment forms as suggested by s…

03589f6

…taticcheck

fixing two noasm issues

8e6944a

emkornfield closed this in b2fa55d Mar 26, 2021

zeroshade mentioned this pull request Mar 26, 2021

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

Closed

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

This was referenced Mar 26, 2021

First Chunk of ported Parquet Code #27874

Closed

[Go] Build failure in parquet/internal/bmi on s390x #28717

Closed

[Go][Parquet] Port the C++ Parquet implementation to Go #24127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-12045: [Go][Parquet] Initial Chunk of Parquet port to Go #9671

ARROW-12045: [Go][Parquet] Initial Chunk of Parquet port to Go #9671

zeroshade commented Mar 10, 2021

github-actions bot commented Mar 10, 2021

emkornfield commented Mar 10, 2021

zeroshade commented Mar 10, 2021

zeroshade commented Mar 10, 2021

zeroshade commented Mar 12, 2021 •

edited

Loading

emkornfield Mar 16, 2021

zeroshade Mar 16, 2021

emkornfield left a comment

emkornfield Mar 16, 2021

zeroshade Mar 16, 2021

emkornfield Mar 22, 2021

emkornfield Mar 16, 2021

zeroshade Mar 16, 2021

emkornfield commented Mar 16, 2021

zeroshade commented Mar 17, 2021

WilliamWhispell left a comment

emkornfield Mar 22, 2021

zeroshade Mar 22, 2021

emkornfield Mar 22, 2021

zeroshade Mar 22, 2021

emkornfield left a comment

emkornfield commented Mar 22, 2021

zeroshade commented Mar 22, 2021

emkornfield commented Mar 22, 2021

zeroshade commented Mar 22, 2021

github-actions bot commented Mar 22, 2021

zeroshade commented Mar 25, 2021

emkornfield commented Mar 25, 2021

ARROW-12045: [Go][Parquet] Initial Chunk of Parquet port to Go #9671

ARROW-12045: [Go][Parquet] Initial Chunk of Parquet port to Go #9671

Conversation

zeroshade commented Mar 10, 2021

github-actions bot commented Mar 10, 2021

emkornfield commented Mar 10, 2021

zeroshade commented Mar 10, 2021

zeroshade commented Mar 10, 2021

zeroshade commented Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

emkornfield commented Mar 16, 2021

zeroshade commented Mar 17, 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

emkornfield left a comment

Choose a reason for hiding this comment

emkornfield commented Mar 22, 2021

zeroshade commented Mar 22, 2021

emkornfield commented Mar 22, 2021

zeroshade commented Mar 22, 2021

github-actions bot commented Mar 22, 2021

zeroshade commented Mar 25, 2021

emkornfield commented Mar 25, 2021

zeroshade commented Mar 12, 2021 •

edited

Loading