enh: Adds support for Polars Time datatype #2113

marvinl803 · 2025-02-28T04:06:38Z

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

Related issues

Related issue [Enh]: Add support for Binary & Time datatype #1989

Checklist

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

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

I have added support for the Polars Time datatype. I wanted to implement this first to ensure there are no issues before proceeding with the Binary datatype, as mentioned in the issue.

Please let me know if any changes or fixes are needed. Thank you!

FBruzzesi

Thanks for the contribution @marvinl803 - This is a good start 🙏🏼

I would like to aim to support the following in this PR:

Add a case test in tests/expr_and_series/cast_test.py
Support more than just polars, as specified in #1989 comment, pyarrow and duckdb should be possible to support
Make sure that conversion from and to the backend is supported

narwhals/_polars/utils.py

marvinl803 · 2025-03-03T21:06:41Z

Thanks for the contribution @marvinl803 - This is a good start 🙏🏼

I would like to aim to support the following in this PR:

Add a case test in tests/expr_and_series/cast_test.py

Support more than just polars, as specified in #1989 comment, pyarrow and duckdb should be possible to support

Make sure that conversion from and to the backend is supported

Hi @FBruzzesi,

I have added the test cases where you suggested and also implemented support for PyArrow and DuckDB. However, I encountered an issue in narwhals/dtypes.py—I wasn’t sure how to provide an example for the docs using DuckDB.

Additionally, I added Time to all the other supported types in narwhals_to_native_dtype, except for PyArrow. I’m uncertain whether we should return time32 or time64 in that case. Could you provide some guidance on how best to handle this?

I appreciate your help!

FBruzzesi · 2025-03-04T06:26:14Z

I have added the test cases where you suggested and also implemented support for PyArrow and DuckDB. However, I encountered an issue in narwhals/dtypes.py—I wasn’t sure how to provide an example for the docs using DuckDB.

Thanks for your adjustments - I think we are very close to have this ready. Regarding the narwhals failing tests:

One is because of coverage: conversion from narwhals to native has no test yet
The other one if because of top level imports - try use constructor and mark unsupported backends as xfail rather than manually instantiating dataframes

Additionally, I added Time to all the other supported types in narwhals_to_native_dtype, except for PyArrow. I’m uncertain whether we should return time32 or time64 in that case. Could you provide some guidance on how best to handle this?

I think we should convert narwhals Time to pyarrow time64 since that's what polars Time mentions:

The underlying representation of this type is a 64-bit signed integer. The integer indicates the number of nanoseconds since midnight.

As a side note, we will need to make PRs in shiny and marimo to fix their failing tests

FBruzzesi · 2025-03-06T22:48:43Z

narwhals/_pandas_like/utils.py

+    if dtype.startswith("time") and dtype.endswith("[pyarrow]"):
+        return dtypes.Time()


Pattern is time<32|64>[<unit>][pyarrow]

Example:

from datetime import time import pandas as pd import pyarrow as pa data = {"a": [time(12, 0, 0), time(12, 0, 5)]} pd.DataFrame(data).convert_dtypes(dtype_backend="pyarrow").astype(pd.ArrowDtype(pa.time64("ns"))).dtypes a time64[ns][pyarrow] dtype: object

FBruzzesi · 2025-03-06T22:49:30Z

narwhals/_pandas_like/utils.py

-    if isinstance_or_issubclass(dtype, (dtypes.Struct, dtypes.Array, dtypes.List)):
+    if isinstance_or_issubclass(
+        dtype, (dtypes.Struct, dtypes.Array, dtypes.List, dtypes.Time)
+    ):
        if implementation is Implementation.PANDAS and backend_version >= (2, 2):


Unrelated, but... should we check modin backed by pyarrow?

FBruzzesi · 2025-03-06T22:50:29Z

narwhals/_spark_like/utils.py

