chore(typing): Use SQLFrame instead of PySpark for `_spark_like` internally by MarcoGorelli · Pull Request #2190 · narwhals-dev/narwhals

MarcoGorelli · 2025-03-11T19:07:03Z

closes #2185

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

CONTRIBUTING.md

dangotbanned · 2025-03-11T21:04:11Z

narwhals/_spark_like/dataframe.py

-    SQLFrameDataFrame: TypeAlias = _SQLFrameDataFrame[Any, Any, Any, Any, Any]
-    _NativeDataFrame: TypeAlias = "DataFrame | SQLFrameDataFrame"
+    NativeFrame = BaseDataFrame[Any, Any, Any, Any, Any]
+    NativeSession = _BaseSession[Any, Any, Any, Any, Any, Any, Any]


Don't think I'll ever get used to seeing 7 TypeVars 😂

narwhals/_spark_like/dataframe.py

narwhals/dependencies.py

dangotbanned · 2025-03-12T10:48:09Z

Thanks for the updates @MarcoGorelli

Do you mind holding off on merging?
I'd like to have a look in an IDE today - just to be confident we're not regressing anywhere 🙂

- `mypy` just needed the `else` block - but this is much more maintainable

Need it to start a thread

- Added in narwhals-dev#2187, narwhals-dev#2051 - No longer needed since `sqlframe` only typing

dangotbanned · 2025-03-12T13:06:07Z

narwhals/translate.py

+                native_object,  # NOTE: this is leaking the internal lie
+                # error: Argument of type "DataFrame" cannot be assigned to parameter "native_dataframe" of type "SQLFrameDataFrame" in function "__init__"
+                #    "DataFrame" is not assignable to "BaseDataFrame[Any, Any, Any, Any, Any]" (reportArgumentType)


@MarcoGorelli this seems like it'll impact LazyFrame[T] - since we need to say that'll only be SQLFrameDataFrame for SparkLikeLazyFrame

#2185 (comment)

Turned out that while Series isn't an issue - Frame may well be

how so?

I tried

# ruff: noqa from __future__ import annotations import narwhals as nw import pandas as pd import polars as pl from sqlframe.duckdb import DuckDBSession from sqlframe.duckdb.dataframe import DuckDBDataFrame import sqlframe.duckdb.functions as F from pyspark.sql.dataframe import DataFrame as SparkDataFrame def func(a: SparkDataFrame) -> None: reveal_type(nw.from_native(a).to_native()) def func2(a: DuckDBDataFrame) -> None: reveal_type(nw.from_native(a).to_native())

and get

/home/marcogorelli/polars-api-compat-dev/t.py:14:17 - information: Type of "nw.from_native(a).to_native()" is "DataFrame" /home/marcogorelli/polars-api-compat-dev/t.py:16:17 - information: Type of "nw.from_native(a).to_native()" is "DuckDBDataFrame"

Try changing that to this and you'll see it matches DataFrame and not LazyFrame

def func(a: SparkDataFrame) -> None: b = nw.from_native(a) reveal_type(b) c = b.to_native() reveal_type(c)

Type of "b" is "narwhals.dataframe.DataFrame[pyspark.sql.dataframe.DataFrame]"

Bit confused on this one though 🤔

Type of "c" is "DataFrame"

@MarcoGorelli just in case you missed this while writing #2190 (comment)

isn't this an issue on main as well:

(.310venv) marcogorelli@DESKTOP-U8OKFP3:~/polars-api-compat-dev$ cat t.py # ruff: noqa from __future__ import annotations import narwhals as nw import pandas as pd import polars as pl from sqlframe.duckdb import DuckDBSession from sqlframe.duckdb.dataframe import DuckDBDataFrame import sqlframe.duckdb.functions as F from pyspark.sql.dataframe import DataFrame as SparkDataFrame def func(a: SparkDataFrame) -> None: reveal_type(nw.from_native(a).to_native()) reveal_type(nw.from_native(a)) def func2(a: DuckDBDataFrame) -> None: reveal_type(nw.from_native(a).to_native()) reveal_type(nw.from_native(a)) (.310venv) marcogorelli@DESKTOP-U8OKFP3:~/polars-api-compat-dev$ pyright t.py /home/marcogorelli/polars-api-compat-dev/t.py /home/marcogorelli/polars-api-compat-dev/t.py:14:17 - information: Type of "nw.from_native(a).to_native()" is "DataFrame" /home/marcogorelli/polars-api-compat-dev/t.py:15:17 - information: Type of "nw.from_native(a)" is "narwhals.dataframe.DataFrame[pyspark.sql.dataframe.DataFrame]" /home/marcogorelli/polars-api-compat-dev/t.py:17:17 - information: Type of "nw.from_native(a).to_native()" is "DuckDBDataFrame" /home/marcogorelli/polars-api-compat-dev/t.py:18:17 - information: Type of "nw.from_native(a)" is "DataFrame[DuckDBDataFrame]" 0 errors, 0 warnings, 4 informations

