[V1, V2] Fix http compression #2645
Merged
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
…4 to reflect logic in headers builder
There was a problem hiding this comment.
Pull Request Overview
This PR fixes HTTP compression handling issues in both V1 and V2 clients, resolving conflicts between HTTP-level compression and ClickHouse's native LZ4 compression, and adds support for common compression algorithms (including ZSTD) to the V2 client.
Key Changes:
- Fixed V1 client HTTP compression by preventing
accept-encodingheader when using LZ4 algorithm - Added support for ZSTD, deflate, gzip compression methods in V2 client via new
CompressedEntityclass - Disabled automatic HTTP client content compression to allow manual control of compression handling
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java | Changed visibility of test helper fields and methods from private to protected for inheritance |
| client-v2/src/test/java/com/clickhouse/client/query/QueryServerHttpCompressionTests.java | Added test for HTTP-compressed queries with multiple compression algorithms |
| client-v2/src/test/java/com/clickhouse/client/insert/InsertTests.java | Changed visibility to protected and added assertions to verify inserted data |
| client-v2/src/test/java/com/clickhouse/client/insert/InsertClientHttpCompressionTests.java | Added test for HTTP-compressed inserts with multiple compression algorithms |
| client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java | Improved code clarity by extracting magic number to named variable |
| client-v2/src/main/java/com/clickhouse/client/api/internal/LZ4Entity.java | Made httpEntity field final for immutability |
| client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java | Refactored compression logic to support HTTP-level compression and renamed addHeader to setHeader |
| client-v2/src/main/java/com/clickhouse/client/api/internal/CompressedEntity.java | New class implementing HttpEntity wrapper for HTTP-level compression/decompression |
| client-v2/pom.xml | Added zstd-jni dependency for ZSTD compression support in tests |
| clickhouse-http-client/src/main/java/com/clickhouse/client/http/ClickHouseHttpConnection.java | Fixed V1 client to not set accept-encoding header when using LZ4 compression |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { "lz4" }, | ||
| { "zstd" }, | ||
| { "deflate" }, | ||
| { "gz" }, |
There was a problem hiding this comment.
The compression algorithm name should be "gzip", not "gz". In line 56 of QueryServerHttpCompressionTests.java, the correct value "gzip" is used, but here "gz" is used inconsistently.
Suggested change
| { "gz" }, | |
| { "gzip" }, |
| | ||
| public class CompressedEntity implements HttpEntity { | ||
| | ||
| private HttpEntity httpEntity; |
There was a problem hiding this comment.
The httpEntity field should be declared as final for consistency with the LZ4Entity class (line 18 of LZ4Entity.java) and to prevent accidental reassignment since it's set only in the constructor.
Suggested change
| private HttpEntity httpEntity; | |
| private final HttpEntity httpEntity; |
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Other comments (6)
- client-v2/src/test/java/com/clickhouse/client/insert/InsertClientHttpCompressionTests.java (53-53) Using a hardcoded index (1) to access the Id column is brittle. Consider using the column name consistently as done with the other columns:
assertEquals(records.get(i).getInteger("Id"), i); - client-v2/src/test/java/com/clickhouse/client/insert/InsertClientHttpCompressionTests.java (46-46) The `metrics` variable is declared but never used. Consider removing it to avoid unused variables.
- client-v2/src/main/java/com/clickhouse/client/api/internal/CompressedEntity.java (17-17) Consider making the `httpEntity` field final since it's never reassigned after initialization.
- client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java (109-109) The `DEFAULT_HTTP_COMPRESSION_ALGO` should be defined as a static constant rather than an instance variable since it's not changing per instance and is used in multiple methods.
- client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java (501-501) The method name was changed from `addHeader` to `setHeader`, but it's still called as `addHeaders` in line 433. Consider renaming this method to `setHeaders` for consistency.
- client-v2/src/main/java/com/clickhouse/client/api/internal/CompressedEntity.java (22-22) Consider adding a comment explaining why the constructor is package-private or providing a factory method if this class is meant to be instantiated by external code.
💡 To request another review, post a new comment with "/windsurf-review".
client-v2/src/main/java/com/clickhouse/client/api/internal/CompressedEntity.java Show resolved Hide resolved
Comment on lines 680 to 687
| private String getCompressionAlgoName(String contentEncoding) { | ||
| String algo = contentEncoding; | ||
| if (algo.equalsIgnoreCase("gzip")) { | ||
| algo = CompressorStreamFactory.GZIP; | ||
| } else if (algo.equalsIgnoreCase("lz4")) { | ||
| algo = CompressorStreamFactory.LZ4_FRAMED; | ||
| } | ||
| return algo; |
There was a problem hiding this comment.
The getCompressionAlgoName method should handle ZSTD compression since it's mentioned in the PR description as a supported algorithm. Consider adding a case for ZSTD similar to the existing ones for gzip and lz4.
Contributor Author
There was a problem hiding this comment.
this is not to enable handling but converting encoding type to compressor name
Comment on lines +87 to +88
| public String getContentEncoding() { | ||
| return httpEntity.getContentEncoding(); |
There was a problem hiding this comment.
The getContentEncoding() method returns the encoding from the wrapped entity, but this might not match the contentEncoding field used for compression/decompression. Consider returning the actual compression encoding used or documenting this behavior.
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
|
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
accept-encodingwhen LZ4 algorithm is set (default). What caused misalignment in logic - server was sending http compressed stream and client expected data compressed only and was using another LZ4 algorithm.User should have https://mvnrepository.com/artifact/com.github.luben/zstd-jni in dependencies to work with ZSTD. Library is not included in "All" bundle because there is only JNI implementation and LZ4 works for most cases.
Closes #2636
Checklist
Delete items not relevant to your PR: