Skip to content

GH-50464: [C++][Python] Simplify arrow_to_pandas DateOffset handling for nanoseconds/milliseconds - #50465

Merged
raulcd merged 4 commits into
apache:mainfrom
raulcd:GH-50464
Jul 23, 2026
Merged

GH-50464: [C++][Python] Simplify arrow_to_pandas DateOffset handling for nanoseconds/milliseconds#50465
raulcd merged 4 commits into
apache:mainfrom
raulcd:GH-50464

Conversation

@raulcd

@raulcd raulcd commented Jul 10, 2026

Copy link
Copy Markdown
Member

Rationale for this change

Pandas 1.4 fixed the following:

Which made us special case microseconds extracting those from our MonthDayNano and manually building microseconds.

DateOffset can be created with the following:

DateOffset(years=1, months=1,
                   days=1, seconds=1, microseconds=1,
                   minutes=1, hours=1, weeks=1, nanoseconds=1)

We currently only roundtrip Month, Day, Microseconds and Nano due to our MonthDayNano data type. We special cased microseconds but we were not special casing years, seconds, minutos, hours or weeks.

As the initial issue that made us special case has been solved upstream we can stop roundtripping microseconds and just return nanoseconds simplifying our codebase.

What changes are included in this PR?

Remove the unnecessary code to retrieve microseconds from nanoseconds.
Update test as microseconds won't be returned, only nanoseconds.

Are these changes tested?

Yes via CI

Are there any user-facing changes?

Yes, this is a change of the UX.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50464 has been automatically assigned in GitHub to PR creator.

…Year, seconds, microseconds, minutes, hours, weeks
@raulcd

raulcd commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

@jorisvandenbossche would there be any reason to keep roundtriping microseconds on DateOffset(months=1, days=1, microseconds=1, nanoseconds=1) when we don't do it for years, seconds, minutes, hours, weeks?
We can simplify a little here with this PR.

@raulcd
raulcd marked this pull request as ready for review July 10, 2026 13:26
@raulcd
raulcd requested review from AlenkaF, pitrou and rok as code owners July 10, 2026 13:26
Copilot AI review requested due to automatic review settings July 10, 2026 13:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR simplifies the Pandas DateOffset conversion path for month_day_nano_interval in arrow_to_pandas by no longer splitting nanoseconds into microseconds + remainder nanoseconds, and updates Python tests to expect nanoseconds-only roundtripping.

Changes:

  • Simplify MonthDayNanoIntervalType → Pandas DateOffset conversion to pass interval.nanoseconds directly.
  • Update interval/dateoffset-related tests to assert nanoseconds-only behavior.
  • Adjust expected DateOffset construction in tests to reflect the new output shape.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
python/pyarrow/src/arrow/python/arrow_to_pandas.cc Removes microseconds/remainder splitting and passes total nanoseconds into DateOffset.
python/pyarrow/tests/test_pandas.py Updates roundtrip expectations for DateOffset values produced from month-day-nano intervals.
python/pyarrow/tests/test_array.py Updates DateOffset expectations in interval array tests to use nanoseconds-only representation.

Comment on lines +1296 to +1299
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));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this Copilot comment is right @raulcd , though obviously it already applied to the existing code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pitrou, applied the fix. I am unsure whether all RETURN_IF_PYERROR checks are necessary or one after all PyDict_SetItemString is enough as I don't think PyErr_Clear is exercised but I suppose better safe than sorry. Is there a way to know which CPython methods from the C-API clear PyErr?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally, C API functions shouldn't clear the error indicator. You should still call RETURN_IF_PYERROR after creating all the OwnedRefs, I think, lest one of them may be NULL.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is what I've done, have I missed any?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. But you need only one RETURN_IF_PYERROR after all the PyDict_SetItemString, not after each one of them.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah! I understand you now. Yes the single RETURN_IF_PYERROR after all PyDict_SetItemString is what I wasn't sure. Thanks for confirming.

Copilot AI review requested due to automatic review settings July 22, 2026 09:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 22, 2026
Copilot AI review requested due to automatic review settings July 22, 2026 10:02
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 22, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 22, 2026

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@raulcd

raulcd commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

I am going to merge this. This is currently not only simplifying part of the custom logic but fixing existing reference leaks.
If there are concerns about the UX change in the future we can discuss on a follow up.

@raulcd
raulcd merged commit 6287272 into apache:main Jul 23, 2026
39 checks passed
@raulcd raulcd removed the awaiting changes Awaiting changes label Jul 23, 2026
@raulcd
raulcd deleted the GH-50464 branch July 23, 2026 08:29
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 6287272.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants