-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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: Add "future" versioning #17421
Conversation
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 understand the benefit of the future
parameter internally, but the whole thing feels to me like we're leaking internals to the public API.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17421 +/- ##
==========================================
- Coverage 80.58% 80.46% -0.12%
==========================================
Files 1480 1483 +3
Lines 193682 195006 +1324
Branches 2765 2780 +15
==========================================
+ Hits 156071 156910 +839
- Misses 37103 37585 +482
- Partials 508 511 +3 ☔ View full report in Codecov by Sentry. |
This comment was marked as resolved.
This comment was marked as resolved.
644b8d8
to
c9f3c55
Compare
I think this is a breaking change for both Rust and Python. (In other words, something like duckdblabs/db-benchmark#79, which was broken a few days ago, will break again.) |
I have renamed |
Thanks, this looks good to me. (By the way, shouldn't this have been unmarked unstable when that was changed to |
@eitsupi this wasn't a breaking change in the benchmarks. We also don't guarantee stability of in memory formats and we may export string views once consumers support them. For instance latest pyarrow can handle string views. That said. I don't want to leak this inner future to the users like in this PR. |
Could you explain more what you were doing in pyo3-polars? |
See pola-rs/pyo3-polars#85. I'm trying to add For example, a new version of polars introduces a new arrow data type, which is a breaking change to the data structure, so the flavor version increases ( |
Right, now I understand more. Note that we don't call that plugins, but rather pyo3 extensions. The plugins are versioned separately. Instead of an enum, can make it a struct with an internal version: |
Good idea. In this way, we can hide the implementation detail. |
This is undocumented. This allows easy calling `to_arrow()` from pyo3-polars
341f66a
to
9362a7f
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 @ruihe774.
I think it looks great now. I only have a few minor comments left and then it is good to go.
Previously,
future=True
renders data structure unstable. When exchanging data with e.g.pyo3-polars
, this can result in data corruption if the versions of polars do not match. This PR introduces versioning to "future" (in Python side) and to "pl_flavor" (in Rust side). This allows efficient data exchange and also allows future updates to the data structure.The current flavors are:
N.B. it is a breaking change to Rust polars. Python polars is backward compatible.