Bump Polars to v0.41.3 #917
Conversation
lkarthee
force-pushed
the
ps-bump-polars-to-v0.39.2
branch
from
f5ed00c
to
fcd8678
Compare
lkarthee
force-pushed
the
ps-bump-polars-to-v0.39.2
branch
from
5fde410
to
66ee582
Compare
There was a problem hiding this comment.
Finished in 3.5 seconds (3.5s async, 0.00s sync)
610 doctests, 1 property, 1485 tests, 71 failures, 20 excluded, 1 skipped
Randomized with seed 279745
Most of the test cases fail because:
- pow dtype change
- pivot_wider, pivot_longer
- string slice
- cross join param
- window functions
- commented few tests which are crashing polars
This is to avoid Polars to choose a different type.
This reverts commit 2644d27.
If exponent is float, it follows dtype of exponent. Otherwise, it follows dtype of base. See: pola-rs/polars#15506
|
We are almost there! But a problem regarding the cloud integration is still blocking this. I'm trying to fix it by updating |
This makes the "ObjectStore" choose to upload using a single request or a multi request (multi part) upload depending on the size of the chunks. This change is needed because Polars is now calling the writer without buffering, so this was breaking the upload. There are two tests failing for different reasons: - the IPC upload to a unknown bucket is failing because the new CloudWriter is not propagating the error. This is probably an easy fix. - the Lazy "parquet to cloud" is failing for the reason I wrote above. This is probably related to this issue: pola-rs/polars#17172
This is an attempt to fix the issues with the sink_parquet_cloud.
The hope is that the new version has fixed the issue.
This reverts commit be9a236.
The idea is to avoid depending on shutting down only on "Drop". This is also important because we need to verify if the file was created.
|
There is a good news and a bad news. The good news is that we have only one failing test! 🎉 🥳 In a nutshell, the problem is on the We can either wait for the fix to land on Polars, or deactivate the feature until there. This is going to affect people using the EDIT: I'm going to play with an idea to use the |
|
What do we for when streaming is not supported? We raise or we just do the best we can? I am worried Polars is not ultimately interested in maintaining these features. We have another related PR that is open for quite some time. |
I think we can raise if it's not possible to support a given feature, but we can try to implement "hacks" when possible, until we need to remove the feature all together. Unfortunately we can't have our patches (via monkey patches) to the Polars codebase, and I don't think it's a good idea to maintain a fork to solve our issues. WDYT?
Yeah, I'm worried too. Maybe it's not their focus to maintain this cloud integration in their Rust codebase. |
|
Yeah, let's raise for now and add a note to the CHANGELOG. Meanwhile, if you could send a PR to them, it would be great! |
The reason it didn't work is that the sink_parquet function will try to create the file where the "fifo" file is located. This is not going to work.
This reverts commit d64af7e.
|
@josevalim OK, I will add a note to the change log later. My PoC didn't work out, but I was wondering if we could still "enable streaming", but first stream to a temporary file, and them read from that file and send to the cloud. This could keep the convenience of saving to S3 right from Explorer, with the downside of requiring some space in disk, and probably extra time. WDYT?
Yeah, I will try that! Something similar to our |
|
Let’s just disable for now. People can stay in the old version until Polars is updated with a fix. |
|
+1 for disabling and we should document it in the changelog. |
There was a problem hiding this comment.
Looks great! I rewrote the changelog a tiny bit so each bullet was similarly worded (and fixed a typo).
Also I'll be honest, I skimmed the Rust code. But my Rust skills are nascent so I'm only sorta helpful on that side of the house anyway 😅. The Elixir side seems solid, though!
Co-authored-by: Billy Lanchantin <[email protected]>
No description provided.