feat: Allow nested structures in lit#3424
Conversation
narwhals/_pandas_like/utils.py
Outdated
| @no_type_check | ||
| def broadcast_series_to_index( |
There was a problem hiding this comment.
@dangotbanned I hope you can forgive me one day!
As commented in the description, I added all the relevant methods and properties, yet, as in #3398, the implementation_test.py fails for Modin
There was a problem hiding this comment.
Combining these guys should work 😄
- feat: Allow nested structures in
lit#3424 (comment) - feat: Allow nested structures in
lit#3424 (comment) - feat: Allow nested structures in
lit#3424 (comment)
It uses pd.arrays.ArrowExtensionArray which is marked as experimental
I wouldn't worry about pandas warning about experimental.
They've had that since introducing pyarrow stuff in (1.5.*), which was over 3 years ago
There was a problem hiding this comment.
Amazing! Thanks a ton @dangotbanned - applied all the suggestions in 8cf634b
I left one TODO regarding whether or not we should pass copy=False to pd.array
MarcoGorelli
left a comment
There was a problem hiding this comment.
seems reasonable, thanks @FBruzzesi !
| # Use ArrowExtensionArray to avoid pandas unpacking the nested structure | ||
| ns = self._implementation.to_native_namespace() | ||
| pandas_series_native = ns.Series( | ||
| pd.arrays.ArrowExtensionArray(pa_array), # type: ignore[attr-defined] | ||
| name="literal", | ||
| index=df._native_frame.index[0:1], | ||
| ) |
There was a problem hiding this comment.
I think the wrapping part of (#3424 (comment)) should be reused here.
Maybe broadcast_series_to_index is too specfic of a function?
The two more useful parts IMO are:
repeat- something related to reconstruction?
There was a problem hiding this comment.
Okay yeah so what I'm thinking is a new constructor (or two?) on PandasLikeSeries might work?
All of these are parts of a constructor for both native & compliant, and they're spread across 4 modules.
narwhals/narwhals/_pandas_like/dataframe.py
Line 308 in dd5c623
narwhals/narwhals/_pandas_like/namespace.py
Lines 86 to 87 in dd5c623
narwhals/narwhals/_pandas_like/series.py
Lines 204 to 206 in dd5c623
narwhals/narwhals/_pandas_like/utils.py
Line 668 in dd5c623
| def xfail_if_pyspark_connect( # pragma: no cover | ||
| constructor: Constructor, request: pytest.FixtureRequest, reason: str = "" | ||
| ) -> None: | ||
| if is_pyspark_connect(constructor): | ||
| request.applymarker(pytest.mark.xfail(reason=reason)) |
There was a problem hiding this comment.
| def xfail_if_pyspark_connect( # pragma: no cover | |
| constructor: Constructor, request: pytest.FixtureRequest, reason: str = "" | |
| ) -> None: | |
| if is_pyspark_connect(constructor): | |
| request.applymarker(pytest.mark.xfail(reason=reason)) | |
| request.applymarker(pytest.mark.xfail(is_pyspark_connect(constructor), reason=reason)) |
|
Note
|
- Fixed typo - Skip using `qualified_type_name` when we know it would be `builtins` (and get stripped anyway)
| ) | ||
|
|
||
| def lit(self, value: NonNestedLiteral, dtype: IntoDType | None) -> ArrowExpr: | ||
| def lit(self, value: PythonLiteral, dtype: IntoDType | None) -> ArrowExpr: |
There was a problem hiding this comment.
Well, isn't this a nice diff 😄
| return self._expr._from_elementwise_horizontal_op(func, *exprs) | ||
|
|
||
| def lit(self, value: Any, dtype: IntoDType | None) -> IbisExpr: | ||
| def lit(self, value: PythonLiteral, dtype: IntoDType | None) -> IbisExpr: |
| # Use ArrowExtensionArray to avoid pandas unpacking the nested structure | ||
| ns = self._implementation.to_native_namespace() | ||
| pandas_series_native = ns.Series( | ||
| pd.arrays.ArrowExtensionArray(pa_array), # type: ignore[attr-defined] | ||
| name="literal", | ||
| index=df._native_frame.index[0:1], | ||
| ) |
There was a problem hiding this comment.
Okay yeah so what I'm thinking is a new constructor (or two?) on PandasLikeSeries might work?
All of these are parts of a constructor for both native & compliant, and they're spread across 4 modules.
narwhals/narwhals/_pandas_like/dataframe.py
Line 308 in dd5c623
narwhals/narwhals/_pandas_like/namespace.py
Lines 86 to 87 in dd5c623
narwhals/narwhals/_pandas_like/series.py
Lines 204 to 206 in dd5c623
narwhals/narwhals/_pandas_like/utils.py
Line 668 in dd5c623
|
Thanks both for your reviews! I am going to merge so that it can make it into Monday's release (yes, it's time to make a new release!!!) @dangotbanned regarding #3424 (comment), we can follow up with a refactor. I am not 100% sure of what you have in mind 😂 so feel free to open a PR for it |


Description
I thought this was going to be more complex for more backends. The only troubled one is, as you might expect, pandas. That said, I am quite ok for how it turned out
Important
TODO:
probably some typing for pandas case. In my defence, each time I try to add anything to. Resolved in 8cf634b_BasePandasLikeSeriesor_BasePandasLikeFrame, a test case fails for modin. I had the same issue in feat: Addconcat(..., how="*_relaxed"})#3398I am no longer able to run it in kaggle due to dependencyCheck discord threadhellissues 😢What type of PR is this? (check all applicable)
Related issues
nw.lit#3408Checklist