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: us...@infra.apache.org