-
Notifications
You must be signed in to change notification settings - Fork 269
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
feat!: update DataFusion to 45.0 and Arrow to 54.1 #3503
Conversation
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3503 +/- ##
==========================================
- Coverage 78.49% 78.49% -0.01%
==========================================
Files 253 253
Lines 94542 94543 +1
Branches 94542 94543 +1
==========================================
- Hits 74213 74211 -2
- Misses 17319 17327 +8
+ Partials 3010 3005 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
We need to make this as a breaking change. |
@eddyxu I tried adding a breaking change text in a commit to see if the github actions bot would add the tag, but that didn't seem to make an impact. Can you add the tag or do I need to put something in the description? |
8136543
to
4923988
Compare
Looks like CI kept failing because there were changes on |
…x<>> in a few places and shifting over to owned data in a couple of structs
… impacts primarily the python crate
9a0ef7c
to
75f4222
Compare
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.
Thank you so much for taking this on. I had made an attempt earlier and gotten daunted by all the python changes.
I have a few minor suggestions but overall things look good
let old_id: String = ob.getattr("old_id")?.extract()?; | ||
let new_id: String = ob.getattr("new_id")?.extract()?; |
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.
Why is it that extract
works here I wonder? I feel like all the other string extract
were changed to downcast
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 for working on this. I recognize the PyO3 changes were a lot.
I would like to not have to modify WriteDestination
like that. It makes the changes rather large. I found an alternative solution using pyo3's PyBackedStr
, and have put up a commit here: rerun-io@aa14dd3
Thank you both for the reviews, and especially for that commit. That is a much nicer solution than I came up with. Can one of you run the workflow again? |
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.
One last tiny nit and then I'm good. Thanks again for taking this task on :)
Co-authored-by: Weston Pace <[email protected]>
The |
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 for following up on those changes. Great work here.
Sorry, I just noticed the PR title was |
Bumping arrow counts as breaking change iiuc? |
Also, anyone who uses the python crate as a dependency will be forced to update pyo3 which introduces many deprecations. |
Fair points. Some of our APIs take arrow data directly so our users would need to stay in lock-step. |
This PR updates DataFusion to 45.0 and Arrow to 54.1.
The update to Arrow required updating PyO3 for the python package. This had a series of breaking changes to their API.