Workflow [PR], commit [399aef3]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR adds the new experimental PCO codec, its encoder/decoder, docs, spec updates, and a substantial set of malformed-input and validation fixes. Most of the earlier correctness issues are addressed, but one rollout contract is still broken: registering PCO in the global compressed-block registry makes it usable on external framed input without allow_experimental_codecs, so the experimental gate is not complete.

Findings

❌ Blockers

• [src/Compression/CompressionCodecPco.cpp:358] Registering PCO globally here makes CompressedReadBufferBase accept method byte 0x9d on external framed input (HTTPHandler decompress=1, compressed TCP queries) without consulting allow_experimental_codecs. Because the wrapper is self-describing and W = 1 / U8 can represent arbitrary bytes, clients can already use the new experimental codec with the gate off. Suggested fix: when external_data is true, reject codec->isExperimental() in the shared CompressedReadBuffer path (or otherwise keep 0x9d out of externally accepted framed codecs).

Final Verdict

Status: ❌ Block

Minimum required actions:

1. Reject experimental codecs, including PCO, from externally supplied compressed frames unless the corresponding gate is enabled, or narrow 0x9d registration so it is only usable on the intended on-disk/per-column paths.

Addressed the AI Review Request changes verdict (and the corresponding clickhouse-gh inline thread on CompressionCodecPco.cpp), whose required action was to make PCO decompression fail closed on invalid block headers and add focused coverage.

325b5041778 — doDecompressData now validates the stored header instead of silently recomputing/ignoring it:

• W must be one of 1/2/4/8;
• the stored leading-byte count B must equal uncompressed_size mod W (which also enforces B < W);
• the early return for the partial-only case (uncompressed_size < W) is removed, so the trailing standalone .pco stream is always decoded and required to produce exactly the expected bytes — a body such as [W=8, B=0, four raw bytes, no standalone stream] is now rejected.

Valid streams are unaffected: the encoder always writes a supported W, B = uncompressed_size mod W, and a standalone stream (header-only for the n == 0 case, which decodeStandalone decodes to 0 bytes). Added gtest_pcodec_malformed_block covering an unsupported W, a mismatched B, and a partial-only body with its standalone stream stripped off. Verified CompressionCodecPco.cpp and the new test compile clean (-fsyntax-only -Werror); the empty-stream / stripped-stream behavior was confirmed against the standalone encoder/decoder sources.

Also merged latest master (was 258 behind) — textually clean, the only PR-touched file master also changed is MergeTreeSettings.cpp, with no interaction; the net PCO diff is preserved. No test renumbering needed (test_numbers_check is gap-based and 04306/04337 are dense within master's range; the PCO filenames are unique).

All CI reds on the previous commit are unrelated flakes:

• Stateless Segmentation fault / Server died / 03714_queries_escaping_3 — a master-wide flaky read-path crash in MergeTreeRangeReader::startReadingChain → getTotalBytesInColumns, reproducing on master itself (pull_request_number = 0) and on 6 different PRs on 2026-06-29 alone (108786, 107567, 108673, 104816, 108565, 106222); no compression code in the stack. Tracked as "Server died" Server died #107487.
• Stress (amd_tsan, arm_tsan) Hung check failed — the chronic master-wide hung-check flake Hung check failed, possible deadlock found #107941.
• Stress (arm_debug) Bad cast … ColumnNullable to ColumnVector<unsigned long> in AggregateFunctionSum — a recurring AST-fuzzer flake seen across 7 separate days on different PRs (06-05/17/18/20/22/24/29), unrelated to compression.

The higher-level design questions in @rienath's review (whether the niche ratio win justifies a new codec, and the C++-port maintenance burden vs. linking the Rust crate) are left for the author's decision — they are not blocking and not actionable here.

Pushed e73c49f0970 addressing the AI Review Request changes verdict (and the corresponding clickhouse-gh inline thread on CompressionFactoryAdditions.cpp): a codec-only ALTER TABLE ... MODIFY COLUMN x CODEC(PCO) was rejected as an untyped codec.

AlterCommands::validate passed command.data_type (null when the type is not restated) straight into validateCodecAndGetPreprocessedAST, so the validation took the untyped-codec branch and threw "Codec PCO requires the column type to compress and can only be specified for a column" — even though AlterCommand::apply falls back to the existing column.type. The fix mirrors that apply-time fallback in the validate path (command.data_type ? command.data_type : all_columns.get(column_name).type). The experimental gate and the fixed-width-numeric requirement are still enforced (both BAD_ARGUMENTS).

New test 04489_pco_codec_alter_modify_column covers the codec-only ALTER, the data round-trip after OPTIMIZE ... FINAL re-encodes parts, the CODEC(Delta, PCO) chain, the experimental gate, and the unsupported-type rejection. Built clean (ARM) and verified 04489, 04306_pco_codec, 04337_pco_codec_untyped_settings, 01061_alter_codec_with_type, and 00804_test_alter_compression_codecs all pass via clickhouse-test.

The sole remaining CI red — Stress test (arm_tsan), Hung check failed, possible deadlock found — is the chronic master-wide hung-check flake #107941 (no compression or ALTER code in any stack frame); the CI summary itself links the same issue.

No master merge: behind 195 but the merge-base is from 2026-06-29 23:39Z (~17h ago) and the only red is the unrelated flake above.

Pushed 8a758cf3df8 addressing the AI Review Request changes verdict (and the corresponding clickhouse-gh inline thread): estimateCompressionRatio('<codec>')(column) could instantiate an experimental codec such as PCO through the typed CompressionCodecFactory::get path without checking allow_experimental_codecs, letting it be exercised to compress real data with the gate off.

Fix: resolve allow_experimental_codecs from the query settings at aggregate creation time and enforce it in getCodecOrDefault, right where the codec is constructed (and immediately before it is used to compress). CompressionCodecMultiple::isExperimental already surfaces an inner experimental codec, so chains wrapping PCO (PCO, ZSTD, Delta, PCO) are rejected too; the check fails closed if the settings are unavailable. The gate is enforced at construction, not at parse time, so the existing behavior of estimateCompressionRatio on an empty input — it returns 0 without resolving the codec — is preserved (03363_estimate_compression_ratio_validation depends on that).

New test 04490_estimate_compression_ratio_pco_experimental_gate: allow_experimental_codecs = 0 rejects PCO and chains wrapping it with BAD_ARGUMENTS while non-experimental codecs and the default codec keep working; allow_experimental_codecs = 1 allows PCO on supported numeric types. Built clean (ARM, no warnings) and verified 04490, 04306_pco_codec, 04337_pco_codec_untyped_settings, 04489_pco_codec_alter_modify_column, 03363_estimate_compression_ratio_validation, 03363_estimate_compression_bug91415, 03364_estimate_compression_ratio, 04053, 04267, 01061_alter_codec_with_type, and 00804_test_alter_compression_codecs all pass via clickhouse-test.

The two remaining CI reds — Stress test (amd_tsan) and Stress test (arm_tsan), both Hung check failed, possible deadlock found — are the chronic master-wide hung-check flake #107941 (the CI summary links the same issue); the stack frames are generic AST-fuzzer query analysis with no compression code, and this PR touches nothing under that path.

No master merge: the PR is MERGEABLE with no conflicts, the merge-base is from 2026-06-29 (~21h ago, well under a week), and the only reds are the unrelated flake above (which fails on master too).

Addressed the AI Review Block verdict on 8a758cf3 (its three minimum required actions), pushed fb4b8d5817c, 41c1e605c1f, c9272448255.

• fb4b8d5817c — IntMult sub-word reconstruction UB. The join primary * base + secondary was evaluated in signed int for sub-32-bit latents (U16/I16, L = uint16_t), so a malformed stream with both factors large (65535 * 65535) overflowed int before the cast back to L. A well-formed IntMult stream can never reach this (primary * base + secondary equals the in-range value), so only malformed input hits it — the same integer-promotion hazard already handled in decodeConv1. The join now runs in an unsigned accumulator at least 32 bits wide, then casts back to L: valid streams are byte-identical, malformed input wraps modularly instead of invoking UB. This also makes the "well-defined unsigned wrapping" premise from the still-open StandaloneDecoder.h range-validation thread actually hold; the semantic per-element range validation there remains deferred as noted (that thread is about rejecting fabricated-but-defined values, which is a separate decision).

• 41c1e605c1f — trailing bytes after the standalone stream. decodeStandalone now verifies the reader consumed exactly src_len after the termination byte, so [W][B][raw][valid .pco][garbage] fails closed instead of being silently accepted by the wrapper's byte-count check alone.

• c9272448255 — server-wide <compression> selector gate. CompressionCodecSelector now rejects a configured codec that requiresColumnTypeToCompress or isExperimental at config load, mirroring the untyped default_compression_codec / marks_compression_codec / primary_key_compression_codec validation, so <compression><case><method>pco</method> (and alp) can no longer bypass the gate and be re-resolved per-column by the part writer. Ordinary codecs (lz4, zstd) are unaffected.

Focused regressions added: IntMultSubWordReconstructionNoOverflow (decodes a hand-built malformed IntMult stream — a sanitizer build trapped on the previous code), RejectsTrailingBytesAfterStandaloneStream, and gtest_compression_codec_selector.cpp (type-dependent, experimental, and ordinary cases). Verified out of the ClickHouse build with a standalone PCODEC_STANDALONE harness under -fsanitize=undefined,address: the pre-fix IntMult arithmetic traps (signed integer overflow: 65535 * 65535 cannot be represented in type 'int'), the fixed decoder produces the defined wrap (1) and round-trips valid data, and trailing-byte tampering is rejected.

No master merge: MERGEABLE, merge-base is from 2026-06-29 (~1.3 days, well under a week), and the PR-touched files are new to this PR.

Addressed the AI Review Request changes verdict on c9272448 — pushed a7d83200a42.

Blocker (MergeTreeSettings.cpp — ATTACH gate bypass): fixed.
The untyped compression settings (default_compression_codec, marks_compression_codec, primary_key_compression_codec) were validated only inside MergeTreeSettingsImpl::sanityCheck, which MergeTreeData runs for LoadingStrictnessLevel::CREATE only — so ATTACH TABLE ... ENGINE = MergeTree ... SETTINGS default_compression_codec = 'PCO' slipped the gate with allow_experimental_codecs = 0. I extracted the check into checkCompressionCodecSettings and now run it for mode <= LoadingStrictnessLevel::ATTACH, i.e. user ATTACH and SECONDARY_CREATE (RESTORE / DatabaseReplicated internal queries); FORCE_ATTACH (server startup) and FORCE_RESTORE keep skipping it so already-loaded tables are never rejected. (Inline thread resolved.)

Test: added. 04337_pco_codec_untyped_settings now covers the full-form ATTACH ... UUID ... SETTINGS default_compression_codec = 'PCO' with allow_experimental_codecs = 0, a ZSTD(1), PCO chain, marks_compression_codec = 'PCO', plus a positive short-form re-attach with an ordinary codec (confirming the common re-attach path still works). It needs no-ordinary-database/no-replicated-database/no-shared-merge-tree because the full-form ATTACH requires an explicit table UUID (cf. 04046, 04159).

Major (StandaloneDecoder.h:250 — FloatQuant/IntMult semantic range validation): left for your decision.
This is the still-open thread you marked "leaving open for a decision". The bot's latest reply (2026-07-01) adds a new reachability argument that predates your last note there: it claims a client can send a checksummed 0x9d frame via the decompress=1 / CompressedReadBuffer path so the PCO decoder is reached without allow_experimental_codecs, making this a current fail-closed boundary rather than only a future raw-.pco-import concern. I did not implement the per-element guards: after the earlier IntMult accumulator fix the arithmetic is well-defined (no UB / OOB — malformed input yields wrong values, not memory unsafety), the guards add per-element branches to the hot decode loop, and the reference decoder doesn't validate these invariants either — so the correctness-vs-throughput tradeoff on this experimental codec is yours to make. Flagging the new argument so the call is informed; happy to add the conservative checked-arithmetic guards if you want them.

Pushed c452ea51c3e: marked 04337_pco_codec_untyped_settings as no-parallel.

The only real, PR-caused CI failure was that test failing the flaky check (amd_tsan 6×, amd_debug 2×, amd_asan_ubsan 1×). Its full-form ATTACH TABLE ... UUID '<literal>' cases use a fixed, server-global table UUID (required for the full-form ATTACH syntax under the Atomic engine), so when the flaky check runs the same test in parallel workers, two concurrent ATTACHes race on the same UUID and one fails with TABLE_ALREADY_EXISTS before reaching the codec-setting gate, instead of the expected BAD_ARGUMENTS. The single-query failing path already releases the reserved UUID via TemporaryLockForUUIDDirectory, so a sequential run is fine — only concurrency collides. no-parallel matches the sibling test 04159_unique_key_ttl_rejected, which uses a fixed ATTACH UUID for the same reason.

The remaining red — AST fuzzer (amd_debug) Logical error: Inconsistent AST formatting … (STID: 1941-26fa) on CREATE TABLE … (rk Array(Tuple(Array(Float64), Float64))) ENGINE = Memory — is unrelated to this PR (no codec content) and is a known, tracked flake: #108372. The same STID fires across many unrelated PRs and on master (Stress test / AST fuzzer / BuzzHouse).

Pushed 5114dc28353 addressing the second AI Review Request changes finding (temporary_files_codec, TemporaryDataOnDisk.cpp).

getCodec for temporary_files_codec only rejected codecs that requiresColumnTypeToCompress (e.g. the experimental PCO). An experimental codec that does not require a column type — ALP — still passed through and could be used for external sort/aggregation spill files even with allow_experimental_codecs = 0. Now the experimental-codec gate is enforced here too, matching the <compression> config selector and the untyped MergeTree compression settings (marks_compression_codec, ...); experimental codecs can only be specified per column. 04337_pco_codec_untyped_settings gets an ALP case (single codec and inside a chain).

The other finding — FloatQuant / IntMult decomposed-latent range validation in StandaloneDecoder.h — is being discussed in its inline thread.

Pushed a86a41f89c2 addressing the remaining AI Review Request changes finding — the FloatQuant / IntMult decomposed-latent range validation in StandaloneDecoder.h.

The bot's follow-up was correct that this is a current fail-closed boundary, not only a future raw-.pco concern: CompressedReadBufferBase::readHeaderAndGetCodecAndSize dispatches an external frame by its method byte alone (CompressionCodecFactory::instance().get(method)), so a checksummed 0x9d frame reaches this decoder without allow_experimental_codecs. A malformed IntMult / FloatQuant chunk would then decode to fabricated values by wrapping modularly instead of raising a decompression exception.

The join now validates the decomposition against the encoder's split (Modes.h::splitForMode) and throws on violation:

• IntMult: base != 0, secondary < base, and primary * base + secondary fits the latent width.
• FloatQuant: primary fits in latentBits - k bits and the remainder m <= (1 << k) - 1.

These are the exact inverse of the encoder's split, so they never reject a valid stream. New regressions: gtest_pcodec_malformed_block runs a malformed IntMult block through codec->decompress (the shared compressed-frame reader), and gtest_pcodec_encode_bounds covers malformed IntMult/FloatQuant across all latent widths, each with a well-formed baseline that still round-trips. Verified locally: all three touched TUs compile clean under -Weverything -Werror, and a standalone run of the decoder guards passes all 13 valid-decodes-vs-malformed-throws checks.

The higher-level review points from @rienath (whether to add the codec at all, and the maintainability of the hand-ported C++ rewrite vs. linking the Rust crate) remain open for your decision.

Pushed 399aef3e027 to fix the only real, PR-caused CI failure: Build (arm_tidy).

The two gtest files added in a86a41f89c2 failed clang-tidy's bugprone-argument-comment (run with -warnings-as-errors) — 18 inline argument comments whose names did not match the callee's parameter names:

• buildIntMultStandaloneU32(/*base=*/…, /*primary=*/…) — params are base_latent / primary_latent
• buildFloatQuantStream(/*k=*/…, /*primary=*/…, /*m=*/…) — params are primary_latent / secondary_latent

This is why the local -Weverything -Werror compile passed but CI failed: bugprone-argument-comment is a clang-tidy-only check, not a compiler warning, so it only runs in the tidy build. The fix renames the argument comments to match the parameter names exactly — comment-only, no behavioral change.

CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=106222&sha=a86a41f89c22f922a954f6049531af2a4ee3473d&name_0=PR&name_1=Build%20%28arm_tidy%29

The Build (arm_tidy) fix from 399aef3e is green — all 18 builds pass and there are no test failures on the current head; CI is re-running the remaining stages normally.

On the new AI Review blocker (CompressionCodecPco.cpp:358 — registering PCO makes method byte 0x9d decodable on the external-data path with allow_experimental_codecs = 0): leaving this design call to you, since it looks like something you've already decided rather than a fresh regression:

• Same pattern as the existing experimental codec. ALP (0x9c, also isExperimental() == true) is registered globally in CompressionFactory.cpp next to PCO and is reachable through the identical external_data = true path (HTTPHandler decompress=1 and direct-client TCPHandler), also without an allow_experimental_codecs check. So PCO follows the established convention here rather than opening a new class of exposure; the bot's suggested external_data-side isExperimental() rejection would be a cross-cutting change to CompressedReadBufferBase that also alters ALP's behavior.
• This is the documented contract. The NativeFormat spec update already states that the transport emits only NONE/LZ4/ZSTD but the shared CompressedReadBuffer accepts any registered method byte on input.
• Mitigation already chosen. The 399aef3e-era hardening (fail-closed IntMult/FloatQuant decomposition validation via the inverse of Modes.h::splitForMode, plus the malformed-block gtests) is exactly the "decoder is robust on untrusted transport input" approach for this path.

Scope note: external_data = true is set only for transport input; stored MergeTree part reads use external_data = false, so a block there would be transport-only and would not affect reading PCO-compressed parts either way. No code change made — deferring the accept-vs-reject-on-transport decision (and @rienath's higher-level "should we add this codec / C++ vs Rust" question) to you.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 2005/2181 (91.93%) · Uncovered code

Full report · Diff report

Answering questions:

1. About comparison with ALP. Yes, if there are no useful cases when it is better, we are not going to merge PCodec.

2. About maintainability. I ported it to C++ specifically for maintainability - because it makes it more accessible, and the build system is simpler. I was also worried a little about memory management (but likely for nothing, as the input and output buffers shouldn't be managed by the library). And also I used it as an opportunity to optimize performance a little (but with only moderate results). If it is in Rust, we can have it if the library does not need a lot of modifications, but as soon as I faced the need to modify something, I ported it to C++.

As a philosophical observation, today the original language of a library does not matter (for codebases under 10k lines of code, assuming they have good test coverage), you can get what you want in your desired language.