Repository navigation
feat: support list.{len,get,min,max,mean,median,sum,sort} for Dask - #4017
jonasdedden wants to merge 2 commits into
Conversation
|
@FBruzzesi this really was minimal work required to get almost all of the List namespace working for Dask 😁 |
FBruzzesi
added
enhancement
FBruzzesi
left a comment
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks @jonasdedden - I agree that the implementation is quite straightforward and low effort to maintain.
After the suggested change in src/narwhals/_dask/expr_list.py I asked Claude for a review and it spotted an issue for the case in which there is a non-null last row and pandas<3.0.0 as pandas' own list[i] accessor returns a fresh 0-based index on 2.2.3 and 2.3.3 (fixed in 3.0). Dask lines partitions up by index, so every partition after the first came back as nulls, and there is nothing in CI to catch that.
The suggested fixes are:
diff --git a/src/narwhals/_pandas_like/series_list.py b/src/narwhals/_pandas_like/series_list.py
index 8f69ac08f..3b0843bfe 100644
--- a/src/narwhals/_pandas_like/series_list.py
+++ b/src/narwhals/_pandas_like/series_list.py
@@ -48,6 +48,10 @@ class PandasLikeSeriesListNamespace(
def get(self, index: int) -> PandasLikeSeries:
result = self.native.list[index]
+ implementation, backend_version = self.implementation, self.backend_version
+ if implementation.is_pandas() and backend_version < (3, 0): # pragma: no cover
+ # `result` is a new object so it's safe to do this inplace.
+ result.index = self.native.index
result.name = self.native.name
return self.with_native(result)and a test to check it:
diff --git a/tests/expr_and_series/list/get_test.py b/tests/expr_and_series/list/get_test.py
index 8a3e4ea41..be8cc607e 100644
--- a/tests/expr_and_series/list/get_test.py
+++ b/tests/expr_and_series/list/get_test.py
@@ -8,10 +8,10 @@ import pytest
import narwhals as nw
from tests.utils import PANDAS_VERSION, Constructor, ConstructorEager, assert_equal_data
-data = {"a": [[1, 2], [None, 3], [None], None]}
+data = {"a": [[1, 2], [None, 3], [None], None, [4]]}
-@pytest.mark.parametrize(("index", "expected"), [(0, {"a": [1, None, None, None]})])
+@pytest.mark.parametrize(("index", "expected"), [(0, {"a": [1, None, None, None, 4]})])
def test_get_expr(
request: pytest.FixtureRequest, constructor: Constructor, index: int, expected: Any
) -> None:
@@ -30,7 +30,7 @@ def test_get_expr(
assert_equal_data(result, expected)
-@pytest.mark.parametrize(("index", "expected"), [(0, {"a": [1, None, None, None]})])
+@pytest.mark.parametrize(("index", "expected"), [(0, {"a": [1, None, None, None, 4]})])
def test_get_series(
request: pytest.FixtureRequest,
constructor_eager: ConstructorEager,
…st partitions Address review on narwhals-dev#4017: - pandas<3 `list[i]` returns a fresh 0-based index, which made every Dask partition after the first come back as nulls; restore the original index. - Extend `list.get` test data with a non-null last row to cover this. - Replace `*args/**kwargs` + `getattr` dispatch in the Dask list namespace with callables. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGTmcygmaAm4q8hXVvjH4C
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (10)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughDask list expressions now support ChangesDask list operations
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant DaskExprListNamespace
participant Dask map_partitions
participant PandasLikeSeries
DaskExprListNamespace->>Dask map_partitions: Apply the shared partition adapter with explicit metadata
Dask map_partitions->>PandasLikeSeries: Wrap each pandas partition and run the list operation
PandasLikeSeries-->>Dask map_partitions: Return the native pandas series
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds Dask support for several list operations and fixes index handling for list get on older pandas versions. No merge-blocking risk was identified. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The new operations reuse existing list computations within Dask’s established partition-execution path. The reviewed changes do not introduce a material security risk or grant new privileges. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@FBruzzesi thanks so much! just FYI, sadly I'm not at a computer right now, I let Claude do things, the CI failure seems to be not because of this PR: (Claude output raw pasted) The pointblank failure is not caused by this PR: the same tests fail identically on narwhals
fix: make the pandas-like |
|
Thanks for investigating this. It seems that we are not testing |
|
Again, feel free to work on this PR if you want 🫶 Could potentially be that I won't have much capacity this week |
This branch has not been deployed
3 participants
Description
Adds
list.len,list.get,list.min,list.max,list.mean,list.median,list.sumandlist.sortfor Dask. Onlylist.uniqueremains unimplemented, as it is for pandas and PyArrow.Each partition is wrapped in a
PandasLikeSeriesand the call isforwarded to the existing pandas.listmethod, so Dask reuses the pandas implementation (and its pyarrow helpers) instead of adding new list logic.list.containsnow goes through the same path.metais computed by running the same method on the empty_metainstead of being inferred by Dask, because inference would wrap errors raised on user input (e.g. theInvalidOperationErrorfor a mismatchedlist.containsitem from #3915) in aValueError.As with pandas, only pyarrow-backed lists are supported, and
list.medianis pyarrow's approximate median. The tests skip Dask on pandas<2.2, as they already do for pandas, since casting toListneeds it.What type of PR is this? (check all applicable)
Related issues
list.containsfor PyArrow, pandas and Dask #4001InvalidOperationErrorfor mismatchedlist.containsitems on pyarrow-backed backends #3915AI assistance
Checklist
Code follows style guide (ruff)
Tests added
Documented the changes (N/A, the API docs already list these methods)
If this is your first PR to narwhals, attach a screenshot of
pytestpassing locally (not CI):Summary by CodeRabbit