arrow icon indicating copy to clipboard operation
arrow copied to clipboard

ARROW-15026: [Python] Error if datetime.timedelta to pyarrow.duration conversion overflows

Open anjakefala opened this issue 1 year ago • 7 comments

anjakefala avatar Jul 26 '22 20:07 anjakefala

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

github-actions[bot] avatar Jul 26 '22 21:07 github-actions[bot]

:warning: Ticket has not been started in JIRA, please click 'Start Progress'.

github-actions[bot] avatar Jul 26 '22 21:07 github-actions[bot]

Thanks for taking over!

Question: you only added overflow checks for micro and nano resolutions. Do we need that for second and milli as well, or will those never run into an overflow? (since a python timedelta also stores days, I would assume also second can still run into an overflow)

Both datetime.timedelta.min and datetime.timedelta.max fit into int64 at s and ms resolution.

notEvil avatar Aug 04 '22 09:08 notEvil

Both datetime.timedelta.min and datetime.timedelta.max fit into int64 at s and ms resolution.

Ah, indeed, the days component being limited to 999999999 (and not just any int) means that this will always fit for s/ms

jorisvandenbossche avatar Aug 09 '22 20:08 jorisvandenbossche

@anjakefala small workflow question: can you add new changes after a review as a new commit instead of combining it in the original commit? That makes it easier to see what changes since the last review.

jorisvandenbossche avatar Aug 09 '22 20:08 jorisvandenbossche

@jorisvandenbossche Oh! Yes, of course! I find it easier to rebase along the way, instead of needing to do one messy rebase in the end, if I have more than one unit of work in the PR. This PR is only a single unit of work, so it is straightforward to just add commits. Screenshot from 2022-08-09 16-19-58

I do not mind making this workflow shift, but in the GitHub UI, you can click compare on the Force Push to see what the difference is.

Is it generally the process to add messy commits, and then everything gets squashed into a single commit in the end?

anjakefala avatar Aug 09 '22 20:08 anjakefala

I am trying to set up clang linting in my local enviornment for Arrow, which is why this PR is currently quiet.

anjakefala avatar Aug 09 '22 20:08 anjakefala

Thank you @jorisvandenbossche and @wjones127 and @notEvil!

anjakefala avatar Nov 15 '22 19:11 anjakefala

Benchmark runs are scheduled for baseline = c3cfc7934ebdc652399af95b8696bd5a05d943fa and contender = 4d8c21bd303833c124f9d5801755c953c6c3260e. 4d8c21bd303833c124f9d5801755c953c6c3260e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. Conbench compare runs links: [Finished :arrow_down:0.0% :arrow_up:0.0%] ec2-t3-xlarge-us-east-2 [Finished :arrow_down:0.57% :arrow_up:0.07%] test-mac-arm [Finished :arrow_down:0.27% :arrow_up:0.82%] ursa-i9-9960x [Finished :arrow_down:0.84% :arrow_up:0.07%] ursa-thinkcentre-m75q Buildkite builds: [Finished] 4d8c21bd ec2-t3-xlarge-us-east-2 [Finished] 4d8c21bd test-mac-arm [Finished] 4d8c21bd ursa-i9-9960x [Finished] 4d8c21bd ursa-thinkcentre-m75q [Finished] c3cfc793 ec2-t3-xlarge-us-east-2 [Finished] c3cfc793 test-mac-arm [Finished] c3cfc793 ursa-i9-9960x [Finished] c3cfc793 ursa-thinkcentre-m75q Supported benchmarks: ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True test-mac-arm: Supported benchmark langs: C++, Python, R ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

ursabot avatar Nov 16 '22 05:11 ursabot