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

feat: expose compression-level configuration for general compression #3034

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

niyue
Copy link
Contributor

@niyue niyue commented Oct 22, 2024

This PR aims to address #3033, and expose the compression level for general compression.

@github-actions github-actions bot added the enhancement New feature or request label Oct 22, 2024
@niyue
Copy link
Contributor Author

niyue commented Oct 22, 2024

I check zstd/lz4/snappy [1][2][3], and all of them have the concept of compression level, and all of them use an integer to express such compression level but they use different integers as their default values (zstd uses 0 which defaults to level 3, lz4 uses 1 as default level, and snappy uses 1 as default level)

[1] zstd compression level, http://facebook.github.io/zstd/zstd_manual.html#Chapter1
[2] lz4 compression level, https://github.com/lz4/lz4/blob/e3974e5a1476190afdd8b44e67106cfb7097a1d5/doc/lz4_manual.html#L144
[3] snappy compression level, https://github.com/google/snappy/blob/32ded457c0b1fe78ceb8397632c416568d6714a0/snappy.h#L54-L72

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 85.89744% with 22 lines in your changes missing coverage. Please review.

Project coverage is 78.38%. Comparing base (f17d88d) to head (8687239).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...-encoding/src/encodings/physical/block_compress.rs 71.21% 17 Missing and 2 partials ⚠️
rust/lance-encoding/src/encoder.rs 95.23% 1 Missing and 1 partial ⚠️
...ust/lance-encoding/src/encodings/physical/value.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3034      +/-   ##
==========================================
+ Coverage   78.24%   78.38%   +0.14%     
==========================================
  Files         240      240              
  Lines       77284    78475    +1191     
  Branches    77284    78475    +1191     
==========================================
+ Hits        60470    61515    +1045     
- Misses      13696    13845     +149     
+ Partials     3118     3115       -3     
Flag Coverage Δ
unittests 78.38% <85.89%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

@wjones127
Copy link
Contributor

@niyue Could you fix the clippy lints from the Java CI? These do originate from your rust code.

@niyue niyue force-pushed the feature/block-compress-level branch from 275177d to 2a43118 Compare October 23, 2024 02:27
@niyue
Copy link
Contributor Author

niyue commented Oct 23, 2024

Sure. I updated the code to fix the clippy lint issue and two more unit tests. Please check them out. Thanks.

@broccoliSpicy
Copy link
Contributor

some lint errors, should be fixable with cargo fmt --all

@niyue niyue force-pushed the feature/block-compress-level branch from 2a43118 to 0346801 Compare October 25, 2024 02:04
@niyue
Copy link
Contributor Author

niyue commented Oct 25, 2024

some lint errors, should be fixable with cargo fmt --all

Thanks, fixed.

@broccoliSpicy broccoliSpicy merged commit 8339b7a into lancedb:main Oct 25, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants