ARROW-5855: [Python] Support for Duration (timedelta) type #5566

jorisvandenbossche · 2019-10-02T15:20:37Z

https://issues.apache.org/jira/browse/ARROW-5855

Basic wrappers for DurationArray/Type/Value
Numpy / pandas conversion
Python conversion

github-actions · 2019-10-02T15:31:07Z

https://issues.apache.org/jira/browse/ARROW-5855

jorisvandenbossche · 2019-10-03T13:16:47Z

cpp/src/arrow/python/python_to_arrow.cc

@@ -449,6 +449,63 @@ class TimestampConverter
  TimeUnit::type unit_;
 };

+template <NullCoding null_coding>
+class DurationConverter


This is almost literally a copy paste of the TimestampConverter above with timestamp-things replaced with duration-things. Is there a way to deduplicate some code?
Templating this might be difficult as I also need to switch Python / numpy C API calls? (PyDateTime_DateTime -> PyDateTime_Delta, PyDatetimeScalarObject -> PyTimedeltaScalarObject, ...)

Can use something like this:

template <typename ArrowType> struct PyDateTimeTraits {}; template <> struct PyDateTimeTraits<TimestampType> { using PyTypeCheck = PyDateTime_Check; using PyDateTimeObject = PyDateTime_DateTime; using py_to_s = internal::PyDateTime_to_s; using py_to_ms = internal::PyDateTime_to_ms; using py_to_us = internal::PyDateTime_to_us; using py_to_ns = internal::PyDateTime_to_ns; using np_traits = internal::npy_traits<NPY_DATETIME>; using NpScalarObject = PyDatetimeScalarObject; };

It seems that using can only be used with type names, and not function names?
(I could define an inline function that wraps the correct function for each, but that gets a bit verbose?)

Inline functions sound fine.

OK, I pushed a version with the template (I am personally not fully sure the end result is easier / better than some duplicated code, but ok ..)

jorisvandenbossche · 2019-10-03T14:45:16Z

Also opened a few follow-up items that I ran into: https://issues.apache.org/jira/browse/ARROW-6778 (casting), overflow in conversion to ns (https://issues.apache.org/jira/browse/ARROW-6779) and parquet support (https://issues.apache.org/jira/browse/ARROW-6780).

Another question is if we want to call this type "timedelta" instead of "duration" in the python interface (like we also have pa.float64() and not pa.double()), since standard library, numpy and pandas all call this "timedelta").
Although I actually see this float64 / double is rather inconsistent (as the type you create with pa.float64still prints as "double")

pitrou

Looks globally fine, but some questions?

pitrou · 2019-10-03T15:01:23Z

cpp/src/arrow/python/arrow_to_pandas.cc

@@ -230,6 +231,26 @@ inline void set_numpy_metadata(int type, const DataType* datatype, PyArray_Descr
      // datatype->type == Type::DATE64
      date_dtype->meta.base = NPY_FR_D;
    }
