comphead commented on code in PR #3323:
URL: https://github.com/apache/iceberg-rust/pull/3323#discussion_r4178487543
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -205,50 +205,13 @@ pub struct Day;
impl Day {
#[inline]
- fn day_timestamp_micro(v: i64) -> Result<i32> {
- let secs = v / MICROS_PER_SECOND;
-
- let (nanos, offset) = if v >= 0 {
- let nanos = (v.rem_euclid(MICROS_PER_SECOND) * 1_000) as u32;
- let offset = 0i64;
- (nanos, offset)
- } else {
- let v = v + 1;
- let nanos = (v.rem_euclid(MICROS_PER_SECOND) * 1_000) as u32;
- let offset = 1i64;
- (nanos, offset)
- };
-
- let delta = Duration::new(secs, nanos).ok_or_else(|| {
- invalid_data!("Failed to create 'TimeDelta' from seconds {secs}
and nanos {nanos}")
- })?;
-
- let days = (delta.num_days() - offset) as i32;
-
- Ok(days)
+ fn day_timestamp_micro(v: i64) -> i32 {
Review Comment:
Could you add a short comment here, and on `day_timestamp_nano`, saying the
Java difference is intended? Right now the only in-code trace is the test
comment that says what Java returns. Since the issue suggested flooring only
the seconds to match Java, a later reader who compares this with
`DateTimeUtil.convertMicros` may align it again and bring back the
non-monotonic transform.
Something like: "Floors to the calendar day. Java returns the previous day
for a pre-epoch timestamp exactly 999_999 µs after midnight. We keep the
calendar day so the transform stays monotonic, which inclusive projection
relies on."
If you file an upstream Java issue, linking it here would make the
divergence trackable. I did not find an existing one.
--
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]