@@ -142,9 +142,10 @@ def narwhals_to_native_dtype(
            dtypes.UInt8,
            dtypes.Enum,
            dtypes.Categorical,
+            dtypes.Time,


I couldn't find a datatype in pyspark for this, so it will just end up in the unsupported list

FBruzzesi · 2025-03-06T23:34:23Z

@marvinl803 I added a few commits to reach coverage and add pandas support (if backed by pyarrow dtype).

I think this is ready to merge, but we should have ready the PRs to address marimo and shiny whenever we press the merge button. I will try to find a bit of time tomorrow (no promises 🙈)

Plotly failure is unrelated, they simply reworked some deps files

dangotbanned · 2025-03-07T09:14:14Z

tests/expr_and_series/cast_test.py

+def test_cast_time(request: pytest.FixtureRequest, constructor: Constructor) -> None:
+    if "pandas" in str(constructor) and PANDAS_VERSION < (2, 2):
+        request.applymarker(pytest.mark.xfail)
+
+    if any(backend in str(constructor) for backend in ("dask", "pyspark", "modin")):
+        request.applymarker(pytest.mark.xfail)
+
+    data = {"a": [time(12, 0, 0), time(12, 0, 5)]}
+    df = nw.from_native(constructor(data))
+    result = df.select(nw.col("a").cast(nw.Time()))
+    assert result.collect_schema() == {"a": nw.Time()}


Great to see this is working for the Expr.cast case!

Could we also gets some tests to see how nw.Time interacts with Expr.dt methods?
I'm thinking we should aim to support anything that doesn't mention date, duration, time_zone components

The polars Temporal docs usually specify which subset of TemporalType they work with

I've only found polars.Expr.dt.to_string mention Time so far

There's also a huge suite here to draw from for test ideas: https://github.com/pola-rs/polars/blob/52b93ef5909cd0a5790001894aec4b473c361631/py-polars/tests/unit/datatypes/test_temporal.py

That might get more complex that expected: pandas does not access pyarrow time via .dt:

from datetime import time import pandas as pd import pyarrow as pa data = {"a": [time(12, 0, 0), time(12, 0, 5)]} ( pd.DataFrame(data) .convert_dtypes(dtype_backend="pyarrow") .astype(pd.ArrowDtype(pa.time64("ns"))) ["a"].dt )

AttributeError: Can only use .dt accessor with datetimelike values

@FBruzzesi we don't have to resolve all the issues in this PR - but I think it would still be worth identifying them for follow-ups.

Before reading through (#2113 (comment)) I wasn't aware of how little support there was for Time

marvinl803 · 2025-03-07T16:05:03Z

@marvinl803 I added a few commits to reach coverage and add pandas support (if backed by pyarrow dtype).

I think this is ready to merge, but we should have ready the PRs to address marimo and shiny whenever we press the merge button. I will try to find a bit of time tomorrow (no promises 🙈)

Plotly failure is unrelated, they simply reworked some deps files

Got it! Thanks for your help and the updates, @FBruzzesi. I'm not too familiar with Marimo and Shiny, but happy to assist however I can. Let me know if there's anything I can do! 👍

FBruzzesi · 2025-03-07T16:10:37Z

Got it! Thanks for your help and the updates, @FBruzzesi. I'm not too familiar with Marimo and Shiny, but happy to assist however I can. Let me know if there's anything I can do! 👍

Hey @marvinl803 I opened one PR in each repo, and forgot to mention it. You should be able to see the link somewhere here (I am from mobile and it's tricky to find the links) 😊

marvinl803 · 2025-03-07T18:03:45Z

Hey @marvinl803 I opened one PR in each repo, and forgot to mention it. You should be able to see the link somewhere here (I am from mobile and it's tricky to find the links) 😊

No problem, @FBruzzesi! I found them—I'll check them out.

Adds the Polars Time datatype for issue 1989.

a6f763e

marvinl803 marked this pull request as ready for review February 28, 2025 17:09

FBruzzesi reviewed Mar 1, 2025

View reviewed changes

narwhals/_polars/utils.py Outdated Show resolved Hide resolved

dangotbanned and others added 3 commits March 2, 2025 19:02

Merge branch 'main' into issue-1989

58f91c0

Merge branch 'narwhals-dev:main' into issue-1989

aa2e7ba

Added the proposed changes.

f256b9a

FBruzzesi and others added 6 commits March 6, 2025 19:29

try pandas backed by pyarrow

b284e02

rm duplicate line

c992c52

Added changes to tests, and pyarrow Time

88580bb

added pyarrow time32 to be supported when converted to narwhals time.

ffd6925

Merge branch 'main' into issue-1989

cac56a8

collapse arrow check into one 'or' condition

c8b3aed

FBruzzesi reviewed Mar 6, 2025

View reviewed changes

Merge branch 'main' into issue-1989

5b73e5c

FBruzzesi requested review from MarcoGorelli and dangotbanned March 7, 2025 07:28

This was referenced Mar 7, 2025

chore: Add support for Narwhals Time dtype in serialize_dtype posit-dev/py-shiny#1892

Open

chore: Add support for narwhals Time dtype marimo-team/marimo#4019

Open

dangotbanned added the enhancement New feature or request label Mar 7, 2025

dangotbanned reviewed Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enh: Adds support for Polars Time datatype #2113

enh: Adds support for Polars Time datatype #2113

marvinl803 commented Feb 28, 2025

FBruzzesi left a comment

marvinl803 commented Mar 3, 2025

FBruzzesi commented Mar 4, 2025

FBruzzesi Mar 6, 2025

FBruzzesi Mar 6, 2025

FBruzzesi Mar 6, 2025

FBruzzesi commented Mar 6, 2025 •

edited

Loading

dangotbanned Mar 7, 2025

dangotbanned Mar 7, 2025

dangotbanned Mar 7, 2025

dangotbanned Mar 7, 2025

FBruzzesi Mar 7, 2025 •

edited

Loading

dangotbanned Mar 7, 2025

marvinl803 commented Mar 7, 2025 •

edited

Loading

FBruzzesi commented Mar 7, 2025 •

edited

Loading

marvinl803 commented Mar 7, 2025

		if dtype.startswith("time") and dtype.endswith("[pyarrow]"):
		return dtypes.Time()

enh: Adds support for Polars Time datatype #2113

Are you sure you want to change the base?

enh: Adds support for Polars Time datatype #2113

Conversation

marvinl803 commented Feb 28, 2025

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

FBruzzesi left a comment

Choose a reason for hiding this comment

marvinl803 commented Mar 3, 2025

FBruzzesi commented Mar 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FBruzzesi commented Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FBruzzesi Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marvinl803 commented Mar 7, 2025 • edited Loading

FBruzzesi commented Mar 7, 2025 • edited Loading

marvinl803 commented Mar 7, 2025

FBruzzesi commented Mar 6, 2025 •

edited

Loading

FBruzzesi Mar 7, 2025 •

edited

Loading

marvinl803 commented Mar 7, 2025 •

edited

Loading

FBruzzesi commented Mar 7, 2025 •

edited

Loading