Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: fn day_timestamp_micro produces wrong results #311

Closed
marvinlanhenke opened this issue Mar 31, 2024 · 6 comments · Fixed by #312
Closed

Bug: fn day_timestamp_micro produces wrong results #311

marvinlanhenke opened this issue Mar 31, 2024 · 6 comments · Fixed by #312

Comments

@marvinlanhenke
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

liurenjie1024 commented Apr 1, 2024

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

@liurenjie1024
Copy link
Contributor

Do you mind to submit a pr to fix it?

@marvinlanhenke
Copy link
Contributor Author

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

@marvinlanhenke
Copy link
Contributor Author

@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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants