Skip to content

fix: honor CompressionZSTD for native block compression#1840

Open
must108 wants to merge 7 commits into
ClickHouse:mainfrom
must108:must108/compressionzstd
Open

fix: honor CompressionZSTD for native block compression#1840
must108 wants to merge 7 commits into
ClickHouse:mainfrom
must108:must108/compressionzstd

Conversation

@must108

@must108 must108 commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR fixes HTTP native block compression settings so CompressionZSTD and CompressionLZ4 no longer behave the same.

Fixes #1772

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added

Signed-off-by: Mustaeen Ahmed <136145181+must108@users.noreply.github.com>
@must108

must108 commented May 10, 2026

Copy link
Copy Markdown
Contributor Author

@chernser @kavirajk bumping, would love for you guys to take a look!

@must108

must108 commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

@chernser @kavirajk bumping, would love for you guys to take a look!

@github-actions

Copy link
Copy Markdown

Summary

This PR fixes a real bug: previously, both CompressionZSTD and CompressionLZ4 for HTTP native block compression only set compress=1/decompress=1 without telling the server which codec to use, so the server effectively defaulted to LZ4 regardless of the client's selection. The fix factors a helper applyHTTPNativeCompressionSettings that additionally sets network_compression_method and (for ZSTD) network_zstd_compression_level, and routes both the query and batch paths through it. The approach is sound and the refactor avoids drift between the two call sites.

Should fix

  • clickhouse_options.go:73 — the doc comment on Compression.Level says "this only applies to lz4, lz4hc, zlib, and brotli compression algorithms". With this PR, ZSTD now also honors Level (via network_zstd_compression_level). Update the comment so users discover the new behavior.
  • The new test only exercises the read path (SELECT getSetting(...)) through httpConnect.query. The fix in httpBatch.Send() shares the helper but is never directly executed by the test — a regression that broke only the batch branch would not be caught. Consider extending the table to also do a PrepareBatch / Append / Send round-trip with CompressionZSTD and verify the server accepts the ZSTD-compressed request body.
  • network_compression_method is now set via query.Set in createRequest (conn_http.go:645), which overrides any user-provided value in Options.Settings. A caller who deliberately set network_compression_method in their own settings will now have it silently overridden. Either honor a user-supplied value or document this precedence in the README block you already updated.

Nits

  • nit: applyHTTPNativeCompressionSettings's early-return guard if method != CompressionLZ4 && method != CompressionZSTD duplicates the switch filter both callers already do. Not harmful, but redundant — happy either way.
  • nit: conn_http_batch.go:243 — the b.conn.opt != nil check is unreachable; dialHttp requires a non-nil opt. Only the opt.Compression != nil check is meaningful.
  • nit: a one-line doc comment on applyHTTPNativeCompressionSettings ("sets the ClickHouse HTTP query parameters for native block compression") would help future readers grasp intent without tracing call sites.
  • nit: CompressionLZ4HC is not handled here (consistent with prior behavior), but worth a quick thought — should a user who selected LZ4HC have network_compression_method=LZ4 set on their behalf? Probably out of scope for this PR; flagging in case it's worth a follow-up.

Verdict

Approve once the stale Compression.Level doc comment is updated. The other items are non-blocking improvements.

@must108

must108 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@mshustov good to check again!

@github-actions

Copy link
Copy Markdown

Review

Summary

This PR fixes #1772, where the HTTP native block compression path set compress=1/decompress=1 but never told the server which codec to use — so CompressionZSTD silently behaved like CompressionLZ4. The fix introduces applyHTTPNativeCompressionSettings, which additionally sets network_compression_method (and network_zstd_compression_level for ZSTD when a level is configured) on both the query and batch paths. The approach is correct: settings are serialized to HTTP query params via fmt.Sprint in conn_http.go, so the int level value is encoded fine, and the helper preserves the prior compress/decompress semantics for each direction (request body compressed only on the batch path). README and the Compression.Level doc comment are updated accordingly. Core logic looks good.

Should fix

  • Batch path is untested. The new test exercises only the query path: QueryRow("SELECT getSetting('network_compression_method')") flows through conn_http_query.go. The matching change in conn_http_batch.go (Send) has no coverage. Consider a round-trip insert under CompressionZSTD (or asserting the codec server-side after a batch) so a regression in the batch branch is caught.
  • std/database/sql (OpenDB) path not covered. Per the review checklist, coverage is expected for both the native and std APIs. The fix lives below both surfaces in conn_http.go, so risk is low, but a std-path assertion would close the gap the checklist calls out.

Nits

  • nit: The level-extraction block
    compressionLevel := 0
    if h.opt != nil && h.opt.Compression != nil {
        compressionLevel = h.opt.Compression.Level
    }
    is duplicated verbatim in conn_http_query.go and conn_http_batch.go. Passing *Compression (or *Options) into the helper would remove the repetition and the redundant nil guards (opt is always set in dialHttp, and Compression is non-nil whenever compression is LZ4/ZSTD).
  • nit: settings["network_zstd_compression_level"] = level stores an int while every other setting in the map is a string. It works (serialized via fmt.Sprint), but strconv.Itoa(level) would keep the map value types consistent.

Verdict

Approve — the fix is correct and has a test for the query path. Adding batch-path (and ideally std-path) coverage before merge would be good but is non-blocking.

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.

Setting CompressionZSTD has no effect

2 participants