You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/08/18 13:19:01 UTC
[GitHub] [arrow] jorisvandenbossche opened a new pull request #7987: ARROW-9768: [Python] Check overflow in conversion of datetime objects to nanosecond timestamps
jorisvandenbossche opened a new pull request #7987:
URL: https://github.com/apache/arrow/pull/7987
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7987: ARROW-9768: [Python] Check overflow in conversion of datetime objects to nanosecond timestamps
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7987:
URL: https://github.com/apache/arrow/pull/7987#issuecomment-675473829
https://issues.apache.org/jira/browse/ARROW-9768
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on pull request #7987: ARROW-9768: [Python] Check overflow in conversion of datetime objects to nanosecond timestamps
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on pull request #7987:
URL: https://github.com/apache/arrow/pull/7987#issuecomment-677749625
> Because int_util_internal.h indirectly includes some more Windows headers (through a vendored dependency).
OK, thanks for fixing that!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on a change in pull request #7987: ARROW-9768: [Python] Check overflow in conversion of datetime objects to nanosecond timestamps
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #7987:
URL: https://github.com/apache/arrow/pull/7987#discussion_r473911916
##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -263,7 +264,12 @@ struct ValueConverter<TimestampType> {
value = internal::PyDateTime_to_us(dt) - offset * 1000 * 1000;
break;
case TimeUnit::NANO:
- value = internal::PyDateTime_to_ns(dt) - offset * 1000 * 1000 * 1000;
+ // Conversion to nanoseconds can overflow -> check multiply of microseconds
+ value = internal::PyDateTime_to_us(dt);
+ if (arrow::internal::MultiplyWithOverflow(value, 1000, &value)) {
+ return internal::InvalidValue(obj, "out of bounds for nanosecond resolution");
+ }
+ value -= offset * 1000 * 1000 * 1000;
Review comment:
Ah, the offset itself won't overflow, but for the *subtraction* (as you actually commented ..) that could indeed happen in a small corner case: if your naive timezone is close to the lower or upper limit of the nanosecond range, applying the offset could indeed overflow.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7987: ARROW-9768: [Python] Check overflow in conversion of datetime objects to nanosecond timestamps
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7987:
URL: https://github.com/apache/arrow/pull/7987#issuecomment-677746192
@jorisvandenbossche Because `int_util_internal.h` indirectly includes some more Windows headers (through a vendored dependency).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #7987: ARROW-9768: [Python] Check overflow in conversion of datetime objects to nanosecond timestamps
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7987:
URL: https://github.com/apache/arrow/pull/7987#discussion_r473907662
##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -263,7 +264,12 @@ struct ValueConverter<TimestampType> {
value = internal::PyDateTime_to_us(dt) - offset * 1000 * 1000;
break;
case TimeUnit::NANO:
- value = internal::PyDateTime_to_ns(dt) - offset * 1000 * 1000 * 1000;
+ // Conversion to nanoseconds can overflow -> check multiply of microseconds
+ value = internal::PyDateTime_to_us(dt);
+ if (arrow::internal::MultiplyWithOverflow(value, 1000, &value)) {
+ return internal::InvalidValue(obj, "out of bounds for nanosecond resolution");
+ }
+ value -= offset * 1000 * 1000 * 1000;
Review comment:
This subtraction could overflow as well, no?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #7987: ARROW-9768: [Python] Check overflow in conversion of datetime objects to nanosecond timestamps
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #7987:
URL: https://github.com/apache/arrow/pull/7987
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on a change in pull request #7987: ARROW-9768: [Python] Check overflow in conversion of datetime objects to nanosecond timestamps
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #7987:
URL: https://github.com/apache/arrow/pull/7987#discussion_r473910873
##########
File path: cpp/src/arrow/python/python_to_arrow.cc
##########
@@ -263,7 +264,12 @@ struct ValueConverter<TimestampType> {
value = internal::PyDateTime_to_us(dt) - offset * 1000 * 1000;
break;
case TimeUnit::NANO:
- value = internal::PyDateTime_to_ns(dt) - offset * 1000 * 1000 * 1000;
+ // Conversion to nanoseconds can overflow -> check multiply of microseconds
+ value = internal::PyDateTime_to_us(dt);
+ if (arrow::internal::MultiplyWithOverflow(value, 1000, &value)) {
+ return internal::InvalidValue(obj, "out of bounds for nanosecond resolution");
+ }
+ value -= offset * 1000 * 1000 * 1000;
Review comment:
The offset is always a small number (time zone offsets are always less then 12 hours ?), so shouldn't give any problem?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on pull request #7987: ARROW-9768: [Python] Check overflow in conversion of datetime objects to nanosecond timestamps
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on pull request #7987:
URL: https://github.com/apache/arrow/pull/7987#issuecomment-677479161
In principle, we have a `safe=True/False` keyword in `pa.array(..)` that could be honored here, and only do the overflow check if `safe=True` (but we currently don't discriminate for safe=True/False anywhere in python_to_arrow.cc)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7987: ARROW-9768: [Python] Check overflow in conversion of datetime objects to nanosecond timestamps
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7987:
URL: https://github.com/apache/arrow/pull/7987#issuecomment-677746868
I will presume the s390x build is ok and merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on pull request #7987: ARROW-9768: [Python] Check overflow in conversion of datetime objects to nanosecond timestamps
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on pull request #7987:
URL: https://github.com/apache/arrow/pull/7987#issuecomment-677719373
@pitrou do you know why the windows macro thing comes up in this PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org