-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Revert fastparquet nullable dtype support #42954
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
Revert fastparquet nullable dtype support #42954
Conversation
lithomas1
commented
Aug 9, 2021
- closes #xxxx
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
Hello @lithomas1! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-08-10 18:43:10 UTC |
OK, failing tests are flaky. So this is ready for review now. The backport will be a bit tricky, so I'll do it manually. (Basically backport the last PR and this PR together. We're still gonna backport that one since it unpins fastparquet even though most of its changes are reverted here). cc @jorisvandenbossche (sorry for not pinging last time) @jreback |
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.
Thanks a lot, looks good to me
The backport will be a bit tricky, so I'll do it manually. (Basically backport the last PR and this PR together. We're still gonna backport that one since it unpins fastparquet even though most of its changes are reverted here).
Another option is to first do a simple plain revert of the previous PR, and then include the changes of both PRs here (that will also make a easier to understand diff of the final changes), and then there is only a single PR to backport.
i am fine with this change. |
ping for merge @jreback. Just merged master |
thanks @lithomas1 |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon! If these instruction are inaccurate, feel free to suggest an improvement. |
@meeseeksdev backport 1.3.x |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon! If these instruction are inaccurate, feel free to suggest an improvement. |
@lithomas1 if you wouldn't mind do the manual backport here |
The net result is no release note. do we need one? It's not clear to me what is changed/fixed? |
Indeed, no release note, but the two combined PRs still have some changes to support the latest fastparquet version properly (in theory that could have been added as release note). |
@lithomas1 what is added and then removed in the release notes is not identical, so have synced the notes in #42985, so no need to backport that change. |
Sorry, thanks for fixing that, planning on doing the backport now. |