Vendor CityHash as an internal part of the library#533
Merged
Conversation
ClickHouse's wire protocol checksums compressed blocks with a specific frozen CityHash128 variant. It is not interchangeable with upstream or system CityHash: a different implementation produces different hashes and the server rejects blocks with "Checksum doesn't match: corrupted data". Treating it as a swappable contrib/ dependency was therefore misleading and unsafe. Move it out of contrib/ and into the library proper: - Move contrib/cityhash/cityhash/ -> clickhouse/cityhash/. - Wrap the code in a clickhouse::cityhash namespace to prevent symbol clashes when consumers link their own CityHash; update call sites in compressed.cpp, lowcardinality.cpp and types.cpp accordingly. - Drop the WITH_SYSTEM_CITYHASH option, which could silently break wire compatibility, along with the Findcityhash machinery it required. - Compile CityHash directly into the clickhouse target (Bazel and CMake) instead of as a separate library.
slabko
changed the title
Contributor
Author
Contributor
|
LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Vendor CityHash as an internal part of the library
ClickHouse's wire protocol checksums compressed blocks with a specific
frozen CityHash128 variant. It is not interchangeable with upstream or
system CityHash: a different implementation produces different hashes and
the server rejects blocks with "Checksum doesn't match: corrupted data".
Treating it as a swappable contrib/ dependency was therefore misleading
and unsafe. Move it out of contrib/ and into the library proper:
clashes when consumers link their own CityHash; update call sites in
compressed.cpp, lowcardinality.cpp and types.cpp accordingly.
compatibility, along with the Findcityhash machinery it required.
instead of as a separate library.