Chrono support #188
Chrono support #188
Conversation
|
Dean Shaff seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
https://docs.rs/chrono/latest/chrono/serde/ts_microseconds/index.html should give microsecond precision, fixing CI |
| @@ -69,17 +69,29 @@ watch = ["dep:sha-1", "dep:serde_json", "serde/derive"] | |||
| uuid = ["dep:uuid"] | |||
| time = ["dep:time"] | |||
| lz4 = ["dep:lz4_flex", "dep:cityhash-rs"] | |||
|
|
|||
| chrono = ["dep:chrono", "dep:serde_json"] | |||
There was a problem hiding this comment.
It shouldn't require the serde_json crate.
| // or if you prefer chrono: | ||
| #[derive(Row, Serialize, Deserialize)] | ||
| struct MyRow { | ||
| ts: u32, |
There was a problem hiding this comment.
ts and dt are shown in one struct MyRow, so let's show chrono also in that one row; it seems redundant to copy-paste the ts variant here.
Same for other examples
| ``` | ||
| </details> | ||
| * `Date32` maps to/from `i32` or a newtype around it and represents a number of days elapsed since `1970-01-01`. Also, [`time::Date`](https://docs.rs/time/latest/time/struct.Date.html) is supported by using `serde::time::date32`, that requires the `time` feature. | ||
| * `Date32` maps to/from `i32` or a newtype around it and represents a number of days elapsed since `1970-01-01`. Also, [`time::Date`](https://docs.rs/time/latest/time/struct.Date.html) is supported by using `serde::time::date32`, that requires the `time` feature. `chrono::NaiveDate` is supported by using `serde::chrono::date`, requiring the `chrono` feature. |
There was a problem hiding this comment.
Let's rephrase it using a list with all variants.
Also, to be consistent, we need to provide a link to these types (as for time-based types).
| @@ -138,6 +138,25 @@ pub struct Row { | |||
| pub datetime64_9: OffsetDateTime, | |||
| #[serde(with = "clickhouse::serde::time::datetime64::nanos")] | |||
| pub datetime64_9_tz: OffsetDateTime, | |||
|
|
|||
There was a problem hiding this comment.
What about adding the time_ prefix to the lines above?
| @@ -106,6 +106,355 @@ pub mod uuid { | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(feature = "chrono")] | |||
There was a problem hiding this comment.
This logic should be tested in the same way as the time feature (via integration tests with a real CH server without serde_json)
https://github.com/ClickHouse/clickhouse-rs/blob/main/tests/it/time.rs
This unit tests are useless with more reliable integration tests.
| S: Serializer, | ||
| { | ||
| let ts = dt.timestamp_nanos_opt().ok_or_else(|| { | ||
| S::Error::custom(format!("Can't create a timestamp from {dt}")) |
There was a problem hiding this comment.
These messages aren't consistent with time variants
Summary
Add support for chrono.
Checklist
Delete items not relevant to your PR: