Workflow [PR], commit [8d6cde8]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR enables framed snappy in the HTTP interface, adds the snappy_mode setting for generic snappy I/O, and threads that mode through the relevant read/write entrypoints. The framed reader/writer issues and most settings-propagation bugs from earlier review rounds are fixed, but one transport-level mismatch still remains in StorageURL: it can advertise HTTP Content-Encoding: snappy while still serializing the body in Hadoop-snappy format under the default snappy_mode = 'basic'.

Findings

❌ Blockers

• [src/Storages/StorageURL.cpp:704-724] [dismissed by author -- https://github.com/ClickHouse/ClickHouse/pull/100752#issuecomment-4850611569] StorageURLSink derives the HTTP header from compression_method (Content-Encoding: snappy) and then compresses the body with settings[Setting::snappy_mode]. In the default basic mode that produces Hadoop-snappy bytes under a framed-snappy HTTP header. This is still a real interoperability bug in the current tree because the receiving HTTP path hard-pins Content-Encoding: snappy to SnappyMode::Framed in src/Server/HTTPHandler.cpp:357-443, so INSERT INTO FUNCTION url('http://<clickhouse-http>/', ..., 'snappy') sends a body that another ClickHouse HTTP server deterministically rejects with a snappy framing decode exception. Suggested fix: separate HTTP transfer encoding from generic file-format snappy. When the URL engine emits Content-Encoding: snappy, force framed snappy on the write path (and honor header-based framed decoding on the HTTP response path), or reject snappy_mode = 'basic' for HTTP transfer encoding instead of emitting an invalid header/body pair.

Final Verdict

Needs changes. The core snappy implementation looks substantially better now, but the StorageURL HTTP transport still violates its advertised Content-Encoding: snappy contract in the default basic mode.

Continued work on this PR.

Addressed the unresolved review thread (src/IO/CompressionMethod.h, the read-path follow-up to the earlier write-path fix): snappy_mode = 'framed' was still unreachable on several user-selectable snappy input APIs because they wrapped the input with wrapReadBufferWithCompressionMethod without threading the setting, silently falling back to the SnappyMode::Basic (Hadoop block) default. Fixed in 380a49e5bf8 by threading the setting (Option A), consistent with the already-threaded file() / url() / object-storage readers:

• ClientBase::sendDataFromStdin — stdin input.
• GRPCServer — main input buffer and external-table read buffer.
• getReadBufferFromASTInsertQuery — new snappy_mode parameter; callers in getInputFormatFromASTInsertQuery and AsynchronousInsertQueue::pushQueryWithInlinedData pass the query setting.

Iceberg metadata (gzip/none per spec), Hive data files (Hadoop-block snappy by format), Parquet's raw-block SNAPPY codec (special-cased), and the Keeper/changelog readers (fixed internal codecs) are intentionally left on the default — none are user-selectable framed-snappy input.

Added 04401_snappy_stdin_compression.sh: a framed-snappy stream round-trips through the client stdin read path when snappy_mode = 'framed', and is rejected by the default basic reader. The four edited translation units compile cleanly.

The remaining red checks are unrelated master-wide flakes outside this PR's scope (compression/HTTP/storage): Stress test refresh scheduling RefreshTask.cpp:919 (#106737), Not-ready Set (Stress test (amd_tsan)), and Block structure mismatch in UnionStep/IntersectOrExceptStep driven by the AST fuzzer; plus Cannot start clickhouse-server. CH Inc sync reflects the standing shared-file Cloud divergence.

The public CI is green (170/170, AI Review: ✅ Approve). The only red required check is CH Inc sync (status tests failed, private sync PR ClickHouse/clickhouse-private#53712). I downloaded and analyzed the log of every failed private job — all six failures are unrelated to this PR's snappy / compression / HTTP / storage-I/O changes, and none of the snappy code paths or 040xx_snappy_* tests are implicated (they pass in the public report and are skipped, not failed, in the SharedCatalog private variants):

• Stress test (amd_tsan, SharedCatalog, meta in keeper) and Stress test (arm_asan_ubsan, SharedCatalog, meta in keeper) — Hung check failed, possible deadlock found, auto-classified by CI as flaky and matched to Hung check failed, possible deadlock found #107941 (zero sanitizer/race reports; the stuck thread is an unrelated AST-fuzzer SELECT in the query planner).
• Integration tests (amd_asan_ubsan, db disk, 5/7) and … old analyzer, 5/7 — test_refreshable_mv_no_multi_read::test_refreshable_mv_attach_without_multi_read failed a status assertion (assert "Disabled" in status, observed Scheduled/Running) — a refreshable-MV scheduling-state race, no snappy involvement.
• Stateless tests (amd_debug, distributed cache, meta in keeper, s3 storage) — 03010_shared_merge_tree_mutations_kill_mutation flaked on a transient Code: 341 … Failed to load all data parts (UNFINISHED) from StorageSharedMergeTree::updateMetadataAndLoadPartsWithConnection during SYSTEM SYNC REPLICA ("having stderror"); private SharedMergeTree replica-sync code.
• Stateless tests (amd_asan_ubsan, SharedCatalog, meta in keeper, s3 storage) — 03008_deduplication_insert_into_partitioned_table flaked on a transient Keeper Coordination::Exception: Connection loss (KEEPER_EXCEPTION) during the harness DROP DATABASE cleanup phase; the snappy tests were all skipped in this variant.

I re-ran the failed jobs of the sync run (https://github.com/ClickHouse/clickhouse-private/actions/runs/28395141680). The transient ones (hung-check, Keeper connection-loss, SharedMergeTree load) should clear on retry.

@groeneai, the test_refreshable_mv_attach_without_multi_read status-race failure is deterministic across both integration shards (failed all 3 retries) and is private-only, so a rerun may not clear it — could you confirm it is the known refreshable-MV flake (and not a regression) and track/fix it in a separate PR? The hung-check reds are already attributed to #107941.

Continued work on this PR — no code change required; it is now fully green and MERGEABLE / CLEAN.

CH Inc sync has cleared. The re-run of the private sync jobs (run 28395141680) has passed, so the six previously-red private flakes (hung-check #107941, test_refreshable_mv_attach_without_multi_read, the 03010/03008 Keeper/SharedMergeTree transients) have gone away on retry. Current state on head e33d24b7:

• Public CI: 170/170 (150 ✅ / 20 skipped, 0 failed)
• CH Inc sync: ✅ success
• Mergeable Check: ✅ success
• AI Review: ✅ Approve
• Cloud performance: no_change

One review thread is still open — the snappy_mode default question on src/Core/SettingsChangesHistory.cpp ("Can we switch the default to framed?"). I left the default at basic: the bot's reasoning holds — generic .snappy file/URL reads have always meant the Hadoop snappy block format, so flipping the default to framed would silently read existing Hadoop-snappy data with the framing decoder and break backward compatibility, unless gated by a compatibility/auto-detection rule. HTTP Content-Encoding: snappy already uses the framing format independently of this setting. This is a maintainer decision (resolve the thread as-is, or flip the default behind an explicit guard); no other feedback is outstanding.

Continued work on this PR — re-merged master to clear a conflict; no code change.

Since the last update the PR had gone CONFLICTING / DIRTY (master advanced ~688 commits in the interim). I merged origin/master into the branch (e33d24b7 → 7af449dad33); the merge was textually clean (no conflict markers — git's ort merge resolved everything that GitHub had flagged as conflicting), and the snappy_mode threading survived intact in every file master also touched (StorageURL.cpp, ReadBufferIterator.cpp, FormatFactory.cpp, SettingsChangesHistory.cpp, Settings.cpp, src/CMakeLists.txt).

To guard against semantic breakage from the large master advance, I -fsyntax-only-compiled every snappy-touched translation unit against the merged tree — the new buffers (SnappyFramedReadBuffer.cpp, SnappyWriteBuffer.cpp, HadoopSnappyWriteBuffer.cpp), the core wiring (CompressionMethod.cpp), and all integration points (HTTPHandler, PrometheusRequestHandler, GRPCServer, StaticRequestHandler, StorageFile, StorageObjectStorageSink, Iceberg/Utils, ClientBase, HTTPDictionarySource, getSourceFromASTInsertQuery, AsynchronousInsertQueue, Parquet/Reader, Parquet/Write, FormatFactory, ReadBufferIterator, StorageURL). All compile clean. The full CI matrix is now re-running on 7af449dad33.

GitHub state: MERGEABLE (no longer CONFLICTING).

The one remaining open thread is still the snappy_mode default question — left at basic for the backward-compatibility reason explained above; that is a maintainer decision.

Continued work on this PR — no code change required.

Since the master re-merge (7af449dad33), the public CI finished and is green except for two failures, both in the parallel-replicas planner and both unrelated to this PR's snappy / compression / HTTP / I/O changes (the diff touches no parallel-replicas or query-planner code). The AI Review verdict on this exact head is ✅ Approve.

1. AST fuzzer (amd_debug) — Logical error: 'local_replica_plan_reading_step->getAnalyzedResult() == nullptr' (STID: 2508-5243) in DB::QueryPlanOptimizations::considerEnablingParallelReplicas (src/Processors/QueryPlan/Optimizations/considerEnablingParallelReplicas.cpp:359). The fuzzed query is a plain aggregation (SELECT ClientIP, WatchID, COUNT(*) ... GROUP BY WatchID, ClientIP, log comment 04034_autopr_...) — no snappy involved. This assertion is a pre-existing, master-wide auto-parallel-replicas planner flake: CIDB (default.checks, 60d) shows 14 hits across 4 distinct, unrelated PRs — #71028 (Randomize parallel_replicas_min_number_of_rows_per_replica), #104513 (Speed up the analyzer / planner), #106073 (Support JOIN queries with AutoPR), and this one — spanning 2026-05-12 → 2026-07-01. No open tracking issue exists.

@groeneai, please investigate the local_replica_plan_reading_step->getAnalyzedResult() == nullptr assertion in considerEnablingParallelReplicas and provide a fix in a separate PR (or link one if already in progress). It is a latent logical error in the auto-parallel-replicas planner, unrelated to this PR.

2. Stateless tests (arm_binary, parallel) — 02875_parallel_replicas_cluster_all_replicas failed once with UNEXPECTED_PACKET_FROM_SERVER (expected TablesStatusResponse, got ProfileInfo), then passed all 181 reruns — the report itself classifies it as "not reproducible (likely a transient issue)". It is a widespread transient flake: CIDB shows 8 timeout/transient hits across 8 unrelated PRs over 2026-06-29 → 07-01.

I have re-run the two failed jobs (run 28477329368). The only remaining open item is the maintainer decision on the snappy_mode default (my unresolved thread on SettingsChangesHistory.cpp asks whether to switch it from basic to framed; I kept basic for backward compatibility with existing Hadoop-snappy data). CH Inc sync is currently running.

please investigate the local_replica_plan_reading_step->getAnalyzedResult() == nullptr assertion

Done. Root cause and fix in #109011 (separate PR, as requested).

Root cause: considerEnablingParallelReplicas() asserts the auto-PR local reading step has no index analysis before transplanting the single-replica one. canUseParallelReplicasOnInitiator() requires automatic_parallel_replicas_mode==0, so the initiator branch in PlannerJoinTree.cpp is skipped while building the single-node plan (mode=1) but runs while building the auto-PR plan (mode forced to 0). When parallel_replicas_min_number_of_rows_per_replica > 0, that branch calls selectRangesToRead() on the reading step to estimate the replica count, caching its analysis; the step is then moved into the auto-PR plan and reaches the assertion already analyzed.

The default of parallel_replicas_min_number_of_rows_per_replica is 0, which is why 04034_autopr_dataflow_cache_reuse_between_different_queries does not hit it on master; CI randomizes the setting (#71028), so it surfaces across unrelated PRs. Confirms it is unrelated to the snappy change here.

Fix: the pre-existing analysis is legitimate, so #109011 drops the over-strict assertion and keeps setAnalyzedResult(analysis), matching the min_rows == 0 path (both are selectRangesToRead() over the same table/query, so identical). Reproduced deterministically on a debug build and added a regression test that pins the setting. No open tracking issue existed.

Continued work on this PR — pushed one fix (24513f849c5).

The public CI on the previous head (7af449dad33) is green: the AST fuzzer (amd_debug) job (the parallel-replicas planner assertion, fixed separately by @groeneai in #109011) completed ✅ SUCCESS on retry, and all Stress test jobs are ✅. The only red required check is CH Inc sync, and its four failing private jobs are pre-existing, PR-unrelated Cloud flakes (details below).

However, the AI review on the internal sync PR (ClickHouse/clickhouse-private#53712) — which runs the db disk / meta in keeper variants the public matrix does not — flagged two behavioral concerns the public AI review did not surface. I verified both against the current code:

1. gRPC external-table snappy_mode ordering — real bug, fixed in 24513f849c5.
In GRPCServer.cpp, the external-table decompression buffer was wrapped with snappy_mode read from the outer query_context before external_table.settings() were applied to external_table_context. A per-table settings['snappy_mode']='framed' (with compression_type='snappy') was therefore ignored and the body decoded with the default basic (Hadoop-block) mode. The fix moves the wrapReadBufferWithCompressionMethod call after the per-table settings are applied and reads snappy_mode from the resulting settings — matching the code's evident intent (the same external_table_context/settings already build the input format). Compiles clean (ENABLE_GRPC=ON).

Ideal follow-up coverage would be a test_grpc_protocol case sending an external table with compression_type='snappy' + per-table settings={'snappy_mode':'framed'} and framed-snappy bytes (generatable via snappy.StreamCompressor — python-snappy is in the integration image — or via file(..., 'snappy') SETTINGS snappy_mode='framed'). I did not add it here because integration tests cannot be run/verified in this environment; happy to add it if you'd like.

2. StorageURL sink Content-Encoding: snappy vs snappy_mode — design decision for you, not changed.
StorageURLSink::initBuffers advertises Content-Encoding: snappy (StorageURL.cpp:704/712) but compresses the body with the session snappy_mode (:719-724, default basic = Hadoop block). The HTTP server decodes Content-Encoding: snappy as framed, independent of snappy_mode (HTTPHandler.cpp:357-359). So INSERT INTO FUNCTION url('http://<clickhouse-http>/', ..., 'snappy') under default settings sends a Hadoop-block body under a framed-advertised header, which the receiving server rejects.

I did not change this because it is a genuine design tension rather than a one-line fix: the url() read path also decompresses by URL extension using snappy_mode (StorageURL.cpp:961/1012), so forcing framed on the write side alone would break url() round-trips to a plain blob store (write framed / read Hadoop-block under default basic). A clean fix needs to distinguish the HTTP transfer-encoding (always framed for Content-Encoding: snappy) from file-format snappy (governed by snappy_mode). This is your call.

CH Inc sync failures — unrelated Cloud flakes (private run 28478238623, head e99cc03ab73):

• Integration tests (amd_asan_ubsan, db disk, [old analyzer] 1/7, 6/7) — all fail the same test_refreshable_mv_no_multi_read::test_refreshable_mv_attach_without_multi_read (assert "Disabled" in status, observed Scheduled), the refreshable-MV scheduling-state race you already classified as unrelated. No snappy involvement.
• Stress test (amd_debug, meta in keeper) — Code: 210 … Connection refused (localhost:9000) server-restart flake under THREAD_FUZZER_EXPLICIT_MEMORY_EXCEPTION_PROBABILITY injection; no sanitizer/logical-error report and no snappy/compression frames.

The one open review thread (snappy_mode default basic vs framed) remains your maintainer decision as before. Fresh CI is running on 24513f849c5.

The gRPC fix (24513f849c5) is correct: the wrap now reads snappy_mode from external_table_context after applySettingsChanges, the same settings object used to build the input format immediately below, so a per-table settings['snappy_mode']='framed' is honored. Ordering is right.

gRPC test: I can add the test_grpc_protocol framed-snappy external-table case (build with ENABLE_GRPC=ON and run integration tests locally here). Your call on timing: (a) I hand you the validated test to include in this PR, or (b) I open a follow-up PR once this merges (a standalone test cannot land earlier, since the fix lives only on this branch). Let me know which you prefer.

StorageURL sink Content-Encoding: snappy vs snappy_mode: confirmed the three sites. The sink advertises Content-Encoding: snappy (StorageURL.cpp:704) but compresses with session snappy_mode (:719); the HTTP request-body decoder pins snappy to framed (HTTPHandler.cpp:359, :434); the url() read decompresses by snappy_mode (:961, :1012). So the header and the body disagree whenever snappy_mode != framed.

The underlying issue is two layers sharing one knob: Content-Encoding is an HTTP transport contract (every HTTP peer, including CH's own server, reads snappy as framed), while snappy_mode governs a file-format payload. A resolution that keeps both HTTP round-trips and blob round-trips intact is to separate them: when the sink emits the Content-Encoding: snappy transport header it compresses framed (the only interpretation any HTTP peer applies), and the read side honors an HTTP Content-Encoding: snappy response as framed too; snappy_mode then governs only the pure file-format case (blob or file() by extension, with no HTTP content-encoding). That yields framed-both-ways over HTTP and snappy_mode-both-ways over blobs, so neither path breaks. This is your call; happy to implement whichever direction you choose.

Continued work on this PR — re-merged master to clear the CONFLICTING / DIRTY status; no code change.

master had advanced ~487 commits since the last merge (head 24513f849c5), so GitHub flagged the PR as CONFLICTING again. I merged origin/master (c794d5be5e1) into the branch and pushed the result as e02758c3a60. The merge was textually clean — git's ort resolved everything GitHub had flagged, with no conflict markers.

Verified the snappy code survived intact: the core wiring and new buffers (CompressionMethod.h/.cpp, HTTPHandler.cpp, GRPCServer.cpp, StorageURL.cpp, src/CMakeLists.txt, StorageObjectStorageSink.cpp) were untouched by master. The four shared files master did modify (StorageFile.cpp, Settings.cpp, SettingsChangesHistory.cpp, ClientBase.cpp) merged cleanly with the snappy_mode threading preserved — snappy_mode is still DECLAREd in Settings.cpp, and its SettingsChangesHistory entry coexists with master's new entries in the same current-version block. Fresh CI is running on e02758c3a60.

The only red on the previous head was the known-flaky test_keeper_session_refuse_stale_server (a ZooKeeper-container / iptables-nft infra flake, unrelated to snappy — it fails to even start the ZooKeeper container); it re-runs in the fresh matrix.

No review feedback addressed in code — the remaining items are maintainer decisions for you: the open snappy_mode default (basic vs framed) thread, and the StorageURL Content-Encoding: snappy vs snappy_mode transfer-encoding tradeoff. The gRPC framed-snappy regression test is with @groeneai pending your timing call.

Continued work on this PR — no code change required; head e02758c3a60 is in good shape.

The fresh CI I mentioned above has finished and the public PR report is green: 172 checks — 152 success, 0 failures, 20 skipped, with PR / Mergeable Check / Style check all ✅. The branch is MERGEABLE on a merge-base from earlier today (~5h old), so no re-merge is needed. CH Inc sync is still running (started 14:43Z).

One thing worth flagging since the re-merge: the AI Review verdict on this head now reads ⚠️ Needs changes (it was ✅ Approve on the previous head). I verified this introduces nothing new — its sole Major is the StorageURL Content-Encoding: snappy vs snappy_mode transfer-format item, which the bot itself annotates as [dismissed by author] and which you already analyzed above as a deliberate design tradeoff (url() reads decompress by snappy_mode, so forcing framed on the write side alone would break blob round-trips under the default basic). No new code-level findings were surfaced.

So the only open items remain the two maintainer decisions you flagged — the snappy_mode default (basic vs framed, your open thread on SettingsChangesHistory.cpp) and the StorageURL transfer-encoding direction — plus the pending gRPC framed-snappy regression test with @groeneai awaiting your timing call.

Continued work on this PR — re-merged master to clear the CONFLICTING / DIRTY status; no code change.

Since the last re-merge (e02758c3a60), master advanced ~121 merges and GitHub flagged the PR as CONFLICTING again. I merged origin/master (44938365f74) into the branch and pushed the result as 8d6cde8cdd3. The merge was textually clean — git's ort resolved everything GitHub had flagged, with no conflict markers.

Verified the snappy work survived intact across every file master also touched:

• SettingsChangesHistory.cpp — the snappy_mode entry sits in the current 26.7 block (single entry, no duplicate version key).
• Settings.cpp / Settings.h / SettingsEnums.* — snappy_mode declaration and the SnappyMode enum (basic/framed) intact.
• ClientBase.cpp — snappy_mode threading on the INTO OUTFILE, default output-compression, and sendDataFrom paths preserved.
• GRPCServer.cpp — the external-table snappy_mode ordering fix (24513f849c5) preserved (wrap after per-table settings are applied).
• HTTPHandler.cpp — HTTP Content-Encoding: snappy still decodes as framed, independent of snappy_mode.
• CompressionMethod.* and the new buffers (SnappyFramedReadBuffer, SnappyWriteBuffer, HadoopSnappyWriteBuffer) were untouched by master; crc32c still unconditionally linked into clickhouse_common_io.

All six 04xxx_snappy_* stateless tests have unique names (no renumbering needed). GitHub now reports the PR as MERGEABLE; fresh CI is running on 8d6cde8cdd3.

The two open items remain your maintainer calls, unchanged: the sole unresolved review thread (snappy_mode default basic vs framed in SettingsChangesHistory.cpp) and the AI-review StorageURL Content-Encoding: snappy vs snappy_mode transfer-format tension ([dismissed by author]).

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 384/456 (84.21%) · Uncovered code

Full report · Diff report