+  } else if (type == NPY_TIMEDELTA) {
+    auto date_dtype = reinterpret_cast<PyArray_DatetimeDTypeMetaData*>(out->c_metadata);
+    if (datatype->id() == Type::DURATION) {


What happens if not?

That shouldn't happen (as NPY_TIMEDELTA should only be passed in case of Type::DURATION). So I can just leave out the if check? (and maybe add a debug assert?)

Yes, add a debug assert.

cpp/src/arrow/python/arrow_to_pandas.cc

cpp/src/arrow/python/datetime.h

pitrou · 2019-10-03T15:22:37Z

cpp/src/arrow/python/python_to_arrow.cc

@@ -449,6 +449,63 @@ class TimestampConverter
  TimeUnit::type unit_;
 };

+template <NullCoding null_coding>
+class DurationConverter


Can use something like this:

template <typename ArrowType> struct PyDateTimeTraits {}; template <> struct PyDateTimeTraits<TimestampType> { using PyTypeCheck = PyDateTime_Check; using PyDateTimeObject = PyDateTime_DateTime; using py_to_s = internal::PyDateTime_to_s; using py_to_ms = internal::PyDateTime_to_ms; using py_to_us = internal::PyDateTime_to_us; using py_to_ns = internal::PyDateTime_to_ns; using np_traits = internal::npy_traits<NPY_DATETIME>; using NpScalarObject = PyDatetimeScalarObject; };

python/pyarrow/tests/test_array.py

pitrou · 2019-10-03T15:30:17Z

python/pyarrow/tests/test_convert_builtin.py

@@ -863,6 +867,34 @@ def test_datetime_subclassing():
    assert arr_us[0].as_py() == datetime.datetime(2007, 7, 13, 1,
                                                  23, 34, 123456)

+    data = [
+        MyTimedelta(1, 1, 1000),


Is it possible to test the conversion with more interesting values?

What values would you find interesting? (thinking about specific corner cases?)

(it's mainly the fact that the subclass is working that is being tested here)

Something less trivial than days and seconds having the same value.

pitrou · 2019-10-03T15:31:35Z

python/pyarrow/tests/test_convert_builtin.py

+@pytest.mark.parametrize('unit', ['s', 'ms', 'us', 'ns'])
+def test_sequence_duration_with_unit(unit):
+    data = [
+        datetime.timedelta(1, 1, 1001),


Would be nice to test with more interesting values here as well...

pitrou · 2019-10-03T15:34:06Z

python/pyarrow/tests/test_scalars.py

+    def test_duration(self):
+        arr = np.array([0, 3600000000000], dtype='timedelta64[ns]')
+
+        units = ['us', 'ms', 's']


Why not test nanoseconds as well?

Nanoseconds are tested below separately, as they have a different behaviour for with or without pandas

python/pyarrow/tests/test_scalars.py

jorisvandenbossche

Thanks for the review! Will update for your suggestions later

jorisvandenbossche · 2019-10-03T15:52:43Z

cpp/src/arrow/python/arrow_to_pandas.cc

@@ -230,6 +231,26 @@ inline void set_numpy_metadata(int type, const DataType* datatype, PyArray_Descr
      // datatype->type == Type::DATE64
      date_dtype->meta.base = NPY_FR_D;
    }
+  } else if (type == NPY_TIMEDELTA) {
+    auto date_dtype = reinterpret_cast<PyArray_DatetimeDTypeMetaData*>(out->c_metadata);
+    if (datatype->id() == Type::DURATION) {


That shouldn't happen (as NPY_TIMEDELTA should only be passed in case of Type::DURATION). So I can just leave out the if check? (and maybe add a debug assert?)

cpp/src/arrow/python/arrow_to_pandas.cc

cpp/src/arrow/python/datetime.h

jorisvandenbossche · 2019-10-03T16:08:18Z

python/pyarrow/tests/test_array.py

@@ -926,6 +931,16 @@ def test_cast_date32_to_int():
    assert result2.equals(arr)


+@pytest.mark.xfail(strict=True)  # TODO implement duration cast


I already opened https://issues.apache.org/jira/browse/ARROW-6778 for that (but I also already wrote the test when I found out, so thought to just keep it)

python/pyarrow/tests/test_array.py

jorisvandenbossche · 2019-10-03T16:11:33Z

python/pyarrow/tests/test_convert_builtin.py

@@ -863,6 +867,34 @@ def test_datetime_subclassing():
    assert arr_us[0].as_py() == datetime.datetime(2007, 7, 13, 1,
                                                  23, 34, 123456)

+    data = [
+        MyTimedelta(1, 1, 1000),


What values would you find interesting? (thinking about specific corner cases?)

(it's mainly the fact that the subclass is working that is being tested here)

jorisvandenbossche · 2019-10-03T16:12:48Z

python/pyarrow/tests/test_scalars.py

+    def test_duration(self):
+        arr = np.array([0, 3600000000000], dtype='timedelta64[ns]')
+
+        units = ['us', 'ms', 's']


Nanoseconds are tested below separately, as they have a different behaviour for with or without pandas

python/pyarrow/tests/test_scalars.py

jorisvandenbossche

Updated for your feedback! Added some additional questions

cpp/src/arrow/python/arrow_to_pandas.cc

jorisvandenbossche · 2019-10-04T07:03:11Z

cpp/src/arrow/python/python_to_arrow.cc

@@ -449,6 +449,63 @@ class TimestampConverter
  TimeUnit::type unit_;
 };

+template <NullCoding null_coding>
+class DurationConverter


It seems that using can only be used with type names, and not function names?
(I could define an inline function that wraps the correct function for each, but that gets a bit verbose?)

jorisvandenbossche · 2019-10-04T07:05:02Z

@ursabot crossbow submit docker-python-3.6-nopandas

ursabot · 2019-10-04T07:05:31Z

AMD64 Conda Crossbow Submit (#67612) builder has been succeeded.

Revision: f35aa25

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-238

Task	Status
docker-python-3.6-nopandas

codecov-io · 2019-10-04T15:55:32Z

Codecov Report

Merging #5566 into master will increase coverage by 0.55%.
The diff coverage is 84.22%.

@@            Coverage Diff             @@
##           master    #5566      +/-   ##
==========================================
+ Coverage   88.79%   89.35%   +0.55%     
==========================================
  Files         983      791     -192     
  Lines      132170   117358   -14812     
  Branches     1501        0    -1501     
==========================================
- Hits       117362   104865   -12497     
+ Misses      14443    12493    -1950     
+ Partials      365        0     -365

Impacted Files	Coverage Δ
python/pyarrow/tests/test_schema.py	`100% <ø> (ø)`	⬆️
python/pyarrow/__init__.py	`71.25% <ø> (ø)`	⬆️
python/pyarrow/tests/test_misc.py	`94.73% <ø> (ø)`	⬆️
python/pyarrow/array.pxi	`79.37% <0%> (-0.16%)`	⬇️
python/pyarrow/tests/test_types.py	`97.33% <100%> (+0.11%)`	⬆️
python/pyarrow/tests/test_convert_builtin.py	`97.5% <100%> (+0.25%)`	⬆️
python/pyarrow/types.py	`97.89% <100%> (+0.04%)`	⬆️
cpp/src/arrow/python/numpy_to_arrow.cc	`95.96% <100%> (+0.01%)`	⬆️
cpp/src/arrow/python/datetime.h	`100% <100%> (ø)`	⬆️
python/pyarrow/tests/test_array.py	`93.52% <100%> (-0.07%)`	⬇️
... and 231 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af097e6...cd9f12a. Read the comment docs.

pitrou

+1, just a nit

pitrou · 2019-10-07T12:50:25Z

cpp/src/arrow/python/python_to_arrow.cc

+  static constexpr const char* const np_name = "timedelta64";
+};
+
+template <NullCoding null_coding, typename Type>


Nit: call this ArrowType?

jorisvandenbossche · 2019-10-08T08:32:16Z

@pitrou updated, and all green!

pitrou · 2019-10-08T11:13:10Z

Thanks @jorisvandenbossche !

jorisvandenbossche · 2019-10-08T11:17:33Z

And thanks a lot for your review and help!

jorisvandenbossche commented Oct 3, 2019

View reviewed changes

jorisvandenbossche force-pushed the ARROW-5855-duration-python branch from 725c49c to c38a20a Compare October 3, 2019 13:38

pitrou reviewed Oct 3, 2019

View reviewed changes

jorisvandenbossche commented Oct 3, 2019

View reviewed changes

jorisvandenbossche commented Oct 4, 2019

View reviewed changes

kszucs force-pushed the master branch from fc93312 to af097e6 Compare October 5, 2019 09:47

jorisvandenbossche added 6 commits October 5, 2019 12:03

ARROW-5855: [Python] Support for Duration (timedelta) type

03d3992

implement python conversion

779e724

clean-up

6bfafbe

handle python 2 compat

f4d6b4b

update for feedback

56bd1b3

templated TimestampConverter

7458000

kszucs force-pushed the ARROW-5855-duration-python branch from 9b3f61d to 7458000 Compare October 5, 2019 10:03

pitrou approved these changes Oct 7, 2019

View reviewed changes

Type -> ArrowType

cd9f12a

pitrou closed this in c805b5f Oct 8, 2019

jorisvandenbossche deleted the ARROW-5855-duration-python branch October 8, 2019 11:17

asfimport mentioned this pull request Oct 8, 2019

[Python] Add support for Duration type #22273

Closed

abhiaagarwal mentioned this pull request Apr 1, 2024

Upgrade to pyo3 0.21.0 delta-io/delta-rs#2363

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-5855: [Python] Support for Duration (timedelta) type #5566

ARROW-5855: [Python] Support for Duration (timedelta) type #5566

jorisvandenbossche commented Oct 2, 2019 •

edited

github-actions bot commented Oct 2, 2019

jorisvandenbossche Oct 3, 2019

pitrou Oct 3, 2019

jorisvandenbossche Oct 4, 2019

pitrou Oct 4, 2019

jorisvandenbossche Oct 4, 2019

jorisvandenbossche commented Oct 3, 2019

pitrou left a comment

pitrou Oct 3, 2019

jorisvandenbossche Oct 3, 2019

pitrou Oct 3, 2019

pitrou Oct 3, 2019

pitrou Oct 3, 2019

jorisvandenbossche Oct 3, 2019

pitrou Oct 3, 2019

pitrou Oct 3, 2019

pitrou Oct 3, 2019

jorisvandenbossche Oct 3, 2019

jorisvandenbossche left a comment

jorisvandenbossche Oct 3, 2019

jorisvandenbossche Oct 3, 2019

jorisvandenbossche Oct 3, 2019

jorisvandenbossche Oct 3, 2019

jorisvandenbossche left a comment

jorisvandenbossche Oct 4, 2019

jorisvandenbossche commented Oct 4, 2019

ursabot commented Oct 4, 2019

codecov-io commented Oct 4, 2019 •

edited

pitrou left a comment

pitrou Oct 7, 2019

jorisvandenbossche commented Oct 8, 2019

pitrou commented Oct 8, 2019

jorisvandenbossche commented Oct 8, 2019

		@@ -926,6 +931,16 @@ def test_cast_date32_to_int():
		assert result2.equals(arr)


		@pytest.mark.xfail(strict=True) # TODO implement duration cast

ARROW-5855: [Python] Support for Duration (timedelta) type #5566

ARROW-5855: [Python] Support for Duration (timedelta) type #5566

Conversation

jorisvandenbossche commented Oct 2, 2019 • edited

github-actions bot commented Oct 2, 2019

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

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 3, 2019

pitrou left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche left a comment

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

Choose a reason for hiding this comment

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 4, 2019

ursabot commented Oct 4, 2019

codecov-io commented Oct 4, 2019 • edited

Codecov Report

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 8, 2019

pitrou commented Oct 8, 2019

jorisvandenbossche commented Oct 8, 2019

jorisvandenbossche commented Oct 2, 2019 •

edited

codecov-io commented Oct 4, 2019 •

edited