Integration with parquet-format #82
Conversation
|
This is WIP at the moment, I just want to test if travis passes with my changes, and we need to discuss if this is an appropriate strategy! |
|
Thanks @sadikovi . I agree on the benefits of including I'm wondering if there's any workaround on the docs.rs issue. I'll also try to explore on my end before deciding which choice to take. |
sadikovi
changed the title
|
I completely understand your concern. I was just thinking that we could have more people using it and/or building on top of it, if crate was self-contained (you would only need Thrift for development). But, I also agree, that it would be better to generate, which makes me feel confused at this point:)
|
Another approach I'm thinking about is to add a feature flag to fetch the |
|
I will close this for now, and reopen if we decide to go this route. Current discussion is on #84. |
sadikovi
changed the title
sadikovi
force-pushed
the
include-parquet-rs
branch
from
79f70ac
to
3bd7c9c
Compare
|
Opened this PR to test integration with parquet-format. |
|
@sadikovi : the |
|
@sunchao Thank you! I will update the pull request. |
sadikovi
changed the title
|
@sunchao I fixed this PR, had to add |
| @@ -1,3 +1,5 @@ | |||
| dist: trusty | |||
| sudo: required | |||
There was a problem hiding this comment.
hmm. why we don't need this before?
There was a problem hiding this comment.
It looks like a regression in travis. I am not sure why we didn't need it before. I will spend some time figuring out why.
There was a problem hiding this comment.
I checked the travis docs. It looks like whenever you explicitly use sudo make or any sudo <command>, travis will assume sudo: required flag. This is mentioned here: https://docs.travis-ci.com/user/reference/overview/#For-a-particular-.travis.yml-configuration
When sudo is required, then build runs on GCE, otherwise in container on EC2. Here is the comparison:
- master branch (without any changes) [KCOV PASS]: https://travis-ci.org/sunchao/parquet-rs/builds/364441406, "Worker information" mentions GCE.
- build with
sudo: required[KCOV PASS]: https://travis-ci.org/sunchao/parquet-rs/builds/365997814, "Worker information" also mentions GCE. - build without any
sudokeywords [KCOV FAIL]: https://travis-ci.org/sunchao/parquet-rs/builds/365537349, "Worker information" lists EC2.
So we have been running with sudo: required all this time, because of this line https://github.com/sadikovi/parquet-rs/blob/master/.travis.yml#L18.
So now we just explicitly state that we require sudo (for kcov issue on travis).
There was a problem hiding this comment.
I see. Nice finding! Thanks for the detailed explanation.
| @@ -36,13 +36,13 @@ extern crate brotli; | |||
| extern crate flate2; | |||
| extern crate rand; | |||
| extern crate x86intrin; | |||
| extern crate parquet_format; | |||
There was a problem hiding this comment.
maybe we can also remove some dependencies that were moved to parquet-format-rs?
There was a problem hiding this comment.
Yes, I will remove them. Thanks.
There was a problem hiding this comment.
Removed them, now lib.rs should contain only parquet dependencies.
| @@ -1,3 +1,5 @@ | |||
| dist: trusty | |||
| sudo: required | |||
There was a problem hiding this comment.
I see. Nice finding! Thanks for the detailed explanation.
|
@sunchao I rebased. Could you have a look again? Thanks! |
|
Merged. Thanks @sadikovi ! |
|
Thank you very much @sunchao! I hope this change will make it easier to use |
This PR adds
parquet-formatdependency and removesparquet_thrift.Uses parquet-format 2.4.0.
With this patch, users of the crate will no longer need Thrift to build the code.