-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: dagster-deltalake-polars LazyFrame support + expose all writer params #19343
Conversation
@roeap Would you be open to reviewing this change for LazyFrame support? |
@PedramNavid I am trying to setup dagster repo and env properly, I followed step by step the contributing guide on the website but pyright just does not work at all. Whatever I do, pylance/pyright doesn't pick up anything: I've been trying to reach out on slack but there seems no channel where these questions can properly land, do you know what to by any chance? It previously worked pylance, but after refreshing the language server it stopped working |
@ion-elgreco Can you message me directly on our Community Slack? It might be easier to troubleshoot that way. |
@PedramNavid yes, I'll see that I get to it this we! |
e58956a
to
c56f856
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.
Thanks @ion-elgreco, looking great. Very much appreciate the nice tests as well! Just left some minor style comments.
...libraries/dagster-deltalake-polars/dagster_deltalake_polars/deltalake_polars_type_handler.py
Outdated
Show resolved
Hide resolved
...ries/dagster-deltalake-polars/dagster_deltalake_polars_tests/test_type_handler_save_modes.py
Outdated
Show resolved
Hide resolved
python_modules/libraries/dagster-deltalake/dagster_deltalake/handler.py
Outdated
Show resolved
Hide resolved
python_modules/libraries/dagster-deltalake/dagster_deltalake/handler.py
Outdated
Show resolved
Hide resolved
@roeap applied all the requested changes :) |
@PedramNavid - from my end this can be merged! |
@PedramNavid could we get this merged? My team would like to start getting their hands on it as soon as possible :) |
Sorry I've been off this week! I just reviewed as well, and code looks good. Unblocked the tests, if they pass we can merge! |
@PedramNavid could you retrigger, I didn't see I also had to separately bump the requipements-pinned.txt, this is also updated now. |
@PedramNavid should be fine now. Maybe it's easier if you allow me to trigger the CI myself Also, the failing doc-tests seem unrelated to this PR |
…feat/deltalake_v2
Can you merge from master and try again? This solves that issue: #19455 |
@PedramNavid done |
merged! thanks for bearing with us |
@PedramNavid thanks! :) I see there was a release but didn't include my changes? I guess I missed the release window there |
That’s right, we cut the release on Wednesday, so this will go out with the next scheduled release |
Summary & Motivation
Added LazyFrame support in dagster-deltalake-polars integration to allow for lazy queries which will do predicate and projection pushdown to the storage layer.
How I Tested These Changes
Extended the tests and parametrized them.