-
Notifications
You must be signed in to change notification settings - Fork 135
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
Replace i64 with DateTime #94
Conversation
6db9eaf
to
6cc3ff1
Compare
Moving back to draft while i address comments. Will ping when it's ready again, shouldn't take too long! |
06ccad7
to
9521da8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this pr. I've left some comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
cc @Fokko PTAL |
Looks like we have a sad clippy:
|
The failure is caused by upgrades of cc @fqaiser94 |
7f45f05
to
a57de25
Compare
cc @Fokko Any other comments? |
Thanks @fqaiser94 for the PR, and @liurenjie1024 & @Xuanwo for the review 🙌 |
Fixes: #90
First time contributor to this repo.
Extremely new to rust (not new to iceberg, used it extensively and contributed a couple of small things there).
How is this for an approach?I usedchrono::DateTime<Utc>
but had to wrap it in a NewType to implement theSerialize
andDeserialize
traits (as otherwise we run into Rust's orphan rule issue).