fix: support converting empty `time` Series to pyarrow Array #11

tswast · 2021-09-27T16:00:24Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #10 🦕

tswast · 2021-09-27T16:01:04Z

Currently getting pyarrow.lib.ArrowNotImplementedError: Unsupported cast from double to time64 using function cast_time64 in the empty time Series test.

tswast · 2021-09-27T16:53:13Z

I'm a bit surprised this doesn't happen for date. I'll check with plain pandas timedelta64 to see what it does with to_numpy() for an empty series.

tswast · 2021-09-27T16:58:51Z

Seems to work fine on latest pandas. 🤔

In [1]: import pandas

In [2]: pandas.__version__
Out[2]: '1.4.0.dev0+502.g019d782633'

In [3]: s = pandas.Series([], dtype='timedelta64[ns]')

In [4]: import pyarrow

In [5]: a = pyarrow.array(s)

In [6]: a
Out[6]:
<pyarrow.lib.DurationArray object at 0x7fb98813bd60>
[]

In [7]: s.to_numpy()
Out[7]: array([], dtype='timedelta64[ns]')

tswast · 2021-09-27T20:37:34Z

db_dtypes/__init__.py

@@ -98,7 +98,8 @@ def astype(self, dtype, copy=True):

    def __arrow_array__(self, type=None):
        return pyarrow.array(
-            self.to_numpy(), type=type if type is not None else pyarrow.time64("ns"),
+            self.to_numpy(dtype="object"),


This might have performance implications, but it does seem to prevent the cast to float64 for empty arrays. Also, the dtype seems to be object whenever there are any values in the array, anyway.

I approached this by adding the missing to_numpy() for pandas <1, that just uses astype('object')

Sweet. That seems to have done the trick. I copied the implementation from your PR https://github.com/googleapis/python-db-dtypes-pandas/pull/9/files#diff-1956943f14005805ef968dfc37c26fe3eee995786f62a1c66dee5a29d9b1a251R111

jimfulton · 2021-09-28T01:55:53Z

tests/unit/test_arrow.py

+            ),
+        ),
+        (
+            pandas.Series(
+                [
+                    dt.time(0, 0, 0, 0),
+                    dt.time(12, 30, 15, 125_000),
+                    dt.time(23, 59, 59, 999_999),
+                ],
+                dtype="time",
+            ),
+            pyarrow.array(
+                [
+                    dt.time(0, 0, 0, 0),
+                    dt.time(12, 30, 15, 125_000),
+                    dt.time(23, 59, 59, 999_999),
+                ],
+                type=pyarrow.time64("ns"),
+            ),
+        ),
+    ),
+)
+def test_to_arrow(series, expected):
+    array = pyarrow.array(series)
+    assert array.equals(expected)
+
+
+@pytest.mark.parametrize(
+    ("series", "expected"),
+    (
+        (pandas.Series([], dtype="date"), pyarrow.array([], type=pyarrow.date64())),
+        (
+            pandas.Series([None, None, None], dtype="date"),
+            pyarrow.array([None, None, None], type=pyarrow.date64()),
+        ),
+        (
+            pandas.Series(
+                [dt.date(2021, 9, 27), None, dt.date(2011, 9, 27)], dtype="date"
+            ),
+            pyarrow.array(
+                [dt.date(2021, 9, 27), None, dt.date(2011, 9, 27)],
+                type=pyarrow.date64(),
+            ),
+        ),
+        (
+            pandas.Series(
+                [dt.date(1677, 9, 22), dt.date(1970, 1, 1), dt.date(2262, 4, 11)],
+                dtype="date",
+            ),
+            pyarrow.array(
+                [dt.date(1677, 9, 22), dt.date(1970, 1, 1), dt.date(2262, 4, 11)],
+                type=pyarrow.date64(),
+            ),
+        ),
+        (pandas.Series([], dtype="time"), pyarrow.array([], type=pyarrow.time32("ms"))),
+        (
+            pandas.Series([None, None, None], dtype="time"),
+            pyarrow.array([None, None, None], type=pyarrow.time32("ms")),
+        ),
+        (
+            pandas.Series(
+                [dt.time(0, 0, 0, 0), None, dt.time(23, 59, 59, 999_000)], dtype="time"
+            ),
+            pyarrow.array(
+                [dt.time(0, 0, 0, 0), None, dt.time(23, 59, 59, 999_000)],
+                type=pyarrow.time32("ms"),
+            ),
+        ),
+        (
+            pandas.Series(
+                [dt.time(0, 0, 0, 0), None, dt.time(23, 59, 59, 999_999)], dtype="time"
+            ),
+            pyarrow.array(
+                [dt.time(0, 0, 0, 0), None, dt.time(23, 59, 59, 999_999)],
+                type=pyarrow.time64("us"),
+            ),
+        ),
+        (
+            pandas.Series(
+                [
+                    dt.time(0, 0, 0, 0),
+                    dt.time(12, 30, 15, 125_000),
+                    dt.time(23, 59, 59, 999_999),
+                ],
+                dtype="time",
+            ),
+            pyarrow.array(
+                [
+                    dt.time(0, 0, 0, 0),
+                    dt.time(12, 30, 15, 125_000),
+                    dt.time(23, 59, 59, 999_999),
+                ],
+                type=pyarrow.time64("us"),
+            ),
+        ),
+    ),
+)
+def test_to_arrow_w_arrow_type(series, expected):
+    array = pyarrow.array(series, type=expected.type)
+    assert array.equals(expected)


