feat: Add backend argument to lazy#1895
Conversation
narwhals/dataframe.py
Outdated
| supported_backends = ( | ||
| Implementation.DASK, | ||
| Implementation.DUCKDB, | ||
| Implementation.POLARS, | ||
| ) |
There was a problem hiding this comment.
RIP Pyspark 😂 Happy to add it later no worries, I am just kidding!
There was a problem hiding this comment.
Sure, I will try to look into adding PySpark in a follow-up PR
| def lazy(self: Self, *, backend: Implementation | None = None) -> Self: | ||
| return self |
There was a problem hiding this comment.
LazyFrame does not have backend option anyway.
If it would, I feel like I rather raise if the passed backend here was different than None or Implementation.DUCKDB
There was a problem hiding this comment.
Thank you for the review, @FBruzzesi!
I added the backend argument to be able to run the following code:
import narwhals.stable.v1 as nw_v1
nw_v1.from_native(duckdb_rel).lazy()
which otherwise made the tests fail for V1. I also now added ValueError if backend is not None for DuckDB.
Co-authored-by: Francesco Bruzzesi <42817048+FBruzzesi@users.noreply.github.com>
for more information, see https://pre-commit.ci
…/lazy-backend-arg
FBruzzesi
left a comment
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks @raisadz , I left a tiny suggestion for DuckDB only.
In the meanwhile, I adjusted .collect to allow backend to take the following types: str | ModuleType | Implementation | None - see #1734
We might want to allow the same here? It can be a follow up PR anyway, but I just wanted to make you aware of it.
| # backwards compatibility because in `narwhals.stable.v1` | ||
| # function `.from_native()` will return a DataFrame for DuckDB. | ||
|
|
||
| if backend is not None: # pragma: no cover |
There was a problem hiding this comment.
I think duckdb should be allowed?
| if backend is not None: # pragma: no cover | |
| if backend not in (None, Implementation.DUCKDB): # pragma: no cover |
There was a problem hiding this comment.
i don't think it needs to be - if this path only exists for v1 backwards-compatibility, then #1895 (comment) is something somebody could have written, but they wouldn't have passed a backend, so raising for backend is not None wouldn't break anyone's code
MarcoGorelli
left a comment
MarcoGorelli
left a comment
There was a problem hiding this comment.
thanks @raisadz ! and @FBruzzesi for reviewing
3 participants
What type of PR is this? (check all applicable)
Related issues
backendargument tolazy#1889Checklist
If you have comments or can explain your changes, please do so below