chore: fixup operator type ignores for `SQLExpr` #2920

ym-pett · 2025-08-01T14:35:06Z

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

Related issues

Related issue #<issue number>
Closes #<issue number>

Checklist

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

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

narwhals/_sql/typing.py

MarcoGorelli · 2025-08-01T16:00:25Z

narwhals/_sql/typing.py

+    # not sure how to initialise the class, I want it to have all the methods etc of SQLExprT and add its own operator
+    # methods in addition to those
+    def __init__(self, other: [Int | Float]) -> None:
+        super().__init__(other)


this class will only be used for type-checking purposes, so there's no need to fill in the implementations for any methods, in particular, you don't need to define __init__

for the others, you can just return ...

narwhals/_sql/typing.py

MarcoGorelli · 2025-08-02T08:47:18Z

narwhals/_sql/typing.py

+
+class NativeSQLExpr(NativeExpr):
+    # TODO @mp: fix input type for all these!
+    def __gt__(self, value: float) -> Boolean: ...


the return type should be Self

In [11]: import duckdb In [12]: duckdb.ColumnExpression('a') == 3 Out[12]: (a = 3) In [13]: type(duckdb.ColumnExpression('a') == 3) Out[13]: duckdb.duckdb.Expression

when you do arithmetic / comparisons between these expressions, the output is still an expression

ah thanks for that, now I understand why Self!

ym-pett · 2025-08-04T08:16:45Z

pytests seem to suggest something going on with the arguments to NativeSQLExprT, I think I do need to define the arguments by making a NativeSQLExprTAny; looking into this!

but NativeSQLExpr doesn't take any arguments, so unlikely to be the solution..

MarcoGorelli · 2025-08-04T09:35:32Z

I think I do need to define the arguments by making a NativeSQLExprTAny; looking into this!

yup, that's right!

but NativeSQLExpr doesn't take any arguments, so unlikely to be the solution..

~~NativeSQLExpr doesn't, but NativeSQLExprT does~~

EDIT: discussed over a call how to resolve this

ym-pett · 2025-08-04T09:57:46Z

I think I do need to define the arguments by making a NativeSQLExprTAny; looking into this!

yup, that's right!

but NativeSQLExpr doesn't take any arguments, so unlikely to be the solution..

NativeSQLExpr doesn't, but NativeSQLExprT does

ah just seen this, thanks!

ym-pett · 2025-08-04T15:28:44Z

hmm i can see why the typing errors happen, i'm not sure how to fix though. I'll add comments to the files where the errors are flagged.

for more information, see https://pre-commit.ci

ym-pett · 2025-08-05T10:02:25Z

2 ibis type errors remain - don't know if my technique is right, is there a better way?

MarcoGorelli · 2025-08-05T13:22:54Z

I'm seriously tempted to say that we should just switch

class IbisExpr(SQLExpr["IbisLazyFrame", "ir.Value"])

to

class IbisExpr(SQLExpr["IbisLazyFrame", Any])

Afterall, Ibis doesn't export its type annotations, nor do they even check them, so I don't think it's worth bending backwards over their non-public types anyway. We're having to introduce cast / type: ignore in every other ibis function anyway... 😩

cc @dangotbanned in case you have thoughts

ym-pett · 2025-08-05T14:31:00Z

re going for an 'Any' type, let me know if/when I can implement or if I should try to find another fix

dangotbanned · 2025-08-05T14:39:26Z

Thanks for the ping @MarcoGorelli

Let me have a think on it today

dangotbanned · 2025-08-05T15:57:08Z

Afterall, Ibis doesn't export its type annotations, nor do they even check them, so I don't think it's worth bending backwards over their non-public types anyway.
We're having to introduce cast / type: ignore in every other ibis function anyway... 😩

Let me have a think on it today

