feat(compression): zstd dictionary compression support#131
Conversation
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'lsm-tree db_bench'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.15.
| Benchmark suite | Current: 9439847 | Previous: e99ede9 | Ratio |
|---|---|---|---|
overwrite |
938669.3796464545 ops/sec |
1233004.5504094583 ops/sec |
1.31 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @polaz
There was a problem hiding this comment.
Pull request overview
Adds zstd dictionary-based compression as a first-class CompressionType and threads an optional ZstdDictionary through configuration and (most) block write/read paths so SST data blocks can be compressed/decompressed with a caller-supplied dictionary, with explicit mismatch detection.
Changes:
- Introduces
CompressionType::ZstdDict { level, dict_id }plusZstdDictionary(raw bytes + xxh3-derived fingerprint) and serialization support. - Threads an optional dictionary through config/tree/table writer+reader plumbing and enforces
dict_idmatching viaError::ZstdDictMismatch. - Adds integration/unit tests and README documentation; explicitly rejects
ZstdDictfor blob files (vlog) for now.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/zstd_dict_roundtrip.rs | New integration tests for tree roundtrip + mismatch behavior under zstd dict compression. |
| src/vlog/blob_file/writer.rs | Rejects CompressionType::ZstdDict for blob-file writes. |
| src/vlog/blob_file/reader.rs | Rejects CompressionType::ZstdDict for blob-file reads. |
| src/vlog/blob_file/meta.rs | Updates block read/write calls for new (cfg’d) zstd dict argument. |
| src/tree/mod.rs | Threads configured dictionary into table writer/table open paths (cfg zstd). |
| src/tree/ingest.rs | Threads configured dictionary into ingestion writer/table open paths (cfg zstd). |
| src/table/writer/mod.rs | Stores optional zstd dictionary in table writer and passes it to data-block writes. |
| src/table/writer/index/partitioned.rs | Updates index block writes with new zstd dict argument (currently None). |
| src/table/writer/index/full.rs | Updates index block writes with new zstd dict argument (currently None). |
| src/table/writer/filter/partitioned.rs | Updates filter/TLI block writes with new zstd dict argument (currently None). |
| src/table/writer/filter/full.rs | Updates filter block writes with new zstd dict argument. |
| src/table/util.rs | Extends load_block API to accept optional zstd dictionary for block decoding (cfg zstd). |
| src/table/tests.rs | Updates table tests to pass the new (cfg’d) zstd dict parameter. |
| src/table/scanner.rs | Updates sequential table scanner to call Block::from_reader with new argument (currently None). |
| src/table/multi_writer.rs | Threads optional zstd dictionary through multi-writer rotation (cfg zstd). |
| src/table/mod.rs | Threads dictionary into table reads/iterators and table construction (cfg zstd). |
| src/table/meta.rs | Updates meta block reads with new zstd dict argument. |
| src/table/iter.rs | Stores optional zstd dictionary in table iterator and passes it to data-block loads (cfg zstd). |
| src/table/inner.rs | Adds optional zstd dictionary field to table inner state (cfg zstd). |
| src/table/block_index/volatile.rs | Updates index-block loads with new zstd dict argument. |
| src/table/block_index/two_level.rs | Updates index-block loads with new zstd dict argument. |
| src/table/block/mod.rs | Implements zstd dict compress/decompress in block read/write and enforces dict_id matching. |
| src/lib.rs | Re-exports ZstdDictionary behind the zstd feature. |
| src/error.rs | Adds Error::ZstdDictMismatch { expected, got }. |
| src/config/mod.rs | Adds optional zstd_dictionary to config and a builder method (cfg zstd). |
| src/compression.rs | Adds ZstdDictionary, CompressionType::ZstdDict, and encoding/decoding + tests. |
| src/compaction/flavour.rs | Threads configured dictionary into compaction table-writer setup and table open path (cfg zstd). |
| src/blob_tree/mod.rs | Threads configured dictionary into blob-tree index table writer/open paths (cfg zstd). |
| src/blob_tree/ingest.rs | Threads configured dictionary into blob-tree ingestion table open path (cfg zstd). |
| README.md | Documents the zstd feature and mentions ZstdDict support. |
12bde97 to
e3b3b1b
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds feature-gated Zstd dictionary support: new ZstdDictionary type and CompressionType::ZstdDict, plumbing to thread an optional dictionary through writers, MultiWriter, Table recovery/load, scanners/iterators, and block IO, config validation and error variant, blob-file rejection for ZstdDict, tests, and README docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Config
participant MultiWriter
participant Table
participant BlockIO as Block::write_into/from_file
participant Storage
rect rgba(200, 220, 255, 0.5)
Client->>Config: open(config with zstd_dictionary)
Config->>Config: validate_zstd_dictionary()
end
rect rgba(200, 255, 200, 0.5)
Client->>MultiWriter: create and use_zstd_dictionary(dictionary)
MultiWriter->>MultiWriter: store and apply zstd_dictionary to new Writer(s)
end
rect rgba(255, 230, 200, 0.5)
Client->>Table: flush/write data blocks
Table->>BlockIO: Block::write_into(data, CompressionType::ZstdDict, zstd_dict)
BlockIO->>Storage: write compressed block + metadata(dict_id)
end
rect rgba(255, 200, 200, 0.5)
Client->>Table: reopen / recover
Table->>Table: Table::recover(zstd_dictionary)
Table->>BlockIO: Block::from_file(..., zstd_dict)
BlockIO->>Table: validate dict_id == zstd_dict.id() or return ZstdDictMismatch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
5ba6d1f to
f4f1b7c
Compare
9439847 to
df775a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 69-72: Update the README wording around Zstd to clarify that
CompressionType::ZstdDict (dictionary compression) currently only applies to
table blocks and is not supported for blob files; change the sentences that
imply universal support to explicitly state "dictionary compression
(CompressionType::ZstdDict) improves ratios for table blocks (4–64 KiB) but
blob-file dictionary compression is not yet supported." Also add a short note or
TODO calling out future support for blob-file dictionary compression so readers
know it's a known limitation.
In `@src/config/mod.rs`:
- Around line 283-291: Config currently holds a single zstd_dictionary
(Config::zstd_dictionary) but CompressionPolicy and CompressionType::ZstdDict {
dict_id, .. } can request different dict_id values (used in tree flushing and
compaction paths in src/tree/mod.rs and src/compaction/flavour.rs), which will
cause Error::ZstdDictMismatch when a different dict_id is selected; fix by
either (A) validating at Config initialization that every
CompressionPolicy/level and both data/index policies only reference the same
dict_id and fail early if multiple ids are configured, or (B) replace
Config::zstd_dictionary with a registry (e.g., HashMap<dict_id,
Arc<ZstdDictionary>>) and update call sites in tree::flush/recover and
compaction::flavour to look up the dictionary by dict_id before cloning/using
it.
In `@src/table/mod.rs`:
- Around line 453-460: read_tli() currently hardcodes None when calling
Block::from_file so the zstd_dictionary argument added later never reaches the
index/TLI block reader, allowing CompressionType::ZstdDict to be selected via
Writer::use_index_block_compression() but not decoded on read; fix by either
plumbing the zstd_dictionary through read_tli() into the Block::from_file call
(propagate the zstd_dictionary parameter to read_tli and pass it to
Block::from_file) or enforce rejection up front (modify
Writer::use_index_block_compression() to disallow CompressionType::ZstdDict for
index/filter TLI blocks and return an error), and add corresponding
validation/tests so the chosen approach prevents decode failures at recovery
time.
In `@src/table/writer/filter/partitioned.rs`:
- Around line 140-142: The current filter TLI write passes None for the zstd
dictionary regardless of self.compression which will cause a runtime
ZstdDictMismatch when CompressionType::ZstdDict is selected; update the call
site in partitioned.rs (the filter TLI write path) to explicitly handle
self.compression: match on CompressionType::ZstdDict and either supply the
proper dictionary (if available) or normalize the compression to the
non-dictionary variant (e.g., map ZstdDict -> Zstd) before calling the writer,
and keep passing None for other compression variants so filter blocks never
silently mismatch.
In `@src/table/writer/index/partitioned.rs`:
- Around line 70-72: The partitioned index write paths currently pass a
hardcoded None dictionary to the index writer even when self.compression may be
CompressionType::ZstdDict, which can cause ZstdDictMismatch; update both call
sites in partitioned.rs (the index block write calls that pass None for the dict
at the spots referenced around lines ~70 and ~135) to inspect self.compression
and only pass None when not CompressionType::ZstdDict—when it is ZstdDict,
obtain and pass the appropriate dictionary (or propagate an error if the dict is
unavailable) so the writer receives the matching Zstd dictionary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f49c494d-ee29-4032-bc30-23f6aeb35f15
📒 Files selected for processing (30)
README.mdsrc/blob_tree/ingest.rssrc/blob_tree/mod.rssrc/compaction/flavour.rssrc/compression.rssrc/config/mod.rssrc/error.rssrc/lib.rssrc/table/block/mod.rssrc/table/block_index/two_level.rssrc/table/block_index/volatile.rssrc/table/inner.rssrc/table/iter.rssrc/table/meta.rssrc/table/mod.rssrc/table/multi_writer.rssrc/table/scanner.rssrc/table/tests.rssrc/table/util.rssrc/table/writer/filter/full.rssrc/table/writer/filter/partitioned.rssrc/table/writer/index/full.rssrc/table/writer/index/partitioned.rssrc/table/writer/mod.rssrc/tree/ingest.rssrc/tree/mod.rssrc/vlog/blob_file/meta.rssrc/vlog/blob_file/reader.rssrc/vlog/blob_file/writer.rstests/zstd_dict_roundtrip.rs
0ff14dd to
0426b3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/config/mod.rs (1)
414-439:⚠️ Potential issue | 🟡 MinorNormalize index policies before this dictionary check.
Writer::use_index_block_compression()downgradesCompressionType::ZstdDict { .. }to plainZstd, but this loop still treatsindex_block_compression_policyas if it required a live dictionary. That makesopen()reject configurations that can never write dict-compressed index/TLI blocks, including mixed data/index dict IDs that the writer would otherwise handle by downgrading the index side.Based on learnings,
Writer::use_index_block_compression()intentionally downgradesCompressionType::ZstdDict { .. }toCompressionType::Zstd(level)because index/filter/TLI blocks never carry a dictionary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/mod.rs` around lines 414 - 439, The loop in open() checks index_block_compression_policy for CompressionType::ZstdDict and rejects mismatched dict IDs, but Writer::use_index_block_compression() intentionally downgrades index-side ZstdDict to CompressionType::Zstd, so you must normalize index policies before this check; update the logic that iterates over self.data_block_compression_policy and self.index_block_compression_policy to first map or filter index_block_compression_policy through the same downgrading used by Writer::use_index_block_compression() (or explicitly treat index entries as Zstd without dict) so only true data-side ZstdDict entries are validated for dict_id mismatches and index-side entries are ignored for dict requirements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/config/mod.rs`:
- Around line 414-439: The loop in open() checks index_block_compression_policy
for CompressionType::ZstdDict and rejects mismatched dict IDs, but
Writer::use_index_block_compression() intentionally downgrades index-side
ZstdDict to CompressionType::Zstd, so you must normalize index policies before
this check; update the logic that iterates over
self.data_block_compression_policy and self.index_block_compression_policy to
first map or filter index_block_compression_policy through the same downgrading
used by Writer::use_index_block_compression() (or explicitly treat index entries
as Zstd without dict) so only true data-side ZstdDict entries are validated for
dict_id mismatches and index-side entries are ignored for dict requirements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a7c63f7-8aa0-4227-b2a6-d25802e5255e
📒 Files selected for processing (30)
README.mdsrc/blob_tree/ingest.rssrc/blob_tree/mod.rssrc/compaction/flavour.rssrc/compression.rssrc/config/mod.rssrc/error.rssrc/lib.rssrc/table/block/mod.rssrc/table/block_index/two_level.rssrc/table/block_index/volatile.rssrc/table/inner.rssrc/table/iter.rssrc/table/meta.rssrc/table/mod.rssrc/table/multi_writer.rssrc/table/scanner.rssrc/table/tests.rssrc/table/util.rssrc/table/writer/filter/full.rssrc/table/writer/filter/partitioned.rssrc/table/writer/index/full.rssrc/table/writer/index/partitioned.rssrc/table/writer/mod.rssrc/tree/ingest.rssrc/tree/mod.rssrc/vlog/blob_file/meta.rssrc/vlog/blob_file/reader.rssrc/vlog/blob_file/writer.rstests/zstd_dict_roundtrip.rs
ffdc4d2 to
57afad7
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/table/mod.rs (1)
593-600:⚠️ Potential issue | 🟠 MajorValidate the filter TLI block type before constructing
IndexBlock.This branch bypasses
read_tli()and never checksblock.header.block_type == BlockType::Index. A corruptedfilter_tlihandle can therefore be interpreted as a partition index and feed bogus handles into the bloom path, which risks false negatives and skipped reads.🛠 Suggested fix
let block = Block::from_file( file_handle.as_ref(), filter_tli_handle, metadata.index_block_compression, encryption.as_deref(), #[cfg(feature = "zstd")] None, )?; + if block.header.block_type != BlockType::Index { + return Err(crate::Error::InvalidTag(( + "BlockType", + block.header.block_type.into(), + ))); + } Some(IndexBlock::new(block))As per coding guidelines, "Flag missing validation: unchecked block offset, unvalidated segment metadata from disk" and "When adding validation for on-disk data, add a test that tampers the relevant field and asserts the error."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/table/mod.rs` around lines 593 - 600, The code constructs an IndexBlock from a filter_tli without validating the on-disk block type; call the existing read_tli() helper or, after Block::from_file(...) and before constructing IndexBlock, assert that block.header.block_type == BlockType::Index and return an error if not (use the same error type/path as read_tli), to avoid interpreting corrupted filter_tli as a partition index; also add a unit/integration test that tampers the filter_tli block_type on disk and verifies the code returns the validation error rather than proceeding into the bloom path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/table/mod.rs`:
- Around line 593-600: The code constructs an IndexBlock from a filter_tli
without validating the on-disk block type; call the existing read_tli() helper
or, after Block::from_file(...) and before constructing IndexBlock, assert that
block.header.block_type == BlockType::Index and return an error if not (use the
same error type/path as read_tli), to avoid interpreting corrupted filter_tli as
a partition index; also add a unit/integration test that tampers the filter_tli
block_type on disk and verifies the code returns the validation error rather
than proceeding into the bloom path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5fa0570-8cea-4fbe-949c-0ddced09ebc7
📒 Files selected for processing (30)
README.mdsrc/blob_tree/ingest.rssrc/blob_tree/mod.rssrc/compaction/flavour.rssrc/compression.rssrc/config/mod.rssrc/error.rssrc/lib.rssrc/table/block/mod.rssrc/table/block_index/two_level.rssrc/table/block_index/volatile.rssrc/table/inner.rssrc/table/iter.rssrc/table/meta.rssrc/table/mod.rssrc/table/multi_writer.rssrc/table/scanner.rssrc/table/tests.rssrc/table/util.rssrc/table/writer/filter/full.rssrc/table/writer/filter/partitioned.rssrc/table/writer/index/full.rssrc/table/writer/index/partitioned.rssrc/table/writer/mod.rssrc/tree/ingest.rssrc/tree/mod.rssrc/vlog/blob_file/meta.rssrc/vlog/blob_file/reader.rssrc/vlog/blob_file/writer.rstests/zstd_dict_roundtrip.rs
34411a5 to
b7d5b96
Compare
## 🤖 New release * `coordinode-lsm-tree`: 4.0.0 -> 4.1.0 <details><summary><i><b>Changelog</b></i></summary><p> <blockquote> ## [4.1.0](v4.0.0...v4.1.0) - 2026-03-24 ### Added - *(fs)* io_uring Fs implementation for high-throughput I/O ([#106](#106)) - *(compression)* zstd dictionary compression support ([#131](#131)) ### Documentation - add benchmark dashboard link and update badges ([#151](#151)) - add v4.0.0 fork epoch changelog (all changes since upstream v3.1.1) ### Fixed - *(version)* fsync version file before rewriting CURRENT pointer ([#152](#152)) - thread UserComparator through ingestion guards and range overlap ([#139](#139)) ### Performance - *(bench)* add multi-threaded support to all db_bench workloads ([#155](#155)) - *(merge)* replace IntervalHeap with sorted-vec heap + replace_min/replace_max ([#148](#148)) - *(compaction)* merge input ranges before L2 overlap query ([#146](#146)) ### Refactored - *(version)* comparator API cleanup — TransformContext + rename Run::push() ([#153](#153)) - add #[non_exhaustive] to CompressionType enum </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/). Co-authored-by: sw-release-bot[bot] <255865126+sw-release-bot[bot]@users.noreply.github.com>
Summary
CompressionType::ZstdDict { level, dict_id }variant for zstd dictionary-based block compressionZstdDictionarystruct (raw bytes + xxh3-based dict_id fingerprint)Error::ZstdDictMismatch { expected: u32, got: Option<u32> }for dict_id validationTechnical Details
InvalidTag#[cfg(feature = "zstd")]gating to avoid any overhead when the feature is disabledzstd::bulk::Compressor::with_dictionary(), decompression useszstd::bulk::Decompressor::with_dictionary()ZstdDictentries in data block compression policies must match the provided dictionary'sdict_idKvSeparationOptions::compressionset toZstdDictis rejected (ErrorKind::Unsupported)Table::recover()validates the persisteddata_block_compressiondict_id against the provided dictionaryWriter::use_index_block_compression()silently downgradesZstdDictto plainZstd— dictionaries are trained on data block content, not index/filter structuresErrorKind::UnsupportedforZstdDictat both config and runtime levelsKnown Limitations
Test Plan
--all-features(800+ tests, 0 failures)--no-default-features,--features lz4,--features zstd,--all-featuresCloses #129