jorisvandenbossche commented on code in PR #13718:
URL: https://github.com/apache/arrow/pull/13718#discussion_r937424636
##########
cpp/src/arrow/python/datetime.h:
##########
@@ -147,14 +151,25 @@ inline int64_t PyDelta_to_ms(PyDateTime_Delta*
pytimedelta) {
}
ARROW_PYTHON_EXPORT
-inline int64_t PyDelta_to_us(PyDateTime_Delta* pytimedelta) {
- return (PyDelta_to_s(pytimedelta) * 1000000LL +
- PyDateTime_DELTA_GET_MICROSECONDS(pytimedelta));
+// TODO which PyDeltas do we cover?
Review Comment:
What's the question exactly?
##########
cpp/src/arrow/python/datetime.h:
##########
@@ -35,6 +37,8 @@
#define PyDateTimeAPI ::arrow::py::internal::datetime_api
namespace arrow {
+using internal::MultiplyWithOverflow;
+using internal::AddWithOverflow;
Review Comment:
Either you can leave this out, or either you don't have to include the
`arrow::internal::` below when using those, I think
##########
python/pyarrow/tests/test_types.py:
##########
@@ -845,6 +845,11 @@ def test_decimal_overflow():
with pytest.raises(ValueError):
pa.decimal256(i, 0)
+def test_timedelta_overflow()::
+ d = datetime.timedelta(days=-106751992, seconds=71945, microseconds=224192)
+ with pytest.raises(pa.ArrowInvalid):
+ pa.scalar(d)
Review Comment:
The current test looks good, but will only test it for a single resolution
(the default, which is micro, I think), and thus not cover all your changes. So
maybe add some additional cases where you pass the data type (with specific
resolution) explicitly)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
