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

Chrono support #188

Merged
merged 11 commits into from
Jan 22, 2025
Merged

Chrono support #188

merged 11 commits into from
Jan 22, 2025

Conversation

dean-shaff
Copy link
Contributor

Summary

Add support for chrono.

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided so that we can include it in CHANGELOG later
  • For significant changes, documentation in README and https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

@CLAassistant
Copy link

CLAassistant commented Dec 11, 2024

CLA assistant check
All committers have signed the CLA.

serprex
serprex previously approved these changes Dec 11, 2024
serprex
serprex previously approved these changes Dec 12, 2024
@serprex
Copy link
Member

serprex commented Dec 12, 2024

https://docs.rs/chrono/latest/chrono/serde/ts_microseconds/index.html should give microsecond precision, fixing CI

Copy link
Collaborator

@loyd loyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

serprex
serprex previously approved these changes Jan 8, 2025
serprex
serprex previously approved these changes Jan 8, 2025
@slvrtrn
Copy link
Contributor

slvrtrn commented Jan 16, 2025

@dean-shaff, can you please sign the CLA? We won't be able to merge without it.

@StashOfCode
Copy link

When is this getting released?

@StashOfCode
Copy link

Hey, if you add a mod with DateTime::<Utc>::from_str, some types of datetime strings would become insertable into Clickhouse, e.g. 2025-01-19 16:36:15.157350+00:00.
See https://docs.rs/chrono/latest/src/chrono/datetime/mod.rs.html#1816-1818

pub mod dt_from_str {
    use super::*;

    // type DateTimeUtc = DateTime<Utc>;

    // option!(
    //     DateTimeUtc,
    //     "Ser/de `Option<DateTime<Utc>>` to/from `Nullable(DateTime)`."
    // );

    pub fn serialize<S>(dt: &DateTime<Utc>, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        let ts: i64 = dt.timestamp();

        u32::try_from(ts)
            .map_err(|_| S::Error::custom(format!("{dt} cannot be represented as DateTime")))?
            .serialize(serializer)
    }

    pub fn deserialize<'de, D>(deserializer: D) -> Result<DateTime<Utc>, D::Error>
    where
        D: Deserializer<'de>,
    {
        let ts: String = Deserialize::deserialize(deserializer)?;
        DateTime::<Utc>::from_str(&ts).map_err(|_| {
            D::Error::custom(format!("{ts} cannot be converted to DateTime<Utc>"))
        })
    }


#[derive(Serialize, Deserialize, Debug, Row)]
pub struct MyRow {
    pub id: String,
    #[serde(with = "dt_from_str")]
    pub requested_at: DateTime<Utc>,

@dean-shaff
Copy link
Contributor Author

@StashOfCode I think my intention with this PR was the replicate the existing time functionality; I'd be happy to add those string conversions in a new PR; what do you think?

@slvrtrn
Copy link
Contributor

slvrtrn commented Jan 20, 2025

@loyd

Merging is blocked
Merging can be performed automatically once the requested changes are addressed.

If everything is OK with this PR, let's merge and release it with the current pending changes as 0.14.0?

@loyd
Copy link
Collaborator

loyd commented Jan 22, 2025

LGTM, let's merge

@StashOfCode
Copy link

StashOfCode commented Jan 22, 2025

@dean-shaff, I found a crate called serde_with that seems to have already implemented what I did for myself. The documentation is pretty bad there though :/

@serprex serprex merged commit c8c65ef into ClickHouse:main Jan 22, 2025
6 checks passed
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 this pull request may close these issues.

6 participants