Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Parquet version 2.9.0 #20

Merged
merged 3 commits into from
Apr 26, 2022

Conversation

kylebarron
Copy link
Contributor

@kylebarron kylebarron commented Apr 21, 2022

Change list

@sunchao
Copy link
Owner

sunchao commented Apr 25, 2022

Thanks @kylebarron , let me know when the PR is ready!

@kylebarron
Copy link
Contributor Author

I assume the breaking changes from updating thrift (see #18) aren't desired? So we need to figure out a way to use thrift 0.13, right?

Any suggestions there? I assume figuring out their docker setup is the easiest approach

@kylebarron kylebarron marked this pull request as ready for review April 25, 2022 17:48
@kylebarron
Copy link
Contributor Author

I was able to build the docker image for thrift 0.13, so I think this PR should be good to review now.

Copy link
Owner

@sunchao sunchao left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @kylebarron !

I assume the breaking changes from updating thrift (see #18) aren't desired? So we need to figure out a way to use thrift 0.13, right?

I think we can still upgrade Thrift to the latest version in a separate PR, and bump up the major version after that. Thrift 0.13 is a bit old now.

@kylebarron
Copy link
Contributor Author

It seemed from #18 that there wasn't consensus over whether to upgrade.

I figured the easier approach would be to upgrade the thrift definitions while still using thrift 0.13, because then we can sidestep for now the decision of whether upgrading the thrift compiler version is worth it.

@sunchao sunchao merged commit 4d79c86 into sunchao:master Apr 26, 2022
@sunchao
Copy link
Owner

sunchao commented Apr 26, 2022

Sounds good. Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants