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

Add Klickhouse client integration #264

Closed
wants to merge 1 commit into from
Closed

Conversation

Protryon
Copy link

@Protryon Protryon commented Dec 19, 2022

Adds klickhouse crate integration for the clickhouse database. Notably klickhouse already has this internally, but it seems appropriate to move it here. After this lands in a release, I will remove the klickhouse implementation and push a minor update to avoid a feature-driven cyclic dependency.

Implementation notes:

  • Didn't add barrel compatibility -- Clickhouses model differs too greatly from standard SQL for this to work cleanly. Could be done with with upstream changes to barrel though, but I am not inclined. (I only write raw SQL migrations)
  • Clickhouse has no notion of transactions, so this implementation simply ignore them. Once clickhouse implements them, this could be readded.

@@ -13,13 +13,14 @@ categories = ["database"]
edition = "2018"

[features]
default = []
default = ["clickhouse"]
Copy link

Choose a reason for hiding this comment

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

Why is it default?

@jxs
Copy link
Member

jxs commented Feb 6, 2023

Hi @Protryon, thanks for this PR and I am sorry for only replying now, I missed its notification, will look into it in the next days!

Meanwhile I know that in September I said the opposite, but without the time I had then and thinking more about it regarding organization churn I think it's probably better for independent drivers to live outside of refinery. Just a couple of reasons:

  • faster iterations as you don't require me to review
  • faster CI time on both crates

I am willing to create a list with them on the README.md what do you think about it?

@Protryon
Copy link
Author

@jxs Fine by me.

@Protryon Protryon closed this Feb 21, 2023
@jxs
Copy link
Member

jxs commented Dec 4, 2023

Hi @Protryon did you end up publishing this as an external crate?

@jxs jxs mentioned this pull request Dec 4, 2023
@Protryon
Copy link
Author

Yeah, it's embedded as a feature flag in Klickhouse crate. @jxs

@cameronbraid
Copy link

Since Klickhouse crate supports refinery - how can I use it with the refinery_cli ?

Not sure if this should be a question for Klickhouse or refinery project, I would think the refinery since it would need to support plugins from external crates for this to work

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.

4 participants