upgraded lz4 version #2706
Merged
upgraded lz4 version #2706
Conversation
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
chernser
requested review from
Copilot and
mzitnik
and removed request for
mzitnik
There was a problem hiding this comment.
Pull request overview
This PR upgrades the LZ4 compression library from version 1.8.0 to 1.10.2, switching from the org.lz4 group to the at.yawk.lz4 fork. The upgrade addresses security concerns and JVM limitations in the latest version, which no longer includes the Unsafe factory instance. A test assertion was updated to accommodate both JavaSafe and JavaUnsafe implementations.
Key Changes:
- Updated LZ4 version from 1.8.0 to 1.10.2 with new Maven group ID
- Removed
lz4-pure-javadependency in favor of unifiedlz4-javaartifact - Modified test to handle both safe and unsafe LZ4 implementations
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Updated LZ4 version property and changed groupId in dependency management, removed lz4-pure-java dependency |
| client-v2/pom.xml | Changed LZ4 dependency groupId to at.yawk.lz4 |
| client-v2/src/test/java/com/clickhouse/client/ClientTests.java | Updated test assertion to accept both JavaSafe and JavaUnsafe implementations |
| clickhouse-r2dbc/pom.xml | Changed LZ4 dependency groupId to at.yawk.lz4 |
| clickhouse-jdbc/pom.xml | Changed LZ4 dependency groupId to at.yawk.lz4 |
| clickhouse-http-client/pom.xml | Removed lz4-pure-java dependency, added lz4-java as test dependency, updated shading configuration |
| clickhouse-data/pom.xml | Changed LZ4 dependency groupId to at.yawk.lz4 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
clickhouse-http-client/pom.xml
Outdated
| <dependency> | ||
| <groupId>at.yawk.lz4</groupId> | ||
| <artifactId>lz4-java</artifactId> | ||
| <scope>test</scope> |
There was a problem hiding this comment.
The lz4-java dependency was previously available at runtime (optional) but is now only available in test scope. If LZ4 compression is used in production code within this module, this change may cause runtime failures. Verify whether this dependency should have compile or runtime scope instead of test scope.
Suggested change
| <scope>test</scope> | |
| <optional>true</optional> |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
mzitnik
approved these changes
Jan 7, 2026
Contributor
mzitnik
left a comment
mzitnik
left a comment
There was a problem hiding this comment.
LGTM, just address cursor comments
Contributor
Author
|
Just for records - LZ4 still present in shaded file |
| </dependency> | ||
|
|
||
| <dependency> | ||
| <groupId>org.lz4</groupId> |
There was a problem hiding this comment.
Native-image configs reference potentially missing Unsafe LZ4 classes
Medium Severity
The library upgrade to at.yawk.lz4:lz4-java removes Unsafe factory instances per the PR description, and the test was updated accordingly to accept either JavaSafe or JavaUnsafe. However, the native-image reflection configs in clickhouse-jdbc and jdbc-v2 still reference LZ4JavaUnsafeCompressor, LZ4JavaUnsafeFastDecompressor, LZ4JavaUnsafeSafeDecompressor, and LZ4HCJavaUnsafeCompressor classes with their INSTANCE fields. If these classes or fields no longer exist in the new library, GraalVM native-image builds will fail when attempting to register these for reflection.
Additional Locations (1)
Contributor
Author
There was a problem hiding this comment.
Do not see any references in native image.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
at.yawk.lz4BUT package has not been changed so NO relocation rules should be changed.Closes #2697
Checklist
Delete items not relevant to your PR:
Note
Upgrade LZ4 and dependency coordinates
lz4.versionto1.10.2and replacesorg.lz4withat.yawk.lz4forlz4-javaacross modules (clickhouse-data,clickhouse-http-client,clickhouse-jdbc,clickhouse-r2dbc,client-v2, rootpom.xml).org.lz4:lz4-pure-javadependency and its shaded include; updates shade includes toat.yawk.lz4:lz4-javawhere applicable.Tests
client-v2testClientTests.testDisableNative()to acceptJavaSafeorJavaUnsafeintoString()due to changes in LZ4 factory availability.Written by Cursor Bugbot for commit bafb3c5. This will update automatically on new commits. Configure here.