Skip to content

Conversation

@sbinet
Copy link
Contributor

@sbinet sbinet commented Apr 4, 2019

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
Contributor Author

sbinet commented Apr 4, 2019

@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
Copy link
Member

wesm commented Apr 4, 2019

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

@codecov-io
Copy link

codecov-io commented Apr 4, 2019

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 Files Coverage Δ
go/arrow/ipc/message.go 0% <0%> (ø)
go/arrow/ipc/reader.go 0% <0%> (ø)
go/arrow/ipc/ipc.go 0% <0%> (ø)
go/arrow/ipc/file_reader.go 0% <0%> (ø)
go/arrow/ipc/metadata.go 0% <0%> (ø)
go/arrow/internal/bitutil/bitutil.go 93.33% <100%> (+0.11%) ⬆️
go/arrow/ipc/dict.go 100% <100%> (ø)
go/arrow/schema.go 87.77% <35.71%> (-9.6%) ⬇️
js/src/ipc/metadata/json.ts 92.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
Contributor Author

sbinet commented Apr 4, 2019

@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
Contributor Author

sbinet commented Apr 5, 2019

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

Copy link
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
Contributor Author

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.

Copy link
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
Contributor Author

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.

Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

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 🎉

@sbinet
Copy link
Contributor Author

sbinet commented Apr 9, 2019

PTAL.

Copy link
Contributor

@nickpoorman nickpoorman left a comment

Choose a reason for hiding this comment

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

This is great!

Sign up for free to 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