alamb commented on code in PR #8266:
URL: https://github.com/apache/arrow-rs/pull/8266#discussion_r2326109795
##########
parquet/src/record/api.rs:
##########
@@ -928,29 +928,22 @@ fn convert_date_to_string(value: i32) -> String {
format!("{}", dt.format("%Y-%m-%d"))
}
-/// Helper method to convert Parquet timestamp into a string.
-/// Input `value` is a number of seconds since the epoch in UTC.
-/// Datetime is displayed in local timezone.
-#[inline]
-fn convert_timestamp_secs_to_string(value: i64) -> String {
- let dt = Utc.timestamp_opt(value, 0).unwrap();
- format!("{}", dt.format("%Y-%m-%d %H:%M:%S %:z"))
-}
-
/// Helper method to convert Parquet timestamp into a string.
/// Input `value` is a number of milliseconds since the epoch in UTC.
/// Datetime is displayed in local timezone.
#[inline]
fn convert_timestamp_millis_to_string(value: i64) -> String {
- convert_timestamp_secs_to_string(value / 1000)
+ let dt = Utc.timestamp_millis_opt(value).unwrap();
+ format!("{}", dt.format("%Y-%m-%d %H:%M:%S%.3f %:z"))
Review Comment:
I don't have much insight into this particular API and if anyone would treat
this as an API change
Since the existing code uses almost the same format string it seems like not
a big change to me, but I don't really know
##########
parquet/src/record/api.rs:
##########
@@ -1278,44 +1271,44 @@ mod tests {
#[test]
fn test_convert_timestamp_millis_to_string() {
- fn check_datetime_conversion(y: u32, m: u32, d: u32, h: u32, mi: u32,
s: u32) {
+ fn check_datetime_conversion(y: u32, m: u32, d: u32, h: u32, mi: u32,
s: u32, milli: u32) {
let datetime = chrono::NaiveDate::from_ymd_opt(y as i32, m, d)
.unwrap()
- .and_hms_opt(h, mi, s)
+ .and_hms_milli_opt(h, mi, s, milli)
.unwrap();
let dt = Utc.from_utc_datetime(&datetime);
let res =
convert_timestamp_millis_to_string(dt.timestamp_millis());
- let exp = format!("{}", dt.format("%Y-%m-%d %H:%M:%S %:z"));
+ let exp = format!("{}", dt.format("%Y-%m-%d %H:%M:%S%.3f %:z"));
assert_eq!(res, exp);
}
- check_datetime_conversion(1969, 9, 10, 1, 2, 3);
- check_datetime_conversion(2010, 1, 2, 13, 12, 54);
- check_datetime_conversion(2011, 1, 3, 8, 23, 1);
- check_datetime_conversion(2012, 4, 5, 11, 6, 32);
- check_datetime_conversion(2013, 5, 12, 16, 38, 0);
- check_datetime_conversion(2014, 11, 28, 21, 15, 12);
+ check_datetime_conversion(1969, 9, 10, 1, 2, 3, 4);
+ check_datetime_conversion(2010, 1, 2, 13, 12, 54, 42);
+ check_datetime_conversion(2011, 1, 3, 8, 23, 1, 27);
+ check_datetime_conversion(2012, 4, 5, 11, 6, 32, 0);
+ check_datetime_conversion(2013, 5, 12, 16, 38, 0, 15);
+ check_datetime_conversion(2014, 11, 28, 21, 15, 12, 59);
}
#[test]
fn test_convert_timestamp_micros_to_string() {
- fn check_datetime_conversion(y: u32, m: u32, d: u32, h: u32, mi: u32,
s: u32) {
+ fn check_datetime_conversion(y: u32, m: u32, d: u32, h: u32, mi: u32,
s: u32, micro: u32) {
let datetime = chrono::NaiveDate::from_ymd_opt(y as i32, m, d)
.unwrap()
- .and_hms_opt(h, mi, s)
+ .and_hms_micro_opt(h, mi, s, micro)
.unwrap();
let dt = Utc.from_utc_datetime(&datetime);
let res =
convert_timestamp_micros_to_string(dt.timestamp_micros());
- let exp = format!("{}", dt.format("%Y-%m-%d %H:%M:%S %:z"));
+ let exp = format!("{}", dt.format("%Y-%m-%d %H:%M:%S%.6f %:z"));
Review Comment:
it would be nice if we could change these tests to have hard coded values
rather than just programatically checking consistency -- with just these
changes, I don't have any sense of how different the output is after this
change.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