Your tests are nicer than mine. :)

Thanks! They did catch a bug with empty arrays, so I'm glad I wrote them

plamut

Looks good. I just can't say about the possible performance impact of self.to_numpy(dtype="object").

Are we going to use that conditionally for pandas<1 as Jim suggested?

tswast · 2021-09-28T19:48:32Z

I just can't say about the possible performance impact of self.to_numpy(dtype="object").

Thankfully I think this approach with using an extension dtype leaves us room for optimization. We might even switch to backing everything with pyarrow arrays instead of numpy arrays someday.

Are we going to use that conditionally for pandas<1 as Jim suggested?

Yes, that seems to be the right approach. Just pushed a commit.

tswast · 2021-09-29T14:04:24Z

tests/unit/test_dtypes.py

-
-
-@for_date_and_time
-def test_date___arrow__array__(dtype):


Removed because it's redundant with the new test_arrow.py tests.

🤖 I have created a release \*beep\* \*boop\* --- ## 0.1.0 (2021-09-29) ### Features * add `time` and `date` dtypes ([f104171](https://www.github.com/googleapis/python-db-dtypes-pandas/commit/f10417111642e8f5f4b9af790367af930d15a056)) ### Bug Fixes * support converting empty `time` Series to pyarrow Array ([#11](https://www.github.com/googleapis/python-db-dtypes-pandas/issues/11)) ([7675b15](https://www.github.com/googleapis/python-db-dtypes-pandas/commit/7675b157feb842628fa731cc6a472aa9e6b92903)) * support Pandas 0.24 ([#8](https://www.github.com/googleapis/python-db-dtypes-pandas/issues/8)) ([e996883](https://www.github.com/googleapis/python-db-dtypes-pandas/commit/e996883bc9c76fe5f593e9c19a9d2a1c13501f5e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 27, 2021

product-auto-label bot added the api: bigquery Issues related to the googleapis/python-db-dtypes-pandas API. label Sep 27, 2021

fix: support converting empty time Series to pyarrow Array

e793ec6

tswast force-pushed the issue10-arrow-coverage branch from d225329 to e793ec6 Compare September 27, 2021 16:01

tswast changed the title ~~fix: support constucting empty time Series~~ fix: support converting empty time Series to pyarrow Array Sep 27, 2021

tswast marked this pull request as ready for review September 27, 2021 20:35

tswast requested a review from a team as a code owner September 27, 2021 20:35

use object dtype for time numpy array

e08f7fc

tswast commented Sep 27, 2021

View reviewed changes

tswast requested review from plamut and jimfulton September 27, 2021 20:38

tswast mentioned this pull request Sep 27, 2021

test: No warnings #9

Merged

4 tasks

jimfulton reviewed Sep 28, 2021

View reviewed changes

plamut reviewed Sep 28, 2021

View reviewed changes

backport to_numpy

281afb9

tswast requested a review from plamut September 28, 2021 20:48

tswast added 2 commits September 28, 2021 17:02

Merge remote-tracking branch 'upstream/main' into issue10-arrow-coverage

1613bb6

remove redundant test

63b946c

tswast commented Sep 29, 2021

View reviewed changes

plamut approved these changes Sep 29, 2021

View reviewed changes

tswast merged commit 7675b15 into main Sep 29, 2021

tswast deleted the issue10-arrow-coverage branch September 29, 2021 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support converting empty `time` Series to pyarrow Array #11

fix: support converting empty `time` Series to pyarrow Array #11

tswast commented Sep 27, 2021 •

edited

tswast commented Sep 27, 2021

tswast commented Sep 27, 2021

tswast commented Sep 27, 2021

tswast Sep 27, 2021

jimfulton Sep 28, 2021

tswast Sep 28, 2021

jimfulton Sep 28, 2021

tswast Sep 28, 2021

plamut left a comment

tswast commented Sep 28, 2021

tswast Sep 29, 2021

fix: support converting empty time Series to pyarrow Array #11

fix: support converting empty time Series to pyarrow Array #11

Conversation

tswast commented Sep 27, 2021 • edited

tswast commented Sep 27, 2021

tswast commented Sep 27, 2021

tswast commented Sep 27, 2021

tswast Sep 27, 2021

Choose a reason for hiding this comment

jimfulton Sep 28, 2021

Choose a reason for hiding this comment

tswast Sep 28, 2021

Choose a reason for hiding this comment

jimfulton Sep 28, 2021

Choose a reason for hiding this comment

tswast Sep 28, 2021

Choose a reason for hiding this comment

plamut left a comment

Choose a reason for hiding this comment

tswast commented Sep 28, 2021

tswast Sep 29, 2021

Choose a reason for hiding this comment

fix: support converting empty `time` Series to pyarrow Array #11

fix: support converting empty `time` Series to pyarrow Array #11

tswast commented Sep 27, 2021 •

edited