My preference would be to open issue(s) upstream first.
Back in (#2000 (comment)), I noticed that a lot of our ibis typing woes would be fixed by some methods using Self as the return type.

These operator issues would also fall into that category as we want the methods defined in https://github.com/ibis-project/ibis/blob/db1a727b3c4c75e8e4af0f7ef3d0101d26d4450b/ibis/expr/types/numeric.py and not ir.Value itself.

From the looks of (ibis-project/ibis#11511), I'm getting the feeling that the ibis maintainers might be open to improvements if we propose them 🙏

We do have the benefit of an entire sub-package documenting where we've struggled 😂

If we don't have any success there, another option is isolating the problematic paths and wrapping them with our own annotations.
I've done this a lot in (https://github.com/narwhals-dev/narwhals/blob/a8b89fd268f2031b52dfe0548e728fcacbc90a04/narwhals/_arrow/utils.py), (https://github.com/narwhals-dev/narwhals/blob/a8b89fd268f2031b52dfe0548e728fcacbc90a04/narwhals/_arrow/typing.py), but there's stuff like this all over narwhals

Show examples

narwhals/narwhals/_pandas_like/utils.py

Lines 396 to 404 in a8b89fd

    
           # NOTE: Use this to avoid annotating inline 
        
           def iter_dtype_backends( 
        
               dtypes: Iterable[Any], implementation: Implementation 
        
           ) -> Iterator[DTypeBackend]: 
        
               """Yield a `DTypeBackend` per-dtype. 
        
               Matches pandas' `dtype_backend` argument in `convert_dtypes`. 
        
               """ 
        
               return (get_dtype_backend(dtype, implementation) for dtype in dtypes)

narwhals/narwhals/_pandas_like/utils.py

Line 657 in a8b89fd

def import_array_module(implementation: Implementation, /) -> ModuleType:

narwhals/narwhals/_pandas_like/namespace.py

Lines 374 to 410 in a8b89fd

    
           class _NativeConcat(Protocol[NativeDataFrameT, NativeSeriesT]): 
        
               @overload 
        
               def __call__( 
        
                   self, 
        
                   objs: Iterable[NativeDataFrameT], 
        
                   *, 
        
                   axis: _Vertical, 
        
                   copy: bool | None = ..., 
        
               ) -> NativeDataFrameT: ... 
        
               @overload 
        
               def __call__( 
        
                   self, objs: Iterable[NativeSeriesT], *, axis: _Vertical, copy: bool | None = ... 
        
               ) -> NativeSeriesT: ... 
        
               @overload 
        
               def __call__( 
        
                   self, 
        
                   objs: Iterable[NativeDataFrameT | NativeSeriesT], 
        
                   *, 
        
                   axis: _Horizontal, 
        
                   copy: bool | None = ..., 
        
               ) -> NativeDataFrameT: ... 
        
               @overload 
        
               def __call__( 
        
                   self, 
        
                   objs: Iterable[NativeDataFrameT | NativeSeriesT], 
        
                   *, 
        
                   axis: Axis, 
        
                   copy: bool | None = ..., 
        
               ) -> NativeDataFrameT | NativeSeriesT: ... 
        
               def __call__( 
        
                   self, 
        
                   objs: Iterable[NativeDataFrameT | NativeSeriesT], 
        
                   *, 
        
                   axis: Axis, 
        
                   copy: bool | None = None, 
        
               ) -> NativeDataFrameT | NativeSeriesT: ...

narwhals/narwhals/_namespace.py

Lines 102 to 150 in a8b89fd

    
           class _BasePandasLike(Sized, Protocol): 
        
               index: Any 
        
               """`mypy` doesn't like the asymmetric `property` setter in `pandas`.""" 
        
               def __getitem__(self, key: Any, /) -> Any: ... 
        
               def __mul__(self, other: float | Collection[float] | Self) -> Self: ... 
        
               def __floordiv__(self, other: float | Collection[float] | Self) -> Self: ... 
        
               @property 
        
               def loc(self) -> Any: ... 
        
               @property 
        
               def shape(self) -> tuple[int, ...]: ... 
        
               def set_axis(self, labels: Any, *, axis: Any = ..., copy: bool = ...) -> Self: ... 
        
               def copy(self, deep: bool = ...) -> Self: ...  # noqa: FBT001 
        
               def rename(self, *args: Any, inplace: Literal[False], **kwds: Any) -> Self: 
        
                   """`inplace=False` is required to avoid (incorrect?) default overloads.""" 
        
                   ... 
        
           class _BasePandasLikeFrame(NativeFrame, _BasePandasLike, Protocol): ... 
        
           class _BasePandasLikeSeries(NativeSeries, _BasePandasLike, Protocol): 
        
               def where(self, cond: Any, other: Any = ..., **kwds: Any) -> Any: ... 
        
           class _NativeDask(Protocol): 
        
               _partition_type: type[pd.DataFrame] 
        
           class _CuDFDataFrame(_BasePandasLikeFrame, Protocol): 
        
               def to_pylibcudf(self, *args: Any, **kwds: Any) -> Any: ... 
        
           class _CuDFSeries(_BasePandasLikeSeries, Protocol): 
        
               def to_pylibcudf(self, *args: Any, **kwds: Any) -> Any: ... 
        
           class _NativeIbis(Protocol): 
        
               def sql(self, *args: Any, **kwds: Any) -> Any: ... 
        
               def __pyarrow_result__(self, *args: Any, **kwds: Any) -> Any: ... 
        
               def __pandas_result__(self, *args: Any, **kwds: Any) -> Any: ... 
        
               def __polars_result__(self, *args: Any, **kwds: Any) -> Any: ... 
        
           class _ModinDataFrame(_BasePandasLikeFrame, Protocol): 
        
               _pandas_class: type[pd.DataFrame] 
        
           class _ModinSeries(_BasePandasLikeSeries, Protocol): 
        
               _pandas_class: type[pd.Series[Any]] 
        
           _NativePolars: TypeAlias = "pl.DataFrame | pl.LazyFrame | pl.Series" 
        
           _NativeArrow: TypeAlias = "pa.Table | pa.ChunkedArray[Any]" 
        
           _NativeDuckDB: TypeAlias = "duckdb.DuckDBPyRelation" 
        
           _NativePandas: TypeAlias = "pd.DataFrame | pd.Series[Any]" 
        
           _NativeModin: TypeAlias = "_ModinDataFrame | _ModinSeries" 
        
           _NativeCuDF: TypeAlias = "_CuDFDataFrame | _CuDFSeries"

narwhals/narwhals/_spark_like/namespace.py

Lines 53 to 60 in a8b89fd

    
           def _F(self):  # type: ignore[no-untyped-def] # noqa: ANN202, N802 
        
               if TYPE_CHECKING: 
        
                   from sqlframe.base import functions 
        
                   return functions 
        
               return import_functions(self._implementation) 
        
           @property

While this is an option, I wouldn't suggest it until we know that the only alternative is typing everything as Any

MarcoGorelli · 2025-08-07T10:49:18Z

All I can think to suggest is, in narwhals/_ibis/typing.py, to introduce

class Expression(ir.Value, Protocol):
    def __ge__(self, other: Any, /) -> Expression: ...
    def __le__(self, other: Any, /) -> Expression: ...
    # etc.

and fill in the missing bits

Alternatively, use Any when defining the classes, but keep ir.Column / ir.NumericColumn in some methods

perhaps i'll give this a go later

dangotbanned · 2025-08-07T10:58:42Z

All I can think to suggest is, in narwhals/_ibis/typing.py, to introduce
class Expression(ir.Value, Protocol):

Sadly this won't work IIRC, as a Protocol can't inherit from a non-Protocol class 😔

MarcoGorelli · 2025-08-07T11:23:48Z

🤔 i'm wondering if Deferred would be a better candidate than ir.Value, as that's closer to Expression / Column

In [1]: import ibis

In [2]: ibis._['a']
Out[2]: _['a']

In [3]: type(_)
Out[3]: ibis.common.deferred.Deferred

Replaces them with calls to `operator` functions, which use `Any`

dangotbanned · 2025-08-07T16:29:24Z

@MarcoGorelli, @ym-pett

How do you feel about this?

fix(suggestion): Remove problematic protocol members

I've tried to keep the terseness of using operators through aliasing.

This works due to most of these functions being typed more permissively

https://github.com/python/typeshed/blob/ecf2ef48f96f596c4ff4374161675596b9c8c569/stdlib/_operator.pyi

dangotbanned · 2025-08-07T16:58:34Z

narwhals/_compliant/expr.py

+    # NOTE: None of these are annotated for `dx.Series`, but are added imperatively
+    # Probably better to define a sub-protocol for `NativeSQLExpr`
+    # - match `dx.Series` to `NativeExpr`
+    # - match the others to `NativeSQLExpr`
+    def __gt__(self, value: Any, /) -> Self: ...
+    def __lt__(self, value: Any, /) -> Self: ...
+    def __ge__(self, value: Any, /) -> Self: ...
+    def __le__(self, value: Any, /) -> Self: ...
+    def __eq__(self, value: Any, /) -> Self: ...  # type: ignore[override]
+    def __ne__(self, value: Any, /) -> Self: ...  # type: ignore[override]


For context:

https://github.com/dask/dask/blob/d0a9d158ff1dc8d29145311b81365c28ee82474c/dask/dataframe/dask_expr/_collection.py#L4609-L4632

https://github.com/dask/dask/blob/d0a9d158ff1dc8d29145311b81365c28ee82474c/dask/dataframe/dask_expr/_collection.py#L191-L280

MarcoGorelli

looks good to me, thanks @dangotbanned

So, the idea is to put the comparison operators in NativeExpr, and for the arithmetic ones we just use operator? And just ignore for Dask, which implements these in a hacky way anyway

@ym-pett I think if you clean up the temporary comments, then I think we can ship this

MarcoGorelli · 2025-08-07T17:20:43Z

narwhals/_compliant/expr.py

+    # NOTE: None of these are annotated for `dx.Series`, but are added imperatively
+    # Probably better to define a sub-protocol for `NativeSQLExpr`
+    # - match `dx.Series` to `NativeExpr`
+    # - match the others to `NativeSQLExpr`
+    def __gt__(self, value: Any, /) -> Self: ...
+    def __lt__(self, value: Any, /) -> Self: ...
+    def __ge__(self, value: Any, /) -> Self: ...
+    def __le__(self, value: Any, /) -> Self: ...
+    def __eq__(self, value: Any, /) -> Self: ...  # type: ignore[override]
+    def __ne__(self, value: Any, /) -> Self: ...  # type: ignore[override]


looks good, thanks - if anything, I like it better if we match on these than if we match on between and isin (which, for example, Daft doesn't have)

This is the important bit

narwhals/narwhals/_compliant/expr.py

Lines 57 to 62 in 5bcdb6e

class NativeExpr(Protocol):

"""An `Expr`-like object from a package with [Lazy-only support](https://narwhals-dev.github.io/narwhals/extending/#levels-of-support).

Protocol members are chosen *purely* for matching statically - as they

are common to all currently supported packages.

"""

which, for example, Daft doesn't have

I had a similar issue for ibis.Table in (#2944)

narwhals/narwhals/typing.py

Line 124 in 0d4b6f7

IntoLazyFrame: TypeAlias = Union["NativeLazyFrame", "_NativeIbis"]

It's okay to have multiple protocols/sub-protocols/aliases if we need that now

I started with 1 because we had a common denominator, but we grow 😄

had a common denominator

right, so if we add these comparison operators, we can remove isin and between?

Keep between if possible

https://docs.daft.ai/en/stable/api/expressions/#daft.expressions.Expression.between

Kind of like how we have filter for NativeSeries - it's something to avoid false-positives

Waaaaay more than just the types we're interested in will have comparison dunders, but fewer will also have between as well

narwhals/_compliant/expr.py

MarcoGorelli

thanks both

merging then so we can make progress on extensions

ym-pett added 2 commits August 1, 2025 14:25

temp fix to type problem

40e1737

notes on what should happen

a8af821

MarcoGorelli reviewed Aug 1, 2025

View reviewed changes

corrected base class

f8e853c

ym-pett force-pushed the fix_sql_operator_problem branch from 2bd2b9b to f8e853c Compare August 1, 2025 16:13

wip experimenting with methods

fda9a7d

MarcoGorelli reviewed Aug 1, 2025

View reviewed changes

narwhals/_sql/typing.py Outdated Show resolved Hide resolved

wip improved types

86ed14a

MarcoGorelli reviewed Aug 2, 2025

View reviewed changes

ym-pett added 2 commits August 3, 2025 17:08

wip operator

ec08949

wip operator: added thoughts

a4de0bb

ym-pett force-pushed the fix_sql_operator_problem branch from d3af7d3 to a4de0bb Compare August 3, 2025 17:04

wip operator: modified Self import

d372db6

ym-pett force-pushed the fix_sql_operator_problem branch from 9377eb7 to d372db6 Compare August 4, 2025 08:08

wip operator: added thoughts & only using in sqrt

3a97dcb

ym-pett force-pushed the fix_sql_operator_problem branch from efcbd68 to 3a97dcb Compare August 4, 2025 09:55

wip operator: more comments, going back to reading

9a544e8

ym-pett force-pushed the fix_sql_operator_problem branch from f540cd8 to 9a544e8 Compare August 4, 2025 10:20

wip operator: fixed NativeSQLExprT bound

266cab9

ym-pett force-pushed the fix_sql_operator_problem branch from c0e19c5 to 266cab9 Compare August 4, 2025 12:02

wip operator: adding NativeSQLExprT to get rid of operator issue

4aae069

ym-pett force-pushed the fix_sql_operator_problem branch from 660b4e3 to 4aae069 Compare August 4, 2025 12:23

wip operator: added all changes from pair session, 1 problem remaining

7ad112a

ym-pett force-pushed the fix_sql_operator_problem branch from 247ad11 to 7ad112a Compare August 4, 2025 13:45

checkpoint

937e2a1

ym-pett force-pushed the fix_sql_operator_problem branch from 23eb022 to 937e2a1 Compare August 4, 2025 14:07

ym-pett and others added 4 commits August 5, 2025 10:40

Merge branch 'main' into fix_sql_operator_problem

c65df29

[pre-commit.ci] auto fixes from pre-commit.com hooks

dd39727

for more information, see https://pre-commit.ci

removing commented out

09731da

trying to silence ibis type errors

b4b48d9

annoying stray comment

a131d51

Merge branch 'main' into fix_sql_operator_problem

3521336

dangotbanned mentioned this pull request Aug 7, 2025

Tracking: ibis typing fixes #2960

Open

1 task

fix(suggestion): Remove problematic protocol members

dcf1b80

Replaces them with calls to `operator` functions, which use `Any`

dangotbanned reviewed Aug 7, 2025

View reviewed changes

MarcoGorelli reviewed Aug 7, 2025

View reviewed changes

took out wip comment

e6f7640

ym-pett marked this pull request as ready for review August 8, 2025 09:27

ym-pett added 2 commits August 8, 2025 10:27

Merge branch 'main' into fix_sql_operator_problem

5347b9f

Merge branch 'main' into fix_sql_operator_problem

b17d241

MarcoGorelli reviewed Aug 8, 2025

View reviewed changes

narwhals/_compliant/expr.py Outdated Show resolved Hide resolved

remove isin

462cf8a

MarcoGorelli added the internal label Aug 8, 2025

MarcoGorelli changed the title ~~Fix sql operator problem~~ chore: fixup operator type ignores for SQLExpr Aug 8, 2025

MarcoGorelli approved these changes Aug 8, 2025

View reviewed changes

MarcoGorelli merged commit 787114d into narwhals-dev:main Aug 8, 2025
32 of 34 checks passed

ym-pett deleted the fix_sql_operator_problem branch August 14, 2025 13:28

	class NativeExpr(Protocol):
	"""An `Expr`-like object from a package with [Lazy-only support](https://narwhals-dev.github.io/narwhals/extending/#levels-of-support).

	Protocol members are chosen purely for matching statically - as they
	are common to all currently supported packages.
	"""

chore: fixup operator type ignores for SQLExpr #2920

chore: fixup operator type ignores for SQLExpr #2920

Uh oh!

Conversation

ym-pett commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ym-pett commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoGorelli commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ym-pett commented Aug 4, 2025

Uh oh!

ym-pett commented Aug 4, 2025

Uh oh!

ym-pett commented Aug 5, 2025

Uh oh!

MarcoGorelli commented Aug 5, 2025

Uh oh!

ym-pett commented Aug 5, 2025

Uh oh!

dangotbanned commented Aug 5, 2025

Uh oh!

dangotbanned commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoGorelli commented Aug 7, 2025

Uh oh!

dangotbanned commented Aug 7, 2025

Uh oh!

MarcoGorelli commented Aug 7, 2025

Uh oh!

dangotbanned commented Aug 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoGorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MarcoGorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chore: fixup operator type ignores for `SQLExpr` #2920

chore: fixup operator type ignores for `SQLExpr` #2920

ym-pett commented Aug 1, 2025 •

edited

Loading

ym-pett commented Aug 4, 2025 •

edited

Loading

MarcoGorelli commented Aug 4, 2025 •

edited

Loading

dangotbanned commented Aug 5, 2025 •

edited

Loading