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/06/19 22:58:46 UTC
[GitHub] [arrow] maxburke opened a new pull request #7500: ARROW-9191: [Rust] Do not panic when milliseconds is less than zero as chrono can handle…
maxburke opened a new pull request #7500:
URL: https://github.com/apache/arrow/pull/7500
… these formats
----------------------------------------------------------------
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] nevi-me closed pull request #7500: ARROW-9191: [Rust] Do not panic when milliseconds is less than zero as chrono can handle…
Posted by GitBox <gi...@apache.org>.
nevi-me closed pull request #7500:
URL: https://github.com/apache/arrow/pull/7500
----------------------------------------------------------------
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 #7500: ARROW-9191: [Rust] Do not panic when milliseconds is less than zero as chrono can handle…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7500:
URL: https://github.com/apache/arrow/pull/7500#issuecomment-646890275
https://issues.apache.org/jira/browse/ARROW-9191
----------------------------------------------------------------
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] nevi-me commented on pull request #7500: ARROW-9191: [Rust] Do not panic when milliseconds is less than zero as chrono can handle…
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7500:
URL: https://github.com/apache/arrow/pull/7500#issuecomment-651650955
I've merged this, I've been having computer issues so haven't been able to do much work
----------------------------------------------------------------
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] paddyhoran commented on a change in pull request #7500: ARROW-9191: [Rust] Do not panic when milliseconds is less than zero as chrono can handle…
Posted by GitBox <gi...@apache.org>.
paddyhoran commented on a change in pull request #7500:
URL: https://github.com/apache/arrow/pull/7500#discussion_r446258243
##########
File path: rust/parquet/src/record/api.rs
##########
@@ -893,16 +893,6 @@ mod tests {
assert_eq!(row, Field::TimestampMillis(1238544060000));
}
- #[test]
- #[should_panic(expected = "Expected non-negative milliseconds when converting Int96")]
- fn test_row_convert_int96_invalid() {
- // INT96 value does not depend on logical type
- let descr = make_column_descr![PhysicalType::INT96, LogicalType::NONE];
-
- let value = Int96::from(vec![0, 0, 0]);
- Field::convert_int96(&descr, value);
- }
-
Review comment:
Yea, I agree with the direction. I was just wondering if we were relying on this assumption (non-negative milliseconds) elsewhere in the codebase. I'll merge this soon if there are no other comments.
----------------------------------------------------------------
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] paddyhoran commented on a change in pull request #7500: ARROW-9191: [Rust] Do not panic when milliseconds is less than zero as chrono can handle…
Posted by GitBox <gi...@apache.org>.
paddyhoran commented on a change in pull request #7500:
URL: https://github.com/apache/arrow/pull/7500#discussion_r444443777
##########
File path: rust/parquet/src/record/api.rs
##########
@@ -893,16 +893,6 @@ mod tests {
assert_eq!(row, Field::TimestampMillis(1238544060000));
}
- #[test]
- #[should_panic(expected = "Expected non-negative milliseconds when converting Int96")]
- fn test_row_convert_int96_invalid() {
- // INT96 value does not depend on logical type
- let descr = make_column_descr![PhysicalType::INT96, LogicalType::NONE];
-
- let value = Int96::from(vec![0, 0, 0]);
- Field::convert_int96(&descr, value);
- }
-
Review comment:
Are there other tests we should be adding to test the effect of allowing negative milliseconds to flow through?
I'm not that familiar with Parquet but I see things like [this](https://github.com/apache/arrow/blob/master/rust/parquet/src/record/api.rs#L584) which seem to indicate that we might be relying on milliseconds being non-negative.
----------------------------------------------------------------
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] maxburke commented on a change in pull request #7500: ARROW-9191: [Rust] Do not panic when milliseconds is less than zero as chrono can handle…
Posted by GitBox <gi...@apache.org>.
maxburke commented on a change in pull request #7500:
URL: https://github.com/apache/arrow/pull/7500#discussion_r444470083
##########
File path: rust/parquet/src/record/api.rs
##########
@@ -893,16 +893,6 @@ mod tests {
assert_eq!(row, Field::TimestampMillis(1238544060000));
}
- #[test]
- #[should_panic(expected = "Expected non-negative milliseconds when converting Int96")]
- fn test_row_convert_int96_invalid() {
- // INT96 value does not depend on logical type
- let descr = make_column_descr![PhysicalType::INT96, LogicalType::NONE];
-
- let value = Int96::from(vec![0, 0, 0]);
- Field::convert_int96(&descr, value);
- }
-
Review comment:
As a consumer of Parquet and Arrow I'm trying to not have my program panic when it encounters a date from 1968.
Perhaps it's a mistake to have the timestamp types implemented with unsigned integers? Should these be signed?
Regardless, from an API design point of view, I think that Parquet is the wrong place to be defending against (potential) gaps in functionality of non-project libraries like chrono.
----------------------------------------------------------------
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] maxburke commented on a change in pull request #7500: ARROW-9191: [Rust] Do not panic when milliseconds is less than zero as chrono can handle…
Posted by GitBox <gi...@apache.org>.
maxburke commented on a change in pull request #7500:
URL: https://github.com/apache/arrow/pull/7500#discussion_r444470083
##########
File path: rust/parquet/src/record/api.rs
##########
@@ -893,16 +893,6 @@ mod tests {
assert_eq!(row, Field::TimestampMillis(1238544060000));
}
- #[test]
- #[should_panic(expected = "Expected non-negative milliseconds when converting Int96")]
- fn test_row_convert_int96_invalid() {
- // INT96 value does not depend on logical type
- let descr = make_column_descr![PhysicalType::INT96, LogicalType::NONE];
-
- let value = Int96::from(vec![0, 0, 0]);
- Field::convert_int96(&descr, value);
- }
-
Review comment:
As a consumer of Parquet and Arrow I'm trying to not have my program panic when it encounters a date from 1968.
Perhaps it's a mistake to have the timestamp types implemented with unsigned integers? Should these be signed?
Regardless, from an API design point of view, I think that Parquet is the wrong place to be defending against (potential) gaps in functionality of non-project libraries like chrono. As a consumer, I'd rather receive the panic from chrono than from Parquet :)
----------------------------------------------------------------
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] houqp commented on a change in pull request #7500: ARROW-9191: [Rust] Do not panic when milliseconds is less than zero as chrono can handle…
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #7500:
URL: https://github.com/apache/arrow/pull/7500#discussion_r445998007
##########
File path: rust/parquet/src/record/api.rs
##########
@@ -893,16 +893,6 @@ mod tests {
assert_eq!(row, Field::TimestampMillis(1238544060000));
}
- #[test]
- #[should_panic(expected = "Expected non-negative milliseconds when converting Int96")]
- fn test_row_convert_int96_invalid() {
- // INT96 value does not depend on logical type
- let descr = make_column_descr![PhysicalType::INT96, LogicalType::NONE];
-
- let value = Int96::from(vec![0, 0, 0]);
- Field::convert_int96(&descr, value);
- }
-
Review comment:
I agree that it's better to implement timestamp as signed, not only does it address the support for time before 1970, but also make it much easier to work with subtractions.
----------------------------------------------------------------
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