Skip to content

ARROW-5108: [Go] implement reading primitive arrays from Arrow file - #4114

Closed
sbinet wants to merge 21 commits into
apache:masterfrom
sbinet-gonum:issue-5108
Closed

ARROW-5108: [Go] implement reading primitive arrays from Arrow file#4114
sbinet wants to merge 21 commits into
apache:masterfrom
sbinet-gonum:issue-5108

Conversation

@sbinet

Copy link
Copy Markdown
Contributor

This CL adds the generation of flatbuffers-based files for the IPC backend.
This CL adds the basic functionality to read an Arrow file.

@sbinet

Copy link
Copy Markdown
ContributorAuthor

@stuartcarnie@alexandreyc PTAL.

it's a lot of code, but most of it is coming from the automatically generated code from flatbuffers.
one can safely ignore everything under:

  • go/arrow/internal/flatbuf
  • go/arrow/ipc/feather/fbs

@wesm

wesm commented Apr 4, 2019

Copy link
Copy Markdown
Member

Is there a way to generate the Flatbuffers bindings at build time or do the files have to be checked in?

Comment threadgo/arrow/ipc/message.go Outdated
@codecov-io

codecov-io commented Apr 4, 2019

Copy link
Copy Markdown

Codecov Report

Merging #4114 into master will decrease coverage by 0.56%.
The diff coverage is 6.17%.

Impacted file tree graph

@@ Coverage Diff @@## master #4114 +/- ##
==========================================
- Coverage 88.44% 87.87% -0.57% 
==========================================
Files 752 758 +6 Lines 91515 92163 +648 Branches 1251 1251 ==========================================
+ Hits 80937 80990 +53 - Misses 10457 11052 +595 
Partials 121 121
Impacted FilesCoverage Δ
go/arrow/ipc/message.go0% <0%> (ø)
go/arrow/ipc/reader.go0% <0%> (ø)
go/arrow/ipc/ipc.go0% <0%> (ø)
go/arrow/ipc/file_reader.go0% <0%> (ø)
go/arrow/ipc/metadata.go0% <0%> (ø)
go/arrow/internal/bitutil/bitutil.go93.33% <100%> (+0.11%)⬆️
go/arrow/ipc/dict.go100% <100%> (ø)
go/arrow/schema.go87.77% <35.71%> (-9.6%)⬇️
js/src/ipc/metadata/json.ts92.39% <0%> (-4.35%)⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d49e39a...ede1b34. Read the comment docs.

@sbinet

Copy link
Copy Markdown
ContributorAuthor

@wesm: this (unfortunately?) isn't the way Gophers like to do it.
we prefer to have (for security reasons) only Go stdlib-based code being executed as part of the usual go get incantation to retrieve+build code.
and we prefer to have the whole code needed for users, ready be to built (w/o additional build-time tools.)

@sbinet

Copy link
Copy Markdown
ContributorAuthor

the flurry of activity is now done.
I am pretty happy with my refactoring and ready for the writing part (+ support for binary/string types.)

PTAL

Comment threadgo/arrow/ipc/message.go Outdated
Comment threadgo/arrow/ipc/message.go Outdated
Comment threadgo/arrow/ipc/message.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why you switched from pointer to value receiver?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was mainly to convey that method wouldn't modify the receiver.
I can switch to pointer receiver.

Comment threadgo/arrow/ipc/message.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe check for msg.meta and msg.body to be not nil before releasing them?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put the check+panic in the NewMessage instead.

Comment threadgo/arrow/ipc/message.go Outdated

@stuartcarniestuartcarnie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excited to see this work 🎉

Comment threadgo/arrow/ipc/ipc.go Outdated
Comment threadgo/arrow/ipc/ipc.go Outdated
Comment threadgo/arrow/ipc/ipc.go Outdated
@sbinet

Copy link
Copy Markdown
ContributorAuthor

PTAL.

@nickpoormannickpoorman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

Comment threadgo/arrow/ipc/metadata.go Outdated
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@sbinet@wesm@codecov-io@stuartcarnie@nickpoorman@alexandreyc