apache / iceberg-rust

Apache Iceberg
https://rust.iceberg.apache.org/
Apache License 2.0
616 stars 138 forks source link

Bug: `fn day_timestamp_micro` produces wrong results #311

Closed marvinlanhenke closed 6 months ago

marvinlanhenke commented 6 months ago

Description:

fn day_timestamp_micro (source) seems to produce the wrong result which is off-by-one due to rounding errors. Also the implementation does not align with the Java implementation, which handles the conversion slightly different.

An example of the wrong output can be seen in the test: -115200000000 evaluates to 1969-12-30T16:00:00Z which should be days -2 (since epoch) and not -1.

Proposal:

Align Rust with Java implementation. I already have a prototype, due to working on #264, so I will implement the fix in a different PR - so it can be verified more easily.

marvinlanhenke commented 6 months ago

cc @liurenjie1024 Prototype for the fix can be seen in #309, however I still have to handle unwrap and change the return type to Result<i32>. This should be enough, however, to verify if the logic is correct.

liurenjie1024 commented 6 months ago

Hi, @marvinlanhenke Thanks for the debug, yes we should align with java/python implementation carefully. The fix looks good to me.

liurenjie1024 commented 6 months ago

Do you mind to submit a pr to fix it?

marvinlanhenke commented 6 months ago

@liurenjie1024 I'll try to get to submit a PR this evening. Still need to handle unwrap properly.

marvinlanhenke commented 6 months ago

@liurenjie1024 ...while looking at it - I think we need to handle Year, Month, and Hours as well? The Java implementation does the same thing for all granularities.

liurenjie1024 commented 6 months ago

@liurenjie1024 ...while looking at it - I think we need to handle Year, Month, and Hours as well? The Java implementation does the same thing for all granularities.

Yes, exactly.