c-thiel commented on code in PR #331:
URL: https://github.com/apache/iceberg-rust/pull/331#discussion_r1693949164
##########
crates/iceberg/src/error.rs:
##########
@@ -333,6 +335,28 @@ define_from_err!(
define_from_err!(std::io::Error, ErrorKind::Unexpected, "IO Operation failed");
+/// Converts a timestamp in milliseconds to `DateTime<Utc>`, handling errors.
+///
+/// # Arguments
+///
+/// * `timestamp_ms` - The timestamp in milliseconds to convert.
+///
+/// # Returns
+///
+/// This function returns a `Result<DateTime<Utc>, Error>` which is `Ok` with
the `DateTime<Utc>` if the conversion is successful,
+/// or an `Err` with an appropriate error if the timestamp is ambiguous or
invalid.
+pub(crate) fn timestamp_ms_to_utc(timestamp_ms: i64) -> Result<DateTime<Utc>> {
+ match Utc.timestamp_millis_opt(timestamp_ms) {
+ chrono::LocalResult::Single(t) => Ok(t),
+ chrono::LocalResult::Ambiguous(_, _) => Err(Error::new(
+ ErrorKind::Unexpected,
Review Comment:
I think what @liurenjie1024 proposed above is most correct:
Ambiguous is unexpected - it would mean there is a fold in UTC time, which
sounds unexpected to me.
None on the other hand is returned for different things. Among them is a gap
in UTC time, which would be unexpected again, missing timezone data - also
unexpected, but most likely an overflow - definitely invalid data.
So I am changing None -> DataInvalid
https://github.com/c-thiel/iceberg-rust/commit/abcb4cd0b664333e594706768ba63ed19ca1d7f6
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
