laskoviymishka commented on code in PR #2892:
URL: https://github.com/apache/iceberg-rust/pull/2892#discussion_r4087428884
##########
crates/iceberg/src/spec/values/temporal.rs:
##########
@@ -98,15 +98,12 @@ pub(crate) mod timestamptz {
}
pub(crate) fn microseconds_to_datetimetz(micros: i64) -> DateTime<Utc> {
- let (secs, rem) = (micros / 1_000_000, micros % 1_000_000);
-
- DateTime::from_timestamp(secs, rem as u32 * 1_000).unwrap()
+ // This shouldn't fail until the year 262000
+ DateTime::from_timestamp_micros(micros).unwrap()
Review Comment:
The comment names only the positive ceiling, but `from_timestamp_micros`
also returns `None` below ~year -262144, so the bare `.unwrap()` still panics
on a sufficiently ancient i64 — and the spec puts no bound on the micros value.
Not reachable for realistic data, and it's the same pattern the `timestamp`
sibling already carries, so not a blocker.
Since `from_timestamp_nanos` right below is genuinely infallible, though,
I'd make the asymmetry honest with `.expect(...)` naming both bounds, so nobody
reads the comment as "safe for all inputs":
```suggestion
// Returns None only outside chrono's range (~year -262144 to
+262142);
// unreachable for any realistic Iceberg timestamp.
DateTime::from_timestamp_micros(micros)
.expect("timestamp micros outside chrono's representable range
(~year -262144..+262142)")
```
##########
crates/iceberg/src/spec/values/tests.rs:
##########
@@ -1065,6 +1065,29 @@ fn test_parse_timestamptz() {
);
}
+#[test]
+fn test_pre_epoch_timestamptz_formatting() {
+ assert_eq!(
+ Datum::timestamptz_micros(-1).to_string(),
Review Comment:
`-1` only exercises the `secs = 0`, sub-second path. The case that actually
broke on the old code is `secs ≠ 0` with a non-zero remainder — e.g.
`-1_500_000` µs, where `rem as u32` overflowed — so I'd add a mid-second
assertion (`Datum::timestamptz_micros(-1_500_000).to_string()` → `"1969-12-31
23:59:58.500000 UTC"`) plus its JSON round-trip to guard the path that was
wrong.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
