Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-37377: [C#] Throw OverflowException on overflow in TimestampArray.ConvertTo() #37388

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

DanTm99
Copy link
Contributor

@DanTm99 DanTm99 commented Aug 25, 2023

Throw OverflowException on overflow in TimestampArray.ConvertTo() when DataType.Unit is Nanosecond and ticks is large, instead of silently overflowing and returning the wrong value.

@github-actions
Copy link

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

@DanTm99
Copy link
Contributor Author

DanTm99 commented Sep 22, 2023

@westonpace

Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

I think I could go either way on this. Timestamps are int64 and so I wouldn't expect this overflow to be a frequent occurrence. Also, this ConvertTo could be on the hot loop for something doing a lot of array building.

@CurtHagenlocher , do you have an opinion?

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Sep 25, 2023
@CurtHagenlocher
Copy link
Contributor

I also could go either way. It's hard to predict the kind of scenarios that people might care about but I think the lower precision of DateTime vs nanos makes it a suboptimal fit anyway. Users who need the performance could do the conversion on the outside and then use the overload for appending an int64 directly.

That said, there are other places in the code where multiplication happens e.g. GetTimestampUnchecked (for a different meaning of "unchecked") and I don't know how to describe a policy for when these should be checked and when they shouldn't be.

@westonpace
Copy link
Member

Ok, let's proceed to merge. If we need fast paths in C# we can add them later or use the int64 path.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Sep 27, 2023
@westonpace westonpace merged commit 2864870 into apache:main Sep 27, 2023
10 checks passed
@westonpace westonpace removed the awaiting merge Awaiting merge label Sep 27, 2023
@conbench-apache-arrow
Copy link

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

There were no benchmark performance regressions. 🎉

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

etseidl pushed a commit to etseidl/arrow that referenced this pull request Sep 28, 2023
…Array.ConvertTo() (apache#37388)

Throw `OverflowException` on overflow in `TimestampArray.ConvertTo()` when `DataType.Unit` is `Nanosecond` and `ticks` is large, instead of silently overflowing and returning the wrong value.
* Closes: apache#37377

Authored-by: Danyaal Khan <danyaal99@hotmail.co.uk>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 23, 2023
…Array.ConvertTo() (apache#37388)

Throw `OverflowException` on overflow in `TimestampArray.ConvertTo()` when `DataType.Unit` is `Nanosecond` and `ticks` is large, instead of silently overflowing and returning the wrong value.
* Closes: apache#37377

Authored-by: Danyaal Khan <danyaal99@hotmail.co.uk>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…Array.ConvertTo() (apache#37388)

Throw `OverflowException` on overflow in `TimestampArray.ConvertTo()` when `DataType.Unit` is `Nanosecond` and `ticks` is large, instead of silently overflowing and returning the wrong value.
* Closes: apache#37377

Authored-by: Danyaal Khan <danyaal99@hotmail.co.uk>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…Array.ConvertTo() (apache#37388)

Throw `OverflowException` on overflow in `TimestampArray.ConvertTo()` when `DataType.Unit` is `Nanosecond` and `ticks` is large, instead of silently overflowing and returning the wrong value.
* Closes: apache#37377

Authored-by: Danyaal Khan <danyaal99@hotmail.co.uk>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
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.

[C#] TimestampArray.ConvertTo() returns erroneous value for large ticks when returning nanoseconds
3 participants