Add Sidecar Search Indexes#42
Conversation
Add more block info to references.
There was a problem hiding this comment.
Pull request overview
This PR introduces sidecar search-index streams for MinLZ, allowing search tables to be stored in a separate .mzs stream while keeping the main .mz stream as plain compressed data. It adds library support for generating/consuming sidecars, updates the search-table format/docs for remote block references (0x47), and extends the mz CLI to build/extract sidecars and to search using them.
Changes:
- Add sidecar stream generation (
WriterSidecar,SetSidecar), sidecar building/extraction (BuildSidecar,ExtractSidecar), and sidecar-based searching (SidecarSearcher). - Make per-block search-table compression the default (0x46 emitted when beneficial), plus CLI/documentation updates to reflect new defaults and flags.
- Add a new
mz sidecar {build,extract}CLI and extendmz searchwith optional sidecar usage / auto-detection.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| writer.go | Adds sidecar emission paths (0x44/0x45/0x46 diverted + 0x47 refs) and public WriterSidecar/SetSidecar. |
| sidecar.go | Implements BuildSidecar and ExtractSidecar, including parsing/writing 0x47 remote references. |
| sidecar_search.go | Adds SidecarSearcher that walks the sidecar sequentially and reads main blocks via io.ReaderAt. |
| sidecar_test.go | Comprehensive tests for sidecar generation/searching, concurrency, concat streams, stats parity. |
| search_table.go | Makes table compression default; adds WithoutCompression; adds prefix-driven default tightening. |
| search_reader.go | Extends lazy prev-block support to fetch from io.ReaderAt for sidecar searching. |
| search_examples_test.go | Adds examples for WriterSidecar/BuildSidecar/ExtractSidecar/SidecarSearcher. |
| cmd/mz/sidecar.go | New CLI subcommand for sidecar build/extract. |
| cmd/mz/search.go | Adds --sidecar and sidecar auto-detection; switches to sidecar search when present. |
| cmd/mz/compress.go | Adds -search.sidecar output mode and refactors stream processing options. |
| minlz.go | Adds chunk type constant for 0x47. |
| SPEC.md / SPEC_SEARCH.md / SEARCH.md / README.md / search_test.go | Spec/doc/test updates for 0x46 default compression + 0x47 sidecar references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Added Extras to long prefixes: ExtrasBy default a long-prefix table records one cfg := minlz.NewSearchTableConfig().
WithMatchLen(8).
WithLongPrefix([]byte(`"timestamp":`)).
WithExtras(8) // 9 hashes per occurrence (matchLen+extras = 16)Extras effectively turn the table into a When to use it:
When to avoid it:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements MinLZ sidecar search-index functionality, long-prefix "extras" (E extra match windows), compressed search-table support (0x46), and remote block references (0x47). It adds a reference encoder/decoder, end-to-end CLI wiring (build/extract/search/compress), writer-sidecar emission, sidecar searcher, extensive tests, and documentation/spec updates. ChangesSidecar Search Functionality
Documentation and Code Cleanup
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/mz/search.go (1)
67-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject an explicit
--sidecarwhen more than one input/glob match is searched.The same sidecar path is forwarded into every
searchFilecall here. With multiple matched inputs, later files will be searched against an unrelated index, which can silently produce false positives/negatives instead of failing fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/mz/compress.go`:
- Around line 224-239: The buffered writers (bw/sbw) and output files
(dstFile/sideFile) can report errors on Flush() and Close() that are currently
ignored, so update the compress command around wr.Reset/bw and wr.SetSidecar/sbw
to check and propagate errors from sbw.Flush(), bw.Flush(), sideFile.Close(),
and dstFile.Close(); ensure wr.Close() errors are handled too. Concretely, after
calling wr.Close() call and check bw.Flush() and sbw.Flush() (if used) and then
close the underlying files (dstFile.Close(), sideFile.Close()) and pass any
non-nil errors to exitErr (or return them) so disk-full or final short-write
failures are reported; reference functions/vars: wr.Reset, wr.Close,
wr.SetSidecar, bw, sbw, dstFile, sideFile, and exitErr.
In `@cmd/mz/sidecar.go`:
- Around line 104-108: Before opening/truncating the destination, detect and
reject cases where the destination aliases the input: resolve both src and dst
to their absolute/canonical paths (e.g., filepath.Abs and/or
filepath.EvalSymlinks) and compare them, and if they are equal return an error
instead of proceeding; apply the same check in the second block (the code around
the 175-186 range). Insert this validation before any file is opened/truncated
(i.e., before the code that uses dst and before calling BuildSidecar) and use
the existing variables src, dst, outFile, minlzSidecarExt and the BuildSidecar
call to locate where to add the check.
- Around line 239-268: The code opens the input then creates/truncates outputs
(sideOut/newOut) which can clobber the source or alias outputs; before opening
any files (i.e., before os.Open(src) and before creating sideOut/newOut) resolve
and compare the canonical paths for src, dst (derived from *outFile or
minlzSidecarExt) and *newStream using filepath.Abs and filepath.EvalSymlinks,
and if any two canonical paths are equal return an error via exitErr (e.g.,
"destination X conflicts with source Y" or "outputs X and Y alias each other");
update the logic around variables src, dst, *outFile, *newStream, sideOut and
newOut (and any ExtractSidecar invocation) so path-conflict checks run and fail
fast prior to opening/truncating files.
In `@internal/reference/search_decoder.go`:
- Around line 186-214: ParseRemoteBlockRefChunk currently accepts an empty 0x47
payload and returns (nil, nil); change it to reject empty payloads by adding an
explicit check at the start of ParseRemoteBlockRefChunk that if len(payload) ==
0 it returns (nil, fmt.Errorf("minlz: 0x47 empty payload")) so behavior matches
sidecar.go and other uvarint error patterns.
- Around line 95-104: The parser currently allows tc and nBlocks values beyond
production limits; add validation to mirror the production 0x46 header checks by
returning an error when tc > 16 and when nBlocks > 16. Locate the parsing
routine that computes log2bs, expectedSize, blockSize and nBlocks (using
variables log2bs, expectedSize, blockSize, nBlocks and the header field tc) and
insert guards that reject (fmt.Errorf) any tc greater than 16 and any computed
nBlocks greater than 16, consistent with the existing error style used for
invalid huff0 block sizes.
In `@minlz_test.go`:
- Around line 36-39: Remove the unused constant maxUint and adjust the
declaration of maxInt accordingly: delete maxUint and define maxInt using the
expression previously derived from maxUint (int(^uint(0) >> 1)) or inline the
numeric value where used; update the const block containing maxInt so there are
no unused constants (references: maxUint, maxInt).
- Around line 1753-1755: The example test in minlz_test.go uses the
non-canonical marker "// OUTPUT:" so Go's example runner won't match the
expected stdout; change the marker to the canonical "// Output:" (capitalize
only the O) for the example block that currently shows "// OUTPUT:" and the two
lines "Callback: Chunk Custom Data <nil>" and "Stream data: some data" so the
example test will be recognized and validated by go test.
In `@README.md`:
- Around line 914-918: Update the paragraph that currently claims there are "no
ports" to accurately reflect the new table entries by replacing that statement
with a concise sentence noting that official Go and Rust ports exist and an
experimental C port/Gist is available (or alternatively remove the absolute "no
ports" claim); locate the surrounding text in README.md that asserts "no ports"
and modify it to say something like "There are Go and Rust ports (and an
experimental C gist) — see the table below" so the prose aligns with the table
entries.
In `@search_compressed_test.go`:
- Around line 683-685: The test's sub-block sum omits the new BlocksSparse field
from CompressedSearchStats; update the sum calculation (totalBlocks) to include
got.BlocksSparse and adjust the failure message if desired so the assertion
compares got.SubBlocks against the sum that now includes BlocksSparse
(referencing totalBlocks, got.BlocksOwnTable, got.BlocksGlobalTable,
got.BlocksRaw, got.BlocksRLE, and got.BlocksSparse).
In `@search_compressed.go`:
- Around line 844-859: The fast-path parser parseSearchTableCompressedHeader
currently skips validating the fixed 0x46 chunk header; update it to mirror the
lightweight structural checks in parseSearchTableCompressed by verifying the
payload is long enough for the header byte and that payload[0] == 0x46 (or the
same header constant used in parseSearchTableCompressed), and return an error if
the header byte is wrong or overall payload length is insufficient before
reading reductions; keep the existing parseSearchInfo call and prefix-size
offset logic but perform the header check before returning reductions to prevent
malformed chunks from bypassing validation.
In `@search_table.go`:
- Around line 156-158: The WithExtras setter currently casts int to uint8 before
validation causing overflow (e.g., 256 -> 0); fix by validating the input range
in SearchTableConfig.WithExtras (check n >= 0 && n <= 255) before doing c.extras
= uint8(n) and handle out-of-range values explicitly (return unchanged config
and/or return an error/modify the API to propagate errors), and likewise apply
the same pre-cast range check to the other setter(s) that cast an int to uint8
(the related setters referenced around lines 230-237) and make validate() assert
that extras was set within [0,255] to catch any future regressions.
In `@SEARCH.md`:
- Around line 393-397: The fenced code block containing the example mz sidecar
commands (the lines starting with "mz sidecar build", "mz sidecar extract", and
"mz search --sidecar") lacks a language tag; update the opening triple-backtick
to include a language such as bash or text (e.g. ```bash) so linters recognize
it as a shell snippet and avoid markdown lint failures.
In `@sidecar_search.go`:
- Around line 385-395: The code incorrectly assumes len(tables)==1 implies a
single-config stream and sets s.sideDeferred based only on per-block table
count, which causes resolveSideDeferred to compare deferred hashes against
nextTables[0] from a different config; either restrict this optimization to
single-config streams by adding a guard requiring len(s.streamInfos)==1 before
setting s.sideDeferred (referencing tables, s.sideDeferred, patternDeferHashes,
flushBatch, s.sideDeferredHashes, s.sideDeferredBase), or instead attach the
deferred config identity when creating s.sideDeferred (e.g., store the table's
config id) and update resolveSideDeferred to only compare hashes against
nextTables[0] if that table’s config id matches the deferred config (use
resolveSideDeferred, nextTables and the stored deferred config id).
- Around line 198-206: The code currently destructively updates s.maxBlock when
reading the first sidecar header (using streamBlockSizeFromHeaderByte) and never
recomputes it for subsequent ChunkTypeStreamIdentifier headers, causing later
larger streams to be rejected; change the logic to keep the configured ceiling
separate (e.g., preserve the original configured max as s.maxBlockConfigured or
similar), set s.sideMaxBlk from streamBlockSizeFromHeaderByte for each header,
and recompute a per-stream active limit (e.g., activeMax :=
min(s.maxBlockConfigured, s.sideMaxBlk)) whenever you see a
ChunkTypeStreamIdentifier so both validation and prevLazy use this activeMax
instead of mutating s.maxBlock. Ensure updates at the same spots mentioned
(current block and the other occurrence around lines 483-489) so each
concatenated stream recalculates limits on its header.
In `@sidecar_test.go`:
- Around line 567-577: The test currently fails on valid coalesced reads because
it treats any overlap in rr.ranges as an error, but decodeBatch intentionally
reads a conservative [rangeStart, rangeEnd) for the last ref in a batch; update
the check to only fail when one range is fully contained in the previous range
(i.e., ranges[i][0] < ranges[i-1][0] || ranges[i][1] <= ranges[i-1][1]) instead
of any partial overlap, so allow partial overlaps that extend past the previous
end; refer to rr.ranges and decodeBatch when making this change.
In `@SPEC_SEARCH.md`:
- Line 14: The heading "1.1 Sidecar Search Index Streams" is using H3 (###)
after an H1, breaking incremental heading levels; change that header from "###
1.1 Sidecar Search Index Streams" to H2 ("## 1.1 Sidecar Search Index Streams")
so markdownlint-compliant incremental heading levels are preserved.
- Around line 436-446: The fenced pseudocode block lacks a language identifier
which triggers markdownlint; update the fence surrounding the snippet (the block
containing variables like P, pfx, K, M, E and the loop/checked logic) to include
a language tag such as `text` (e.g., change the opening ``` to ```text) so the
block is properly marked as non-executable pseudocode.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72ba34b9-ab45-42b9-b290-f8fe017efa08
📒 Files selected for processing (44)
README.mdSEARCH.mdSPEC.mdSPEC_SEARCH.md_generate/gen.gobenchmarks_test.gocmd/internal/filepathx/filepathx.gocmd/internal/filepathx/filepathx_test.gocmd/internal/readahead/reader.gocmd/internal/shttp/seekinghttp.gocmd/mz/compress.gocmd/mz/main.gocmd/mz/search.gocmd/mz/sidecar.godecode.goencode.goencode_l3.goencode_test.gofuzz_test.goindex.goindex_test.gointernal/fuzz/helpers.gointernal/reference/decoder.gointernal/reference/search_decoder.gointernal/reference/search_encoder.gointernal/reference/search_test.gointernal/reference/stream.golz4convert.golz4convert_test.gominlz.gominlz_test.gosearch_compressed.gosearch_compressed_test.gosearch_examples_test.gosearch_index.gosearch_reader.gosearch_table.gosearch_test.gosidecar.gosidecar_search.gosidecar_test.gounsafe_enabled.gowriter.gowriter_test.go
…eductions byte only) was weaker than the full parser's (off+1+4+2 > len(payload), room for the whole fixed header: reductions + crc + log2bs + tc). I tightened the header parser to mirror the full one exactly.
Sidecar Streams
Search tables normally live inside the compressed stream — one per block,
right in front of the corresponding data. A sidecar stream instead puts
every search table into a separate file, leaving the main
.mzstream asplain compressed data. Searching then takes two inputs: the sidecar (read
once, top-to-bottom) and the main stream (random-access via
io.ReaderAt,typically a regular file).
Why?
.mzwaswritten long ago, or it lives on read-only storage.
BuildSidecardecodeseach block, builds fresh tables, and writes the sidecar — the original
.mzis never touched.data often can't. Putting them on different storage tiers (or even
different hosts) is a clean way to express that.
compressed with
-searchbut later want to slim the main file withoutlosing search, extract the index to a sidecar and strip the main.
writer only carries one config; a sidecar can carry many (for example one
tuned for short patterns and one tuned for JSON keys).
The main stream and the sidecar are both valid MinLZ streams on their
own. A regular
NewReaderover a sidecar produces zero data bytes (onlyskippable chunks); over a stripped main stream it produces the original
payload.
Sidecar files conventionally use the
.mzsextension (sits next to.mz),but they are ordinary MinLZ streams and can have any filename.
Three ways to produce a sidecar
1. While compressing —
WriterSidecarThe main writer behaves normally; the search-table chunks are diverted to a
second
io.Writer:2. From an existing stream that has no indexes —
BuildSidecarReads the source once, decompresses each block, builds fresh tables, and
writes the sidecar. The source is not modified:
Pass
SidecarSearchTablemultiple times to embed several configs in onesidecar. At search time a block is skipped if any embedded table proves
the pattern absent; tables that don't apply to the query are ignored.
3. From an existing indexed stream —
ExtractSidecarCopies the existing
0x44/0x45/0x46chunks verbatim into a sidecar. Nodecoding required. Two modes:
Searching with a sidecar
The searcher walks the sidecar once and only touches the main stream for
blocks that aren't proven absent. Adjacent must-decode blocks are coalesced
into a single
ReadAt; skipped blocks issue no I/O against the main streamuntil the caller invokes
result.PrevBlock()for boundary context (thenthey are fetched and decoded on demand).
SidecarSearcheraccepts the sameBlockSearchOptions asBlockSearcher(
BlockSearchBailOnMissing,BlockSearchCollectStats,BlockSearchIgnoreCRC, …). Multiple goroutines may run independentSidecarSearchers against the same main file concurrently — each owns itsown sidecar
io.Readerand the underlyingReadAtis shared safely.Commandline
mz sidecar buildaccepts most of the same-search.*flags asmz c -search(match length, prefixes, compression, population limits).The
-search.lensflag is comma-separated to request multiple configs inone sidecar.
Sidecar format at a glance
A sidecar stream contains, in order:
0xff) — same maximum block size as the main stream.0x44) — one per embedded config.0x45/0x46) — one per configthat produced a usable table for that block.
0x47) carrying the block'soffset in the main stream and its uncompressed size.
0x20).For format details see SPEC_SEARCH.md §1.1 / §2.3.
Summary by CodeRabbit
New Features
Improvements
Documentation