-
Notifications
You must be signed in to change notification settings - Fork 119
add support for time and time64 #252
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
base: main
Are you sure you want to change the base?
add support for time and time64 #252
Conversation
PR review rate limit exceeded |
can remove empty |
Sure, was testing if things are working fine with my local clickhouse server, will clean it up. |
6f4acfa
to
6d3751f
Compare
/windsurf-review |
PR review rate limit exceeded |
working on CI failures... |
30b2b2b
to
e6ce37c
Compare
/// Optional timezone | ||
Time(Option<String>), | ||
/// Precision and optional timezone | ||
Time64(DateTimePrecision, Option<String>), |
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.
But it does not have a timezone?
@@ -160,3 +176,22 @@ fn it_deserializes() { | |||
assert_eq!(actual, sample()); | |||
} | |||
} | |||
|
|||
#[test] |
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.
I'd also add a separate time/64 it for a round-trip, including corner case values
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.
Makes sense, WIP
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.
Done!
Clippy is fixed after #254 |
Thanks for the review feedbacks @slvrtrn I shall update once I'm done with the changes. |
e6ce37c
to
9008120
Compare
6f58231
to
b5d2fc3
Compare
How can I trigger the CI? |
tested on local, CI should pass now. |
fixes #242
related to ClickHouse/ClickHouse#81217
Summary
A short description of the changes with a link to an open issue.
Checklist
Delete items not relevant to your PR: