feat: Disallow casting temporal to numeric

[FBruzzesi]

Description

It aligns with the decision of not following polars in supertyping between temporal and numeric in #3396.

Questions/Observations:

1. What should we do in the case of a polars expr?

2. Let me know if we want to keep stable also V2, or none at all and do the check for all versions

   • Following Marco's comment in the live stream, I removed the check for V1 in b13135f

3. For lazy backends, you will see an "extra" dtype.is_numeric(), this is to avoid having to trigger a collect_schema() if there is no need for it

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

• 💾 Refactor
• ✨ Feature
• 🐛 Bug Fix
• 🔧 Optimization
• 📝 Documentation
• ✅ Test
• 🐳 Other

Related issues

• Closes disallow casting temporal to integer #121 well this is a ooold issue!

Checklist

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

[FBruzzesi]

TIL: polars allows also casting to Float, not only to Integer

[FBruzzesi]

CI failure is unrelated?!

Test was failing due to collect_schema in sqlframe with null only, hence the type is not defined. Because of this, you will see a suppress(Exception) context

[dangotbanned]

@FBruzzesi I know this PR isn't active, but I've been reading through marimo for (#3721) and thought of it

I think this would be pretty painful for them here:

• https://github.com/marimo-team/marimo/blob/c3e14e30082c397d2a560e794cfb718f461c169a/marimo/_plugins/ui/_impl/tables/narwhals_table.py

In short, they kinda need to be able to do this because pyarrow apparently doesn't support as much on temporal 😔

[FBruzzesi]

Hey @dangotbanned - which particular bit concerns you? The only cast to numeric in such file are from either decimal or types that are already numerical

[dangotbanned]

Hey @dangotbanned - which particular bit concerns you? The only cast to numeric ... or types that are already numerical

wow 🤦‍♂️ wow 🤦‍♂️ wow 🤦‍♂️

Sorry @FBruzzesi false alarm, JS is rubbing off on me:

Context

narwhals/src/narwhals/_plan/altair/expression.py

Lines 302 to 318 in a5c38b7

	_TEMPORAL_TIME_UNIT: FnMap[SingleTimeUnit_T] = {
	temporal.Year: "year",
	# has no equivalent
	# - "quarter" -> (Q1-Q4) polars has is without the prefix
	# - "month" -> (Jan-Dec)
	temporal.Day: "date",
	# has no equivalent
	# - "week" -> (W01-W52)
	# - "day" -> (Sunday-Saturday)
	temporal.OrdinalDay: "dayofyear",
	# NOTE: These aren't quite equivalent (vega -> Time, polars -> Int),
	# but for the purpose of visualization, you likely want the richer type
	temporal.Hour: "hours",
	temporal.Minute: "minutes",
	temporal.Second: "seconds",
	temporal.Millisecond: "milliseconds",
	}

The right-hand-side are vega expressions,.
There "hours", "minutes", "seconds" and "milliseconds" all result in what we'd call Time (which is "understandably" represented as Date 😭)

My mistake

So I connected the wrong dot here:

        if dtype == nw.Time:
            # Convert to timestamp in ms
            col_in_ms = (
                col.dt.hour().cast(nw.Int64) * 3600000
#                             ^^^^

Maybe they did too?
The only other reason I could see is to avoid an overflow issue, but I would've assumed both polars and pyarrow would promote the type if it got too big after multiplication?

[MarcoGorelli]

Thanks for looking into this

I'm not sure about inserting an extra collect_schema everywhere for the lazy backends, i was thinking that even just doing it for Series would be enough to dissuade people from doing this

[FBruzzesi]

Thanks for looking into this

I'm not sure about inserting an extra collect_schema everywhere for the lazy backends, i was thinking that even just doing it for Series would be enough to dissuade people from doing this

Thanks @MarcoGorelli I also have mixed feelings. Two additional comments I should have mentioned before:

1. The latest implementation tries to do as little as possible:

   try:
   	if (md := self._opt_metadata) is not None and md.is_pure_selection:
       	frame_schema = lf.collect_schema()
           sources = [frame_schema[name] for name in self._evaluate_output_names(lf)]
       else:
           sources = list(lf.select(self).collect_schema().values())

   As you can see, the first branch, it simply collects the schema - this might be cached if accessed before. The else has to run a select as well, which of course can have a larger impact.

2. I asked claude to write a (simple) script to run a benchmark in the following way: build a pipeline with a few operations between input and collect, then cast-bearing expressions within a select context:

   • cast at the start (bare column -> fast path).
   • cast in the middle (derived -> projection path).
   • cast at the end (derived -> projection path).

   I run it for dask and duckdb and the median (over 50 runs at 10M rows) runtime was within +/- 2%

I would still understand if you want to keep this eager only, but now you have a better picture

[dangotbanned]

Now there's been some activity I'm swooping in with another curveball ...

What if - instead - we added these guys?:

• Expr.dt.epoch
• nw.from_epoch

Then we would have explicit options for the equivalent of casting a temporal Arrow data type in either direction 😎

[MarcoGorelli]

that's orthogonal, the purpose of the issue was to prevent people from relying on integer -> datetime conversions

I would still understand if you want to keep this eager only, but now you have a better picture

thanks! if you still have the scripts i'd suggest comparing the minimums rather than the medians

maybe it's fine then, no strong opinions here. i also haven't seen any issues related to people relying on integer->datetime conversions and getting thrown off by different libraries using different default resolutions, so maybe the original issue is even out-of-date at this point and unnecessary? not sure 🤷

[dangotbanned]

the purpose of the issue was to prevent people from relying on integer -> datetime conversions

Why would you need to rely on a cast when a method does it?

https://github.com/pola-rs/polars/blob/0df0c25d4db895ec8cad773bedc4e98400e3135e/py-polars/src/polars/functions/lazy.py#L2609

Convert from integer seconds

df = pl.LazyFrame({"ts": [1666683077, 1666683099]})
df.select(pl.from_epoch(pl.col("ts"), time_unit="s")).collect()
shape: (2, 1)
┌─────────────────────┐
│ ts                  │
│ ---                 │
│ datetime[μs]        │
╞═════════════════════╡
│ 2022-10-25 07:31:17 │
│ 2022-10-25 07:31:39 │
└─────────────────────┘

that's orthogonal

You've stated that (#3430 (comment)) prevention is off the table.

The original issue

in pandas this does something really strange

In [3]: pd.to_datetime(['2020', None]).astype('int64')
Out[3]: Index([1577836800000000000, -9223372036854775808], dtype='int64')

Users shouldn't be relying on phyisical representations anyway, we should nudge them towards .dt.microseconds or whatever it is they're looking for

Encouraging people to take a safer option with clear APIs for what they want to do is not orthogonal.

Being open to alternative solutions to the problem is a healthy thing

[MarcoGorelli]

Why would you need to rely on a cast when a method does it?

you wouldn't need to, but it's the kind of thing i've seen people do unfortunately (even though better alternatives already exist)