chore(typing): `SparkLikeExpr` properties #2152

dangotbanned · 2025-03-05T18:26:59Z

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

Porting over (#2051), didn't realize this was declared twice until (#2132)

Porting over (#2051), didn't realize this was delclared twice until (#2132)

…ime` Resolves (https://github.com/narwhals-dev/narwhals/actions/runs/13682912007/job/38259412675?pr=2152)

dangotbanned · 2025-03-05T19:15:33Z

@MarcoGorelli if you merge this ahead of #2132 - you'll be able to get more useful IDE feedback from SparkLikeExpr.(_F|_Window) 😊

I checked out the PR and was confused why I didn't get hints from #2051

EdAbati · 2025-03-05T21:31:19Z

narwhals/_spark_like/expr_str.py

+def is_naive(obj: Any) -> bool:
+    return obj is not None and {"%s", "%z", "Z"}.isdisjoint(obj)


Suggested change

def is_naive(obj: Any) -> bool:

return obj is not None and {"%s", "%z", "Z"}.isdisjoint(obj)

def is_naive(format: str) -> bool:

return {"%s", "%z", "Z"}.isdisjoint(format)

just to signal that this checks the date time format (should we maybe call it is_naive_format?).
Also based on how it is used, format will always be a non-null str right?

Thanks @EdAbati, yeah is_naive_format sounds good to me

I renamed format to obj to avoid a ruff warning for shadowing builtins.format

Also narrowing from Any to str sounds good as well.
I originally wrote it using a TypeIs but removed that since it gave a false-positive on unreachability

Happy for you to make those changes and merge 🙌

Actually I think the current set needs to be changed to:

{"s", "z", "Z"}

I'm relying on format splitting into a sequence of characters - but two of them are 2-char sequences.

I wonder why that didn't fail a test?

I wonder why that didn't fail a test?

I actually haven't seen a test covering this 👀 double checking with the datetime wizard @MarcoGorelli , is it something that we are not testing on purpose?

Probably related to

refactor: use sqlframe instead of pyspark for testing? #1818

Maybe I'm reading this wrong, but I dont think we have coverage for any backend with a tz-aware format for .str.to_datetime(...)

https://github.com/narwhals-dev/narwhals/blob/3103b20afc1a523137f74003478c77a9debc2781/tests/expr_and_series/str/to_datetime_test.py

@MarcoGorelli really need some help on writing a test for this 🙏

Right now I've got something kinda working for a lot of backends.
But it fails to parse for sqlframe, pyarrow.

Also polars.Expr.str.to_datetime seems to have more options - that might be able to preserve the timezone offset?

test_to_datetime_tz_aware

@pytest.mark.parametrize( ("fmt", "data", "expected_pandas", "expected_polars_duckdb"), [ ( "%Y-%m-%d %H:%M:%S%z", {"a": ["2020-01-01 12:34:56+0200"]}, "2020-01-01 12:34:56+02:00", "2020-01-01 10:34:56+00:00", ) ], ) def test_to_datetime_tz_aware( constructor: Constructor, fmt: str, data: dict[str, Sequence[str]], expected_pandas: str, expected_polars_duckdb: str, ) -> None: constructor_str = str(constructor) if "pandas" in constructor_str or "dask" in constructor_str: expected = expected_pandas elif "polars" in constructor_str or "duckdb" in constructor_str: expected = expected_polars_duckdb else: expected = "fails" result = ( nw.from_native(constructor(data)) .lazy() .select(b=nw.col("a").str.to_datetime(fmt)) .collect() .item(row=0, column="b") ) assert str(result) == expected

I can fully understand nope-ing out of handling timezones for all these different packages 😒

Alright so (e19dcdf) -> (194ef83) now has a single test

sqlframe works, pyspark apprently doesn't https://github.com/narwhals-dev/narwhals/actions/runs/13700361438/job/38312364899?pr=2152

Absolutely don't want to do timezones again 😩

@FBruzzesi I see you added strptime_to_pyspark_format in #1826

AFAICT there were never any tests for tz-aware timestamps and right now I can't get one to work 😞

Tried my best, but as of (ce3145d) I'm giving up

narwhals/_spark_like/expr.py

Resolves #2152 (comment)

Maybe this was just a TZDATA issue locally? https://github.com/narwhals-dev/narwhals/actions/runs/13699734154/job/38310256617?pr=2152

https://github.com/narwhals-dev/narwhals/actions/runs/13699804987/job/38310487932?pr=2152 https://github.com/narwhals-dev/narwhals/actions/runs/13699804987/job/38310488783?pr=2152

Not even sure what `pyarrow` is doing here https://github.com/narwhals-dev/narwhals/actions/runs/13700021595/job/38311197947?pr=2152

https://github.com/narwhals-dev/narwhals/actions/runs/13700267075/job/38312036397?pr=215

https://github.com/narwhals-dev/narwhals/actions/runs/13700361438/job/38312364899?pr=2152

MarcoGorelli

thanks for looking into this!

I think this PR is doing too much, please either address typing or fix str.to_datetime

the former should be fine, some comments for the latter:

can we keep format even if it overrides the builtin?
we should probably make sure that we consistently parse into UTC, so there should only be a single expected for test_to_datetime_tz_aware

I can take care of the timezone handling separately, but it does need splitting out

Addresses #2152 (review)

#2152 (review)

dangotbanned · 2025-03-07T15:48:04Z

#2152 (review)

Thanks for the review @MarcoGorelli
I think you're right, I bit off a lil more than I could chew here.

Caught me at a good time with the feedback - hopefully I've addressed it all in:

(4470a99)
(77a264c)

I can take care of the timezone handling separately, but it does need splitting out

Phew - I'm in no hurry to deal w/ timezones again 😅

Edit

Just for some clarity - I touched that method to resolve these https://github.com/narwhals-dev/narwhals/actions/runs/13682912007/job/38259412675?pr=2152

Looking back now - I'm not sure that was super clear

narwhals/_spark_like/expr.py

MarcoGorelli · 2025-03-07T20:49:47Z

narwhals/_spark_like/expr_str.py

+def is_naive_format(format: str) -> bool:  # noqa: A002
+    return {"s", "z", "Z"}.isdisjoint(format)


i don't really like how we're changing the logic here, 'z' in a format string means the 'z' literal, it's '%z' we need to check for

the rest looks good though, well done!

PR to address time zone aware parsing: #2166

Thanks @MarcoGorelli

I'm happy for that bit to be reverted and just keep the splitting out into the new function?

This kind of any(x in (...)) check would probably be good

https://github.com/MarcoGorelli/narwhals/blob/fdb2f4812945e9fab459b6bc473e095b18fe61a7/narwhals/_pandas_like/series_str.py#L88

sure! if you're happy with #2166 to add a test and adjust some logic, happy to then take this?

Hopefully got it done (311c7ea)

#2152 (comment)

…arwhals into spark-expr-typing

MarcoGorelli

thanks @dangotbanned much appreciated!

chore(typing): Add typing for SparkLikeExpr properties

47e768d

Porting over (#2051), didn't realize this was delclared twice until (#2132)

dangotbanned added internal typing labels Mar 5, 2025

chore: fix typing and simplify `SparkLikeExprStringNamespace.to_datet…

7bb9131

…ime` Resolves (https://github.com/narwhals-dev/narwhals/actions/runs/13682912007/job/38259412675?pr=2152)

dangotbanned marked this pull request as ready for review March 5, 2025 19:11

dangotbanned requested review from MarcoGorelli and FBruzzesi March 5, 2025 19:13

EdAbati reviewed Mar 5, 2025

View reviewed changes

narwhals/_spark_like/expr.py Show resolved Hide resolved

EdAbati and others added 9 commits March 6, 2025 08:05

rename function

89eeed5

use single chars in set

f730d52

Merge branch 'main' into spark-expr-typing

fda0fec

fix: Remove timezone offset replacement

e19dcdf

test: Adds test_to_datetime_tz_aware

a4b619c

Resolves #2152 (comment)

test: possibly fix pyarrow in ci?

2367125

Maybe this was just a TZDATA issue locally? https://github.com/narwhals-dev/narwhals/actions/runs/13699734154/job/38310256617?pr=2152

test: xfail polars 3.8, fix false positive pyarrow

fb48654

https://github.com/narwhals-dev/narwhals/actions/runs/13699804987/job/38310487932?pr=2152 https://github.com/narwhals-dev/narwhals/actions/runs/13699804987/job/38310488783?pr=2152

test: narrower xfail, tz-less expected?

75e48cc

Not even sure what `pyarrow` is doing here https://github.com/narwhals-dev/narwhals/actions/runs/13700021595/job/38311197947?pr=2152

test: account for pyarrow version changes

194ef83

https://github.com/narwhals-dev/narwhals/actions/runs/13700267075/job/38312036397?pr=215

dangotbanned added fix tests labels Mar 6, 2025

dangotbanned added 2 commits March 6, 2025 14:24

test: maybe fix pyspark

ce3145d

https://github.com/narwhals-dev/narwhals/actions/runs/13700361438/job/38312364899?pr=2152

Merge branch 'main' into spark-expr-typing

ef015af

dangotbanned added the help wanted Extra attention is needed label Mar 6, 2025

dangotbanned changed the title ~~chore(typing): Add typing for SparkLikeExpr properties~~ chore: SparkLikeExpr property typing, fix str.to_datetime Mar 6, 2025

Merge branch 'main' into spark-expr-typing

75e4477

MarcoGorelli reviewed Mar 7, 2025

View reviewed changes

dangotbanned added 2 commits March 7, 2025 15:42

revert: go back to typing fixes only

4470a99

Addresses #2152 (review)

chore: ignore format shadowing

77a264c

#2152 (review)

dangotbanned changed the title ~~chore: SparkLikeExpr property typing, fix str.to_datetime~~ chore(typing): SparkLikeExpr properties Mar 7, 2025

dangotbanned removed help wanted Extra attention is needed fix tests labels Mar 7, 2025

Merge remote-tracking branch 'upstream/main' into spark-expr-typing

fbf2459

MarcoGorelli reviewed Mar 7, 2025

View reviewed changes

narwhals/_spark_like/expr.py Show resolved Hide resolved

Merge branch 'main' into spark-expr-typing

fb2820b

MarcoGorelli reviewed Mar 7, 2025

View reviewed changes

dangotbanned added 2 commits March 7, 2025 22:41

keep logic the same I hope

311c7ea

#2152 (comment)

Merge branch 'spark-expr-typing' of https://github.com/narwhals-dev/n…

7936aa8

…arwhals into spark-expr-typing

MarcoGorelli approved these changes Mar 7, 2025

View reviewed changes

MarcoGorelli merged commit 734c641 into main Mar 7, 2025
29 checks passed

MarcoGorelli deleted the spark-expr-typing branch March 7, 2025 23:09

This was referenced Mar 9, 2025

docs(ruff): Improve config docs #2173

Merged

chore(ruff): add format to builtins-ignorelist #2199

Merged

		def is_naive(obj: Any) -> bool:
		return obj is not None and {"%s", "%z", "Z"}.isdisjoint(obj)

		def is_naive_format(format: str) -> bool: # noqa: A002
		return {"s", "z", "Z"}.isdisjoint(format)

chore(typing): SparkLikeExpr properties #2152

chore(typing): SparkLikeExpr properties #2152

Uh oh!

Conversation

dangotbanned commented Mar 5, 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!

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

Uh oh!

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 Mar 5, 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!

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

Uh oh!

Choose a reason for hiding this comment

test_to_datetime_tz_aware

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dangotbanned Mar 6, 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!

Uh oh!

MarcoGorelli left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Edit

Uh oh!

Uh oh!

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

MarcoGorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chore(typing): `SparkLikeExpr` properties #2152

chore(typing): `SparkLikeExpr` properties #2152

dangotbanned commented Mar 5, 2025 •

edited

Loading

dangotbanned commented Mar 5, 2025 •

edited

Loading

dangotbanned Mar 5, 2025 •

edited

Loading

dangotbanned Mar 6, 2025 •

edited

Loading

`test_to_datetime_tz_aware`

dangotbanned Mar 6, 2025 •

edited

Loading

dangotbanned commented Mar 7, 2025 •

edited

Loading

MarcoGorelli Mar 7, 2025 •

edited

Loading