Adapt to polars upstream changes and turn on CI testing #16081
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wence-
added
improvement
github-actions
bot
added
Python
wence-
commented
Jun 25, 2024
wence-
force-pushed
the
wence/fea/adapt-polars-upstream
branch
3 times, most recently
from
0431028 to
80add5e
Compare
wence-
added
improvement
wence-
force-pushed
the
wence/fea/adapt-polars-upstream
branch
from
80add5e to
12c212a
Compare
lithomas1
reviewed
Jun 25, 2024
wence-
force-pushed
the
wence/fea/adapt-polars-upstream
branch
from
40c67eb to
dee33d2
Compare
|
OK, the test aspects are working, but it turns out I have a load of python 3.10isms which need to be fixed... |
wence-
force-pushed
the
wence/fea/adapt-polars-upstream
branch
from
946550e to
e3ddf7a
Compare
|
Ready for a proper look. |
wence-
commented
Jun 26, 2024
wence-
commented
Jun 26, 2024
wence-
commented
Jun 26, 2024
wence-
commented
Jun 26, 2024
wence-
force-pushed
the
wence/fea/adapt-polars-upstream
branch
from
e3ddf7a to
6047282
Compare
lithomas1
approved these changes
Jun 26, 2024
|
I have a couple more questions above (maybe more for the CI people than for you) for the CI side of things, but I'll let the CI reviewer handle reviews for those parts of this PR in more detail. |
jameslamb
approved these changes
Jun 26, 2024
There was a problem hiding this comment.
I reviewed this from the CI/packaging perspective and left some recommendations about simplifying how the polars dependency is managed.
Marking this "Approve" so you can do what you want with them without my review blocking merge... since my recommendations are about simplification and not correctness.
New feature in Python 3.10, so we need the backport while we support 3.9.
This only exists in Python >= 3.10
The short X | Y syntax is Python >= 3.10 only.
Explicitly set dtype when we want nans in our output.
The broadcasting rules are not very clear on the polars side, so let's just defer to them. This should be a niche use case anyway: joining against a literal is better written as a filter.
This is closer to the SQL behaviour and was changed by polars in pola-rs/polars#17061.
Now we don't have literal keys we can simplify the broadcast, but add note referencing polars issue on clarifications.
Duplicate output names need to raised by us.
wence-
force-pushed
the
wence/fea/adapt-polars-upstream
branch
from
f12d7be to
f4c2168
Compare
|
/merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
They changed the semantics of join keys when those keys are expressions to more closely match SQL.
Dtype inference is also tighter, so update tests to adapt to those changes, and some other small deprecation warnings.
Finish the final missing coverage piece and turn on testing in CI (failing if we don't hit 100% coverage as well).
Checklist