isn't this an issue on main as well

Yeah I also noticed it seems to happen for any LazyFrame when passed without any keywords

More motivation towards #2116 I suppose

Tracking in (#2194)

dangotbanned · 2025-03-12T13:11:30Z

@MarcoGorelli more of a general note

A lot of the stuff I've touched is because I still have pyspark installed locally - which I think you may not anymore?

We still need to have that factored into the CI - even if you want to speed things up locally.
Otherwise it'll make it harder for contributors to work with pyspark - without conflicting with CI on where to ignore.
If you remember 😉 #1963 (comment)

MarcoGorelli · 2025-03-12T13:53:26Z

We still need to have that factored into the CI - even if you want to speed things up locally.
Otherwise it'll make it harder for contributors to work with pyspark - without conflicting with CI on where to ignore.
If you remember 😉 #1963 (comment)

could you elaborate please? can't we run pyspark tests in CI, but just use sqlframe for internal type checking?

dangotbanned · 2025-03-12T14:12:20Z

We still need to have that factored into the CI - even if you want to speed things up locally.
Otherwise it'll make it harder for contributors to work with pyspark - without conflicting with CI on where to ignore.
If you remember 😉 #1963 (comment)

could you elaborate please?
@MarcoGorelli

Sure thing
With pyspark installed - I'm getting this warning in translate.py

narwhals/narwhals/translate.py

Line 732 in af20c95

native_object,

Screenshot 1

Which is a result of this change

narwhals/narwhals/_spark_like/dataframe.py

Line 44 in af20c95

_NativeDataFrame: TypeAlias = "DataFrame | SQLFrameDataFrame"

narwhals/narwhals/_spark_like/dataframe.py

Line 53 in af20c95

native_dataframe: _NativeDataFrame,

Screenshot 2

This is what I meant in (#2185 (comment)) when I said

The problem is they're all connected.

Screenshot 3

can't we run pyspark tests in CI
@MarcoGorelli

This is what I want - but the fact that pyright is passing in CI is what I want to fix.
It should be exiting on that error:

>>> pyright
c:\Users\danie\Documents\GitHub\narwhals\narwhals\translate.py
  c:\Users\danie\Documents\GitHub\narwhals\narwhals\translate.py:732:17 - error: Argument of type "DataFrame" cannot be assigned to parameter "native_dataframe" of type "SQLFrameDataFrame" in function "__init__"
    "DataFrame" is not assignable to "BaseDataFrame[Any, Any, Any, Any, Any]" (reportArgumentType)
1 error, 0 warnings, 0 informations

I'm thinking the problem is in these changes:

https://github.com/narwhals-dev/narwhals/pull/2190/files#diff-8da2d7afa6668fbe866ba5f8a8f24dd448aa57df874700c659e8ad93abf29478
https://github.com/narwhals-dev/narwhals/pull/2190/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711

MarcoGorelli · 2025-03-12T14:20:33Z

is it ok if we just type ignore / pyright ignore that line in translate? I think that's what I'd have expected if as you rightly note we're lying about the native objects always being sqlframe ones. But I think that's OK, if passing type-checking is a requirement for any pull request, and the APIs are similar enough, then it seems OK to ignore an internal line in translate.py

From having tried #2190 (comment), it looks like it all works fine with pyspark installed from the user's perspective

dangotbanned · 2025-03-12T14:34:45Z

is it ok if we just type ignore / pyright ignore that line in translate?

Yeah that's fine if it isn't causing an issue externally.

But we do need to have pyspark installed in CI for typing.
The workflow isn't slow with it and it would've caught that warning
https://github.com/narwhals-dev/narwhals/actions/workflows/typing.yml

narwhals-dev#2190 (comment) narwhals-dev#2190 (comment)

narwhals-dev#2190 (comment) https://github.com/narwhals-dev/narwhals/actions/runs/13834285345/job/38705545003?pr=2190

dangotbanned · 2025-03-13T12:28:00Z

narwhals/_spark_like/dataframe.py

        validate_column_names: bool = False,
    ) -> None:
-        self._native_frame = native_dataframe
+        self._native_frame: SQLFrameDataFrame = native_dataframe


I added an annotation to try and trigger an [arg-type] from mypy.

Still not sure why pyright is the only one to detect (d5e899a)

dangotbanned

I'm happy with where everything is at now @MarcoGorelli

Thanks for proposing and working on this 🎉

MarcoGorelli · 2025-03-13T15:34:35Z

Awesome, thanks for your review and help here Dan, much appreciated! 🙏

MarcoGorelli added 3 commits March 11, 2025 19:03

typing: use sqlframe for type validation internally

dad47af

update contributing

d8e432b

fixup

125a4a1

MarcoGorelli marked this pull request as ready for review March 11, 2025 19:20

dangotbanned reviewed Mar 11, 2025

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

dangotbanned reviewed Mar 11, 2025

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

dangotbanned reviewed Mar 11, 2025

View reviewed changes

narwhals/_spark_like/dataframe.py Outdated Show resolved Hide resolved

dangotbanned reviewed Mar 11, 2025

View reviewed changes

narwhals/dependencies.py Outdated Show resolved Hide resolved

MarcoGorelli added 8 commits March 12, 2025 09:16

dont shadow NativeFrame protocol

f5de21c

typing

980aaea

update reqs

92d8b8a

fixup

d9ce539

skip sqlframe on py3.8

28a6b98

fixup

7de4919

one more

4374a29

one more

e923bf9

MarcoGorelli added the internal label Mar 12, 2025

dangotbanned self-assigned this Mar 12, 2025

dangotbanned added the typing label Mar 12, 2025

dangotbanned added 5 commits March 12, 2025 12:46

fix(typing): pyspark compat for std, var

46756cb

refactor(typing): reuse _NativeDType alias

612d974

fix(typing): factor out imports, avoid redef

d1d58a6

- `mypy` just needed the `else` block - but this is much more maintainable

chore(typing): Add note on pyspark installed issue

36adf66

Need it to start a thread

chore(typing): remove unused ignores

4a1aabf

- Added in narwhals-dev#2187, narwhals-dev#2051 - No longer needed since `sqlframe` only typing

dangotbanned reviewed Mar 12, 2025

View reviewed changes

dangotbanned removed their assignment Mar 12, 2025

dangotbanned added 3 commits March 12, 2025 17:36

Merge branch 'main' into replace-pyspark-typing

5721176

ci: add pyspark back to typing.yml

f5bafb6

narwhals-dev#2190 (comment) narwhals-dev#2190 (comment)

chore(typing): Ignore pyspark arg type

d5e899a

narwhals-dev#2190 (comment) https://github.com/narwhals-dev/narwhals/actions/runs/13834285345/job/38705545003?pr=2190

dangotbanned reviewed Mar 13, 2025

View reviewed changes

dangotbanned approved these changes Mar 13, 2025

View reviewed changes

dangotbanned merged commit 7611bd4 into narwhals-dev:main Mar 13, 2025
29 checks passed

dangotbanned changed the title ~~type: Use SQLFrame instead of PySpark to type _spark_like internally~~ chore(typing): Use SQLFrame instead of PySpark for _spark_like internally Mar 13, 2025

Conversation

MarcoGorelli commented Mar 11, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dangotbanned commented Mar 12, 2025

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

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

Uh oh!

Uh oh!

MarcoGorelli commented Mar 12, 2025

Uh oh!

dangotbanned commented Mar 12, 2025

Uh oh!

MarcoGorelli commented Mar 12, 2025

Uh oh!

dangotbanned commented Mar 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dangotbanned left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MarcoGorelli commented Mar 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MarcoGorelli commented Mar 11, 2025 •

edited

Loading

dangotbanned Mar 12, 2025 •

edited

Loading

dangotbanned Mar 12, 2025 •

edited

Loading

dangotbanned commented Mar 12, 2025 •

edited

Loading

dangotbanned left a comment •

edited

Loading