[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…

2020-06-23 Thread GitBox 


maxburke commented on a change in pull request #7500:
URL: https://github.com/apache/arrow/pull/7500#discussion_r70083



##
File path: rust/parquet/src/record/api.rs
##
@@ -893,16 +893,6 @@ mod tests {
 assert_eq!(row, Field::TimestampMillis(123854406));
 }
 
-#[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(, 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:
us...@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…

2020-06-23 Thread GitBox 


maxburke commented on a change in pull request #7500:
URL: https://github.com/apache/arrow/pull/7500#discussion_r70083



##
File path: rust/parquet/src/record/api.rs
##
@@ -893,16 +893,6 @@ mod tests {
 assert_eq!(row, Field::TimestampMillis(123854406));
 }
 
-#[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(, 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:
us...@infra.apache.org