klion26 commented on code in PR #8266:
URL: https://github.com/apache/arrow-rs/pull/8266#discussion_r2317961986
##########
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:
The input is `i64`, so it will never attach a time zone, and the output will
always be' +00:00'. Do we need to add "%:z" in the output?
From
[`Utc.timestamp_millis_opt`](https://docs.rs/chrono/latest/chrono/trait.TimeZone.html#method.timestamp_millis_opt),
it seems that it will always return a UTC timezone value, so maybe we need to
change the comment for this function(the comment said the result will be in
local timezone).
##########
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:
Another question not related to the current PR: this function
`convert_timestamp_millis_to_string` will be used in `Field`, and `Field`
contains an `i64` for `TimestampMillis/TimestampMicros`(the
milliseconds/microseconds from unix epoch), does output the raw timestamp(the
`i64` value) here is enough? and translate the timestamp to `dt.format` value
when displaying something like `LogicalType::Timestamp`
--
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]
