enh: Adds support for `Time` datatype by marvinl803 · Pull Request #2113 · narwhals-dev/narwhals

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, 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

            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.

FBruzzesi · 2025-03-14T09:59:40Z

@MarcoGorelli I have a PR open in marimo that provide a fix, yet we will need a release - either we can merge this PR and make a patch release, or merge it on Monday before the minor release (I would prefer this second option) 😁

FBruzzesi

Thanks a ton for the contribution (and your patience) @marvinl803

Let's ship this to include this feature in today's release

P.S. @dangotbanned I was not able to solve the mypy issues is CI 🥲

dangotbanned · 2025-03-17T09:50:56Z

P.S. @dangotbanned I was not able to solve the mypy issues is CI 🥲

@FBruzzesi Let me take a quick look before merging 🤝

narwhals-dev#2113 (comment)

dangotbanned · 2025-03-17T10:18:09Z

I've opened a PR upstream for another fix

fix: Include Array in chunked_array overload zen-xu/pyarrow-stubs#190

https://github.com/narwhals-dev/narwhals/actions/runs/13896811190/job/38879205961?pr=2113

FBruzzesi · 2025-03-17T10:44:11Z

Thanks @dangotbanned , shall we merge?

@akshayka

## 📝 Summary [Narwhals#2113](narwhals-dev/narwhals#2113) is going to introduce `Time` dtype, this PR prepares for that ## 📋 Checklist - [x] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [ ] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on [Discord](https://marimo.io/discord?ref=pr), or the community [discussions](https://github.com/marimo-team/marimo/discussions) (Please provide a link if applicable). - [ ] I have added tests for the changes made. - [x] I have run the code and verified that it works as expected. ## 📜 Reviewers @akshayka OR @mscolnick --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Myles Scolnick <myles@marimo.io>

Both `pa.date32()` and `pa.time64("ns")` are currently supported in `modin`, but weren't included in #2113

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 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

Merged

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

Merged

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

dangotbanned reviewed Mar 7, 2025

View reviewed changes

Merge branch 'main' into issue-1989

a3732a8

Merge branch 'main' into issue-1989

9fba61f

FBruzzesi changed the title ~~enh: Adds support for Polars Time datatype~~ enh: Adds support for Time datatype Mar 17, 2025

FBruzzesi approved these changes Mar 17, 2025

View reviewed changes

fix(typing): resolve for pyarrow-stubs>=17.18

0237f7a

narwhals-dev#2113 (comment)

no cover?

53bfbfd

https://github.com/narwhals-dev/narwhals/actions/runs/13896811190/job/38879205961?pr=2113

dangotbanned merged commit 806f56b into narwhals-dev:main Mar 17, 2025
27 of 28 checks passed

dangotbanned added the dtypes label Apr 13, 2025

dangotbanned added a commit that referenced this pull request Aug 6, 2025

test: wip test_schema_from_pandas

0c5b7d5

Both `pa.date32()` and `pa.time64("ns")` are currently supported in `modin`, but weren't included in #2113

dangotbanned mentioned this pull request Aug 6, 2025

fix: Allow modin[pyarrow] to use more dtypes #2950

Merged

10 tasks

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

Conversation

marvinl803 commented Feb 28, 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!

FBruzzesi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marvinl803 commented Mar 3, 2025

Uh oh!

FBruzzesi commented Mar 4, 2025

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!

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

Choose a reason for hiding this comment

Uh oh!

FBruzzesi 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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

marvinl803 commented Mar 7, 2025

Uh oh!

FBruzzesi commented Mar 14, 2025

Uh oh!

FBruzzesi left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

dangotbanned commented Mar 17, 2025

Uh oh!

FBruzzesi commented Mar 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marvinl803 commented Feb 28, 2025 •

edited

Loading

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

dangotbanned commented Mar 17, 2025 •

edited

Loading