Skip to content

Conversation

@ConeyLiu
Copy link
Contributor

Supports changing compression codec for ManifestWriter with respect to write.avro.compression-codec. This PR only updates the core part codes.

The benchmark results as following:

Benchmark                                    (codec)  Mode  Cnt  Score   Error  Units
ManifestReadBenchmark.readManifestFile  UNCOMPRESSED    ss    5  4.627 ± 1.371   s/op
ManifestReadBenchmark.readManifestFile        SNAPPY    ss    5  4.580 ± 1.686   s/op
ManifestReadBenchmark.readManifestFile          ZSTD    ss    5  5.296 ± 2.678   s/op
ManifestReadBenchmark.readManifestFile          GZIP    ss    5  6.745 ± 2.267   s/op

@ConeyLiu
Copy link
Contributor Author

cc @aokolnychyi

@nastra nastra self-requested a review August 10, 2023 16:19
@nastra
Copy link
Contributor

nastra commented Aug 10, 2023

Just wanted to mention that there's #6799 that tries to do the same

@ConeyLiu
Copy link
Contributor Author

Thanks @nastra, and I am sorry not to notice that.

@nastra
Copy link
Contributor

nastra commented Aug 11, 2023

@ConeyLiu you might want to get in touch with the author of #6799 to see whether he has cycles to complete #6799.
I think it would be good to have a combination of #6799 and this PR here. In particular what I like about this PR is that we just pass a configuration map to https://github.com/apache/iceberg/pull/8284/files#diff-4e8faf21494a38d340335de99357de31cdca2f3f82af495f832f67fc2446ad49R259 rather than 2 separate settings for compression. However, we should avoid breaking the API here in this PR and you might want to take a look at #6799 how it was achieved without API breakages

@ConeyLiu
Copy link
Contributor Author

@nastra Sure, let me ping in that PR.

However, we should avoid breaking the API here in this PR and you might want to take a look at #6799 how it was achieved without API breakages

Good advice. I will look into it.

@aokolnychyi
Copy link
Contributor

I will take a look at both PRs as well.

@github-actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 13, 2024
@github-actions
Copy link

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants