refactor: adds `_Stores(Native|Compliant)` #2130

dangotbanned · 2025-03-02T14:30:06Z

What type of PR is this? (check all applicable)

Related issues

Checklist

Code follows style guide (ruff)
Tests added
Documented the changes

If you have comments or can explain your changes, please do so below

@FBruzzesi I've tested this out for some parts of _arrow over on main...stores-native-compliant

Really think it's a nice improvement to readabilty and DRYs things up well 🙂

With a lot of the repetition out of the way, I was able to spot a few more things that could be written more concisely.

Not sure if this is worth opening an issue for - but if we went ahead with it - then it'd probably make sense to try and do it in one or more big chunks to avoid conflicts

Note

The parts that I've touched in _arrow/*.py are ready for review.
Other modules have only had the bare-minimum changes to meet the new protocol(s)
The goal would be to repeat this kind of pattern throughout narwhals (pending feedback)

#1301 (comment)

That class was the motivating case in #1301 (comment)

Didn't think of it before, but all the namespace code can be reduced a lot with this

- Only the constructor needs to be implemented for each backend - To avoid #2064 (comment)

…iant

dangotbanned · 2025-03-02T14:35:59Z

TODO

Fix PolarsSeries
- (https://github.com/narwhals-dev/narwhals/actions/runs/13616393358/job/38059996590?pr=2130)

Important

I feel like this clearly demonstrates the benefit of protocols 😅
(342d629)

Wasn't planning to implement it yet but https://github.com/narwhals-dev/narwhals/actions/runs/13616393368/job/38059997094?pr=2130

Also adds coverage https://github.com/narwhals-dev/narwhals/actions/runs/13616480778/job/38060190836?pr=2130

- No changes to runtime - but these really ought to be checking the type actually has the attribute(s)

- Lots of long access paths cleaned up - Making it easier to spot other refactorings

…iant

dangotbanned · 2025-03-02T21:22:04Z

narwhals/utils.py

+class _StoresNative(Protocol[NativeT_co]):
+    @property
+    def native(self) -> NativeT_co:
+        """Return the native object."""
+        ...
+
+
+class _StoresCompliant(Protocol[CompliantT_co]):
+    @property
+    def compliant(self) -> CompliantT_co:
+        """Return the narwhals object."""
+        ...
+
+
+class _SeriesNamespace(  # type: ignore[misc]  # noqa: PYI046
+    _StoresCompliant[CompliantSeriesT_co],
+    _StoresNative[NativeT_co],
+    Protocol[CompliantSeriesT_co, NativeT_co],
+):
+    _compliant_series: CompliantSeriesT_co
+
+    @property
+    def compliant(self) -> CompliantSeriesT_co:
+        return self._compliant_series
+
+    @property
+    def native(self) -> NativeT_co:
+        return self._compliant_series.native
+
+    def from_native(self, series: Any, /) -> CompliantSeriesT_co:
+        return self.compliant._from_native_series(series)
+
+
+class _ExprNamespace(  # type: ignore[misc] # noqa: PYI046
+    _StoresCompliant[CompliantExprT_co], Protocol[CompliantExprT_co]
+):
+    _compliant_expr: CompliantExprT_co
+
+    @property
+    def compliant(self) -> CompliantExprT_co:
+        return self._compliant_expr


The core idea is providing (read-only) accessors and fast-paths to commonly used methods.

These are what I've felt were needed so far.

We can also but get more specific - like for temporal properties:

ArrowSeriesDateTimeNamespace

narwhals/narwhals/_arrow/series_dt.py

Lines 24 to 31 in 5c642eb

class ArrowSeriesDateTimeNamespace(ArrowSeriesNamespace):

@property

def unit(self) -> TimeUnit: # NOTE: Unsafe (native).

return cast("pa.TimestampType[TimeUnit, Any]", self.native.type).unit

@property

def time_zone(self) -> str | None: # NOTE: Unsafe (narwhals).

return cast("Datetime", self.compliant.dtype).time_zone

Ideally, I think these should not be used for methods accepting more than a single argument.
That way it keeps things pretty clean and doesn't introduce backend-specific details at this high of a level

…iant

dangotbanned · 2025-03-06T14:55:00Z

narwhals/_arrow/series_dt.py

    def millisecond(self: Self) -> ArrowSeries:
-        return self._compliant_series._from_native_series(
-            pc.millisecond(self._compliant_series._native_series)
-        )
+        return self.from_native(pc.millisecond(self.native))


This is the most typical change - where it becomes significantly eaiser to see the unique thing the method is doing

Before

def millisecond(self: Self) -> ArrowSeries: return self._compliant_series._from_native_series( pc.millisecond(self._compliant_series._native_series) )

After

def millisecond(self: Self) -> ArrowSeries: return self.from_native(pc.millisecond(self.native))

MarcoGorelli

thanks @dangotbanned ! love these simplifications

just one really minor comment (also, a merge conflict)

narwhals/utils.py

…iant

#2130 (comment)

Couldn't get VSCode to format the doc correctly when using "Examples:" #2130 (comment)

dangotbanned · 2025-03-08T12:00:14Z

narwhals/utils.py

+class _StoresNative(Protocol[NativeT_co]):
+    """Provides access to a native object.
+
+    Native objects are types like:
+
+    >>> from pandas import Series
+    >>> from pyarrow import Table
+    """


Important
Ignore the typo ("are" -> "have")
fixed in (0921f33)

100% expecting this doc to be questioned.

Was the only style that VSCode seemed to parse correctly:

Alternatives

- Bit confused as what I've done is fairly similar to (#2130) - Getting the same issue as (#2084) https://github.com/narwhals-dev/narwhals/actions/runs/13740460535/job/38429062404?pr=2149

dangotbanned added 8 commits February 26, 2025 17:37

chore: Add _StoresNative protocol

7935354

#1301 (comment)

refactor: replace ArrowSeries

732b78f

That class was the motivating case in #1301 (comment)

refactor: replace ArrowSeries*Namespace

6118d66

chore: add _StoresCompliant

1b5edad

Didn't think of it before, but all the namespace code can be reduced a lot with this

refactor: simplifying w/ ArrowSeriesNamespace

7b8e22f

refactor: add & replace w/ ArrowExprNamespace

f725248

refactor: move up a level, add PandasLikeSeries.native

fb1c9f2

- Only the constructor needs to be implemented for each backend - To avoid #2064 (comment)

Merge remote-tracking branch 'upstream/main' into stores-native-compl…

ee493b0

…iant

dangotbanned added the internal label Mar 2, 2025

dangotbanned added 7 commits March 2, 2025 14:40

test: Get coverage for PandasLikeSeries.native

3c0a9d4

Wasn't planning to implement it yet but https://github.com/narwhals-dev/narwhals/actions/runs/13616393368/job/38059997094?pr=2130

fix: Make PolarsSeries compliant

342d629

Also adds coverage https://github.com/narwhals-dev/narwhals/actions/runs/13616480778/job/38060190836?pr=2130

refactor: add _SeriesNamespace.from_native

d8df253

refactor: Simplify _arrow.series_dt

d99a8b4

- No changes to runtime - but these really ought to be checking the type actually has the attribute(s)

refactor: simplify _arrow.utils

2fe5a64

- Lots of long access paths cleaned up - Making it easier to spot other refactorings

Merge remote-tracking branch 'upstream/main' into stores-native-compl…

d7385bf

…iant

Merge remote-tracking branch 'upstream/main' into stores-native-compl…

66aab02

…iant

dangotbanned mentioned this pull request Mar 2, 2025

refactor: Simplify ArrowGroupBy.__iter__ #2133

Merged

10 tasks

dangotbanned added 2 commits March 2, 2025 19:23

Merge remote-tracking branch 'upstream/main' into stores-native-compl…

ffa5396

…iant

Merge branch 'main' into stores-native-compliant

5c642eb

dangotbanned changed the title ~~refactor(DRAFT): adds _Stores(Native|Compliant)~~ refactor(RFC): adds _Stores(Native|Compliant) Mar 2, 2025

dangotbanned commented Mar 2, 2025

View reviewed changes

Merge branch 'main' into stores-native-compliant

39d4be8

dangotbanned marked this pull request as ready for review March 3, 2025 18:32

dangotbanned added 4 commits March 3, 2025 18:32

Merge branch 'main' into stores-native-compliant

8192589

Merge branch 'main' into stores-native-compliant

ad62268

Merge branch 'main' into stores-native-compliant

3b467e1

Merge remote-tracking branch 'upstream/main' into stores-native-compl…

7c7a929

…iant

dangotbanned mentioned this pull request Mar 5, 2025

refactor: adds _compliant sub-package #2149

Open

10 tasks

dangotbanned requested a review from FBruzzesi March 5, 2025 20:40

dangotbanned added 2 commits March 6, 2025 11:03

Merge branch 'main' into stores-native-compliant

5ff4f7c

Merge remote-tracking branch 'upstream/main' into stores-native-compl…

6676341

…iant

dangotbanned commented Mar 6, 2025

View reviewed changes

MarcoGorelli approved these changes Mar 7, 2025

View reviewed changes

narwhals/utils.py Outdated Show resolved Hide resolved

dangotbanned added 3 commits March 8, 2025 11:25

Merge remote-tracking branch 'upstream/main' into stores-native-compl…

946b11c

…iant

docs: "narwhals object" -> "compliant object"

7c4dea3

#2130 (comment)

docs: Show examples of native/compliant

0d41a7a

Couldn't get VSCode to format the doc correctly when using "Examples:" #2130 (comment)

dangotbanned commented Mar 8, 2025

View reviewed changes

docs: fix typo

0921f33

dangotbanned changed the title ~~refactor(RFC): adds _Stores(Native|Compliant)~~ refactor: adds _Stores(Native|Compliant) Mar 8, 2025

dangotbanned merged commit 90ca761 into main Mar 8, 2025
30 checks passed

dangotbanned deleted the stores-native-compliant branch March 8, 2025 12:20

dangotbanned mentioned this pull request Mar 9, 2025

feat: Added struct namespace with field method. #2146

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: adds `_Stores(Native|Compliant)` #2130

refactor: adds `_Stores(Native|Compliant)` #2130

dangotbanned commented Mar 2, 2025 •

edited

Loading

dangotbanned commented Mar 2, 2025 •

edited

Loading

dangotbanned Mar 2, 2025 •

edited

Loading

dangotbanned Mar 6, 2025

MarcoGorelli left a comment •

edited

Loading

dangotbanned Mar 8, 2025 •

edited

Loading

	class ArrowSeriesDateTimeNamespace(ArrowSeriesNamespace):
	@property
	def unit(self) -> TimeUnit: # NOTE: Unsafe (native).
	return cast("pa.TimestampType[TimeUnit, Any]", self.native.type).unit

	@property
	def time_zone(self) -> str \| None: # NOTE: Unsafe (narwhals).
	return cast("Datetime", self.compliant.dtype).time_zone

refactor: adds _Stores(Native|Compliant) #2130

refactor: adds _Stores(Native|Compliant) #2130

Conversation

dangotbanned commented Mar 2, 2025 • edited Loading

What type of PR is this? (check all applicable)

Related issues

Checklist

If you have comments or can explain your changes, please do so below

dangotbanned commented Mar 2, 2025 • edited Loading

TODO

dangotbanned Mar 2, 2025 • edited Loading

Choose a reason for hiding this comment

dangotbanned Mar 6, 2025

Choose a reason for hiding this comment

Before

After

MarcoGorelli left a comment • edited Loading

Choose a reason for hiding this comment

dangotbanned Mar 8, 2025 • edited Loading

Choose a reason for hiding this comment

refactor: adds `_Stores(Native|Compliant)` #2130

refactor: adds `_Stores(Native|Compliant)` #2130

dangotbanned commented Mar 2, 2025 •

edited

Loading

dangotbanned commented Mar 2, 2025 •

edited

Loading

dangotbanned Mar 2, 2025 •

edited

Loading

MarcoGorelli left a comment •

edited

Loading

dangotbanned Mar 8, 2025 •

edited

Loading