raulcd commented on code in PR #50444:
URL: https://github.com/apache/arrow/pull/50444#discussion_r3552289014
##########
python/pyarrow/src/arrow/python/arrow_to_pandas.cc:
##########
@@ -1293,7 +1293,7 @@ struct ObjectWriterVisitor {
PyObject** out) {
ARROW_DCHECK(internal::BorrowPandasDataOffsetType() != nullptr);
// DateOffset objects do not add nanoseconds component to pd.Timestamp.
- // as of Pandas 1.3.3
+ // as of Pandas 1.3.3
// (https://github.com/pandas-dev/pandas/issues/43892).
Review Comment:
It could be simplified, something like:
```diff
diff --git a/python/pyarrow/src/arrow/python/arrow_to_pandas.cc
b/python/pyarrow/src/arrow/python/arrow_to_pandas.cc
index 348d352a04..581da12577 100644
--- a/python/pyarrow/src/arrow/python/arrow_to_pandas.cc
+++ b/python/pyarrow/src/arrow/python/arrow_to_pandas.cc
@@ -1292,24 +1292,10 @@ struct ObjectWriterVisitor {
auto to_date_offset = [&](const
MonthDayNanoIntervalType::MonthDayNanos& interval,
PyObject** out) {
ARROW_DCHECK(internal::BorrowPandasDataOffsetType() != nullptr);
- // DateOffset objects do not add nanoseconds component to
pd.Timestamp.
- // as of Pandas 1.3.3
- // (https://github.com/pandas-dev/pandas/issues/43892).
- // So convert microseconds and remainder to preserve data
- // but give users more expected results.
- int64_t microseconds = interval.nanoseconds / 1000;
- int64_t nanoseconds;
- if (interval.nanoseconds >= 0) {
- nanoseconds = interval.nanoseconds % 1000;
- } else {
- nanoseconds = -((-interval.nanoseconds) % 1000);
- }
PyDict_SetItemString(kwargs.obj(), "months",
PyLong_FromLong(interval.months));
PyDict_SetItemString(kwargs.obj(), "days",
PyLong_FromLong(interval.days));
- PyDict_SetItemString(kwargs.obj(), "microseconds",
- PyLong_FromLongLong(microseconds));
- PyDict_SetItemString(kwargs.obj(), "nanoseconds",
PyLong_FromLongLong(nanoseconds));
+ PyDict_SetItemString(kwargs.obj(), "nanoseconds",
PyLong_FromLongLong(interval.nanoseconds));
*out =
PyObject_Call(internal::BorrowPandasDataOffsetType(), args.obj(),
kwargs.obj());
RETURN_IF_PYERROR();
```
but I think this is a small change on the UX, nowadays we don't roundtrip
hours/minutes on `DateOffset` we show microseconds and nanoseconds. With this
change we will be consistent and we would show, Month/Day/Nano instead of
Month/Day/Microseconds/Nano. I think this is worth it but maybe we should do it
as a separate issue from this one around dropping older pandas supprt and
discuss there whether the UX change is worth?
Are you ok if I move this to its own issue?
--
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]
