-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Switch from deprecated xz2 to libzma #18331
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
Conversation
| # Temporary override: pull apache-avro from upstream to include unreleased fixes. | ||
| # TODO: remove once the next version of apache-avro is published to crates.io and includes commit 3b202c5. | ||
| [patch.crates-io] | ||
| apache-avro = { git = "https://github.com/apache/avro-rs", rev = "3b202c58f12bd1217eccf8a0028e4176ee4aadf9" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to see if a new proper release of apache-avro can be shipped to crates.io in order to let this just be a normal dependency. There seems to be a PR that tries to do this already #17509.
I see that @timsaucer moved apache-avro from xz2 to libzma here apache/avro-rs@3b202c5 (10 Sep).
If https://github.com/apache/avro-rs in fact follows a 2 month release cadence, we should be seeing a new version on crates.io very soon (last release was 25 Aug, 2 months and 3 days ago)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened a ticket there apache/avro-rs#323
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that [email protected] has been released and we can now use it instead of the patch. It seems that there are some discussions going on the adoption of arrow-avro PR.
@timsaucer I think we could merge either my PR or yours while waiting for the long term solution (after using [email protected] instead of the patch), what do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upgrade to [email protected] should be a very simple thing to ship that immediately unblocks getting rid of the deprecated xz2 here, so +1 on shipping any of the two PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can update my PR and move it to ready to review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me 👍 I'll close mine then
|
I think this is a duplicate of #17509 (as @gabotechs points out) Also My guess is that our best long term solution is to get #17861 merged in and then close both this PR and #17509 |
|
At the very least I think this needs to move to Draft. We cannot merge it in with a Thank you for the work on this @N-Boutaib . |
Which issue does this PR close?
Rationale for this change
This PR switches from
xz2toliblzmato reduce duplicate dependencies.What changes are included in this PR?
async-compressionfrom0.4.19to0.4.32, which usesliblzmainstead ofxz2apach-avroto use this revision which switches toliblzmainavro-rsxz2toliblzmaindatafusion-coreanddatafusion-datasourceAre these changes tested?
Yes, via unit tests
Are there any user-facing changes?
None. Getting rid of deprecated carte