search

ClickHouse / clickhouse-rs

Official pure Rust typed client for ClickHouse DB
https://clickhouse.com
Apache License 2.0
283 stars 84 forks source link

Replace `clickhouse_rs_cityhash_sys` with pure Rust crate `cityhash-rs` #107

Closed blind-oracle closed 1 month ago

blind-oracle commented 3 months ago

clickhouse_rs_cityhash_sys is a C++ binding and thus requires a C++ compiler to build which sometimes is a problem.

blind-oracle commented 3 months ago

If the PRs are accepted - I can make it.

loyd commented 2 months ago

Yes, my target is totally avoid native deps (rustls (in progress), lz4 (requires only C compiler, can be done easily without big performance difference), cityhash).

The last one (cityhash) is the most problematic because I don't see any solid crates. I can use cityhash-rs with exactly specified version (to avoid possible attacks in the future), but it seems to be unsupported. Another way is to copy implementation in-place.

I'll accept a PR

blind-oracle commented 2 months ago

@loyd

Thanks! Currently I have a working fork using cityhash-rs with version 1.0.2, will try to make a PR. The only weird thing is that the hash that this crate produces has high/low 64 bits swapped in contrast to what we get from the C version, so I had to swap them back.

Changes are minor: https://github.com/loyd/clickhouse.rs/commit/beba19192da6c50884a0b8f5797fda53185d8844

loyd commented 1 month ago

Let's include a modified cityhash in the repository, giving us both more control and security.

loyd commented 1 month ago

Thanks for your commit, I used it in related PR

blind-oracle commented 1 month ago

Thanks! Sorry i've been on holidays to go through it myself