Improve prefix indexes#44
Merged
Merged
Conversation
Fix prefixed indexes missing the final match (prefix at the end of block).
This forced decoding blocks just to check if this match existed.
Fix concurrent encoding (streaming) not indexing correctly, and not always providing overlap bytes. We will not write an search block on Flush calls.
Both were implementation mistakes. Specification is clarified and implementation guide updated.
Before/after example:
```
λ go build&&mz c -2 -bs=1MB -search -search.sidecar=false -search.prefixes=',\":/{}[]' -search.len=8 -search.lim=5 github-june-2days-2019.json
Compressing github-june-2days-2019.json -> github-june-2days-2019.json.mz 6273951764 -> 1056068951 [16.83% 5.941:1]; 3995.9MB/s
λ mz search -c -v \"DeleteEvent\",\"actor\":{\"id\":32803738,\"login\":\"promehul\" github-june-2days-2019.json.mz
github-june-2days-2019.json.mz: search info: matchLen=8 baseTableSize=20 (1048576 entries) mask-prefix (9 bytes)
4
github-june-2days-2019.json.mz took 5.839s 1074.5 MB/s
Blocks total: 5984, skipped: 9 (0.2%), deferred: 1 (0.0%, 1 skipped)
Blocks searched: 5975 (99.8%), false positive: 5972 (99.9%)
Bytes skipped: 890263 compressed, searched: 6264514580 uncompressed
Tables: 5984 present, 0 missing, 0 unusable
Table bits/byte: 0.0981, log2: 18.7, avg reductions: 1.3
Table total: 76897877 bytes, avg 12850 bytes/table, 1.23% of 6273951764 uncompressed
Table population: avg 3.4%, min 2.5%, max 5.0%
Table types: 0 uncompressed (0x45), 5984 compressed (0x46)
Compressed tables: 5984 (100.0% of total), 76897877 wire bytes, 327819264 uncompressed bitmap bytes (23.46% ratio)
Sub-blocks: 10026 total (10026 tabled, 0 raw, 0 RLE, 0 sparse); 9621 tables emitted (share=0.96 tables/tabled-block)
Payload bytes: tabled=76182921 raw=0 RLE=0 sparse=0; table-header bytes=429666
λ git checkout -
Switched to branch 'fix-prefix-indexed'
Your branch is up to date with 'klauspost/fix-prefix-indexed'.
λ go build&&mz c -2 -bs=1MB -search -search.sidecar=false -search.prefixes=',\":/{}[]' -search.len=8 -search.lim=5 github-june-2days-2019.json
Compressing github-june-2days-2019.json -> github-june-2days-2019.json.mz 6273951764 -> 1056068951 [16.83% 5.941:1]; 4034.6MB/s
λ mz search -c -v \"DeleteEvent\",\"actor\":{\"id\":32803738,\"login\":\"promehul\" github-june-2days-2019.json.mz
github-june-2days-2019.json.mz: search info: matchLen=8 baseTableSize=20 (1048576 entries) mask-prefix (9 bytes)
4
github-june-2days-2019.json.mz took 771ms 8137.4 MB/s
Blocks total: 5984, skipped: 5970 (99.8%), deferred: 5969 (99.7%, 5962 skipped)
Blocks searched: 14 (0.2%), false positive: 11 (78.6%)
Bytes skipped: 976920471 compressed, searched: 13953044 uncompressed
Tables: 5984 present, 0 missing, 0 unusable
Table bits/byte: 0.0981, log2: 18.7, avg reductions: 1.3
Table total: 76897877 bytes, avg 12850 bytes/table, 1.23% of 6273951764 uncompressed
Table population: avg 3.4%, min 2.5%, max 5.0%
Table types: 0 uncompressed (0x45), 5984 compressed (0x46)
Compressed tables: 5984 (100.0% of total), 76897877 wire bytes, 327819264 uncompressed bitmap bytes (23.46% ratio)
Sub-blocks: 10026 total (10026 tabled, 0 raw, 0 RLE, 0 sparse); 9621 tables emitted (share=0.96 tables/tabled-block)
Payload bytes: tabled=76182921 raw=0 RLE=0 sparse=0; table-header bytes=429666
```
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefines encoder/index overlap and long-prefix tail indexing, adds writer final/omitTrailing semantics to omit incomplete trailing tables on Flush, implements rolling decoded tails in reader/sidecar for multi-block boundary scans, extends stats and window metrics, and adds tests and docs for streaming behavior. ChangesLong-prefix search-table boundary semantics and streaming flush behavior
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
search_test.go (1)
3890-3939: 💤 Low valueUnchecked errors from
w.Writecalls in streaming branch.The streaming Write calls (line 3904) do not check errors. While errors are eventually surfaced by
w.Close(), an early-failing block could leave the writer in a partially-valid state and the subsequent Close error message might be confusing. Consider checking errors inline for clearer test diagnostics.Suggested improvement
if stream { for i := 0; i < len(data); i += 1024 { - w.Write(data[i:min(i+1024, len(data))]) + if _, err := w.Write(data[i:min(i+1024, len(data))]); err != nil { + t.Fatal(err) + } } } else if err := w.EncodeBuffer(append([]byte(nil), data...)); err != nil {🤖 Prompt for 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. In `@search_test.go` around lines 3890 - 3939, In TestSearchLongPrefixNoFalseNegatives the streaming branch calls w.Write(...) in a loop without checking returned errors; change the loop to capture and handle the error from Writer.Write (e.g., if err := w.Write(...); err != nil { t.Fatalf("write failed: %v", err) }) so write failures are reported immediately instead of only surfacing from w.Close(), referencing the w.Write calls inside the streaming branch and leaving w.Close() as the final cleanup.search_reader.go (1)
1446-1484: ⚡ Quick winLong-prefix deferral logic is conservative but may miss optimizations.
The current implementation defers absent windows only when the first prefix occurrence has all
E+1windows present (firstAllPresent). When the first occurrence is incomplete (some windows absent), it returnsniland forces block decode.The spec (B.4.3) states: "defer all absent windows after the first present one." This could be interpreted as deferring from all occurrences after the first occurrence with at least one present window, not requiring all windows to be present.
For an incomplete first occurrence:
- Its absent windows should be in block N+1's table (per the overlap invariant).
- Deferring them would allow skipping block N if they're missing in N+1.
- The current code decodes block N conservatively instead.
This is safe but potentially less optimal than the byte-prefix logic (which defers after the first present window, regardless of later windows).
Consider aligning with byte-prefix logic:
Proposed adjustment
-first := true -firstAllPresent := false +firstHasPresent := false var absent []uint32 for i := 0; i <= len(pattern)-pl-ml-ex; i++ { if !bytes.Equal(pattern[i:i+pl], cfg.longPrefix) { continue } - allPresent := true var occAbsent []uint32 for j := 0; j <= ex; j++ { h := hashValue(readLE64Pad(pattern[i+pl+j:]), cfg.baseTableSize, cfg.matchLen) if table[(h&mask)>>3]&(1<<((h&mask)&7)) == 0 { - allPresent = false occAbsent = append(occAbsent, h) + } else if !firstHasPresent { + firstHasPresent = true } } - if first { - firstAllPresent = allPresent - first = false - continue - } absent = append(absent, occAbsent...) } -if !firstAllPresent || len(absent) == 0 { +if !firstHasPresent || len(absent) == 0 { return nil }Or document the design choice if the stricter check is intentional.
🤖 Prompt for 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. In `@search_reader.go` around lines 1446 - 1484, The long-prefix deferral currently uses firstAllPresent (set only when the first occurrence has all E+1 windows present) which prevents deferring when the first occurrence is partially absent; change to record whether the first occurrence has at least one present window (e.g., compute occAbsent for the first match and set firstHasAnyPresent := len(occAbsent) < (ex+1) or equivalently firstHasAnyPresent := !allAbsent), continue, and then use that boolean (instead of firstAllPresent) to decide whether to return absent or nil; update the loop in the searchTableTypeLongPrefix case (variables: first, firstAllPresent, occAbsent, absent, allPresent) to implement this byte-prefix–compatible logic.
🤖 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.
Nitpick comments:
In `@search_reader.go`:
- Around line 1446-1484: The long-prefix deferral currently uses firstAllPresent
(set only when the first occurrence has all E+1 windows present) which prevents
deferring when the first occurrence is partially absent; change to record
whether the first occurrence has at least one present window (e.g., compute
occAbsent for the first match and set firstHasAnyPresent := len(occAbsent) <
(ex+1) or equivalently firstHasAnyPresent := !allAbsent), continue, and then use
that boolean (instead of firstAllPresent) to decide whether to return absent or
nil; update the loop in the searchTableTypeLongPrefix case (variables: first,
firstAllPresent, occAbsent, absent, allPresent) to implement this
byte-prefix–compatible logic.
In `@search_test.go`:
- Around line 3890-3939: In TestSearchLongPrefixNoFalseNegatives the streaming
branch calls w.Write(...) in a loop without checking returned errors; change the
loop to capture and handle the error from Writer.Write (e.g., if err :=
w.Write(...); err != nil { t.Fatalf("write failed: %v", err) }) so write
failures are reported immediately instead of only surfacing from w.Close(),
referencing the w.Write calls inside the streaming branch and leaving w.Close()
as the final cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 010dbbc0-64de-43ff-b751-2a633f42e71f
📒 Files selected for processing (9)
README.mdSEARCH.mdSPEC_SEARCH.mdinternal/reference/search_encoder.gosearch_index.gosearch_reader.gosearch_table.gosearch_test.gowriter.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
search_reader.go (1)
328-345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset boundary/forward state when a new concatenated stream starts.
tailBufis new per-stream state, but the inline reader never clears it atchunkTypeEOF/ the followingChunkTypeStreamIdentifier. That leaves the first block of the next stream eligible for boundary scans andErrSearchForwardre-dispatch against bytes from the previous stream, which can produce false matches, wrong offsets, or unnecessary decodes.SidecarSearcheralready does the reset on the same boundary.🔧 Suggested reset points
case chunkTypeEOF: + if s.deferred != nil { + if err := s.flushDeferred(nil, fn); err != nil { + return err + } + } + s.pending = nil + s.prevBlock = nil + s.prevLazy = nil + s.tailBuf = s.tailBuf[:0] s.readHeader = false continue case ChunkTypeStreamIdentifier: ... s.blockStart = 0 s.pending = nil + s.prevBlock = nil s.prevLazy = nil s.blockTable = nil s.blockTableUnusable = false + s.tailBuf = s.tailBuf[:0] continueAlso applies to: 474-504
🤖 Prompt for 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. In `@search_reader.go` around lines 328 - 345, Reset the per-stream boundary/forward state when a new concatenated stream starts: on encountering chunkTypeEOF (and immediately on the following ChunkTypeStreamIdentifier handling) clear tailBuf (nil or zero-length), reset tailOff to 0, zero bbuf, clear prevLazy to nil and reset prevLazyStore, and clear deferred, pending and cstDec so no leftover lazy/decoder state or pending ErrSearchForward re-dispatches cross stream boundaries; apply the same reset logic in the inline reader code paths mentioned (and the similar block at lines 474-504) and ensure bufferSkippedBlock / resolvePending invocations respect this reset.
🤖 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 `@search_reader.go`:
- Around line 355-359: stats.Windows is initialized from the first usable
SearchTableConfig and then mixed with later, incompatible table layouts (because
prefix mode, matchLen, long-prefix extras, and baseTableSize affect window
layout), producing mislabeled counts; modify the logic around
searchWindows/winInit and stats.Windows so that after you initialize
searchWindows and set winInit from the first usable SearchTableConfig you detect
any subsequent config drift (compare prefixMode, matchLen, longPrefixExtras,
baseTableSize from the current SearchTableConfig against the stored initial
layout) and if they differ either stop updating stats.Windows (disable further
tallies under collectStats) or reset/segment window stats per config; implement
the simplest fix by stopping collection after drift: when a mismatch is detected
set a flag (e.g., collectStats=false or winInit=false) and skip further updates
to stats.Windows in the code paths that tally per-table stats.
---
Outside diff comments:
In `@search_reader.go`:
- Around line 328-345: Reset the per-stream boundary/forward state when a new
concatenated stream starts: on encountering chunkTypeEOF (and immediately on the
following ChunkTypeStreamIdentifier handling) clear tailBuf (nil or
zero-length), reset tailOff to 0, zero bbuf, clear prevLazy to nil and reset
prevLazyStore, and clear deferred, pending and cstDec so no leftover
lazy/decoder state or pending ErrSearchForward re-dispatches cross stream
boundaries; apply the same reset logic in the inline reader code paths mentioned
(and the similar block at lines 474-504) and ensure bufferSkippedBlock /
resolvePending invocations respect this reset.
🪄 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: 8716f1d3-4047-4444-b20c-190e743ff38d
📒 Files selected for processing (4)
cmd/mz/search.gosearch_reader.gosearch_test.gosidecar_search.go
🚧 Files skipped from review as they are similar to previous changes (1)
- search_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix prefixed indexes missing the final match (prefix at the end of block). This forced decoding blocks just to check if this match existed.
Fix concurrent encoding (streaming) not indexing correctly, and not always providing overlap bytes. We will not write an search block on Flush calls.
Both were implementation mistakes. Specification is clarified and implementation guide updated.
Before/after example:
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests