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

C++ API support zstd codec #22

Merged
merged 2 commits into from
Jan 15, 2024
Merged

C++ API support zstd codec #22

merged 2 commits into from
Jan 15, 2024

Conversation

ucasfl
Copy link

@ucasfl ucasfl commented Jan 15, 2024

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@ucasfl ucasfl marked this pull request as draft January 15, 2024 08:20
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


leefeng 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.

@ucasfl ucasfl marked this pull request as ready for review January 15, 2024 10:28
@ucasfl
Copy link
Author

ucasfl commented Jan 15, 2024

@Avogar Please take a look at this.

@Avogar
Copy link
Member

Avogar commented Jan 15, 2024

I don't see this code in upstream in https://github.com/apache/avro. Why not to make a PR to the upstream first so the maintainers can review it properly and verify it's 100% correct? I don't know this code well enough.

@ucasfl
Copy link
Author

ucasfl commented Jan 15, 2024

I don't make PR to upstream because the upstream rely on old boost version, which does not include zstd support, so will need to upgrade boost and introduce zstd library, I don't think it's easy to be accepted. But in ClickHouse, both of the newest boost library and zstd library are self contained.

The logic of zstd is almost the same as deflate(zlib).

@Avogar
Copy link
Member

Avogar commented Jan 15, 2024

Ok, I understood. Let me discuss it with the team

@alexey-milovidov alexey-milovidov self-assigned this Jan 15, 2024
@alexey-milovidov alexey-milovidov merged commit d43acc8 into ClickHouse:master Jan 15, 2024
@ucasfl ucasfl deleted the zstd-encode branch January 16, 2024 02:46
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