Support parallel AddIndexEntry in trie index

[zaidoon1]

Summary

The trie builder previously declined the parallel-add protocol, forcing serial compression when used as a custom index. Opt it into the two-phase protocol: prepare separators and size-estimate counters on the emit thread, then commit trie entries in order on the writer thread.

Keep estimation state separate from writer-owned buffered entries so EstimatedSize() cannot race with entry commits. Preserve serial separator and sequence-number handling, including repeated user keys spanning multiple blocks and the last-block boundary.

Safety and compatibility

The prepared slot is transferred through the existing ordered table-builder pipeline; final serialization runs after workers join. Builders that do not support the protocol still force serial compression. The serialized trie layout is unchanged, and this PR does not change the existing block-offset truncation above 4 GiB.

Test Plan

TrieIndexFactoryTest.ParallelMatchesSerialOutput checks byte-identical serial/parallel output and seeks, including a multi-block run of one user key. ParallelPipelineOverlapsPrepareAndFinish exercises concurrent callbacks through a bounded handoff queue. Other cases cover monotonic size bounds and unfilled prepared entries.

TrieIndexDBTest.ParallelCompressionWithTrieIndex checks end-to-end reads in the three custom-index write modes; ParallelCompressionWithHashStandardIndexAndTrieUdi covers the supported hash/trie combination.

These are included in the 628 focused engine cases that pass normally and with ASSERT_STATUS_CHECKED=1 plus COERCE_CONTEXT_SWITCH=1. Source and format checks pass. No compression-throughput gain or race-sanitizer result is claimed.

Validation used the complete stack on macOS ARM64 with Apple Clang. The published #14968 tree (b9f332dd) is identical to the tested revision (91417d877); intermediate PR heads were not tested independently.

Both full make check runs completed with failures: 173/2095 jobs normally and 152/2088 with ASSERT_STATUS_CHECKED=1. All four assertion cases reproduce on unchanged upstream main; broad timeout causes remain unresolved. The normal run reused 755 jobs from the earlier fix revision. No Linux, Windows, ASAN, UBSAN, TSAN, or hosted CI validation is claimed.

Series

Depends on #14965. Next: #14968.

Changes since #14965. GitHub shows the cumulative stack against main until the preceding PRs land.

[github-actions]

⚠️ clang-tidy: 1 warning(s) on changed lines

Completed in 988.0s.

Summary by check

Check	Count
cppcoreguidelines-pro-type-member-init	1
Total	1

Details

utilities/trie_index/trie_index_test.cc (1 warning(s))

utilities/trie_index/trie_index_test.cc:6069:10: warning: constructor does not initialize these fields: handle [cppcoreguidelines-pro-type-member-init]

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 03ee07a

---

Summary

Clean, well-structured PR that adds parallel compression support to the trie index builder. The split of AddIndexEntry into PrepareAddEntry (emit thread) + FinishAddEntry (BG writer) is logically correct and faithfully mirrors the synchronous path. The same-user-key detection split is sound. Thread safety is handled via atomic mirrors for EstimatedSize(). Four new tests cover the core parallel protocol, serial-parallel equivalence, and edge cases.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Redundant non-atomic counter maintenance -- trie_index_factory.h

• Issue: Both total_separator_bytes_ (non-atomic) and total_separator_bytes_atomic_ (atomic) are maintained in lockstep. The non-atomic total_separator_bytes_ is never read after this PR -- EstimatedSize() was the sole consumer and now reads the atomic. The non-atomic counter appears to be dead code.
• Root cause: The non-atomic counter was preserved for backward compatibility but is no longer consumed.
• Suggested fix: Remove total_separator_bytes_ entirely and use only the atomic, or document why it's kept (e.g., for debug assertions).

M2. No concurrent-thread test for EstimatedSize() -- trie_index_test.cc

• Issue: ParallelEstimatedSizeStaysMonotonic calls PrepareAddEntry and FinishAddEntry sequentially on the same thread. It does NOT exercise the actual concurrent scenario where EstimatedSize() is called on the emit thread while FinishAddEntry runs on a BG worker thread. The test comment acknowledges this.
• Suggested fix: Add a stress test with actual threading, or use COERCE_CONTEXT_SWITCH=1 with sync-point-based concurrency.

M3. Missing parallel-path test for same-user-key overflow runs -- trie_index_test.cc

• Issue: ParallelMatchesSerialOutput has one same-user-key boundary but no large overflow runs (3+ consecutive same-separator blocks). The sync path has LargeOverflowRun and MixedSameKeyRuns tests but no parallel equivalents. The buffer-back-check in FinishAddEntry for long runs deserves dedicated coverage.
• Suggested fix: Add ParallelLargeOverflowRunMatchesSerial with 5+ same-key blocks.

🟢 LOW / NIT

L1. separator_key string allocation in PrepareAddEntry -- trie_index_factory.cc:279

• Issue: ToString() allocates on every call. Ring buffer reuse amortizes this after warmup. Not a bottleneck since it's called once per data block.

L2. Test helper EntryCtx uses raw bit-shifting instead of PackSequenceAndType -- trie_index_test.cc:52

• Issue: Inconsistent with SeekCtx which uses the canonical packing function. Both produce correct results.

Cross-Component Analysis

Context	Assumptions hold?	Action needed?
Parallel compression	YES - emit/BG writer split correct	None
Single-threaded fallback	YES - AddIndexEntry sync path unchanged	None
kCustomOnly mode	YES - index_builder null, custom_indexes has trie	None
kStandardDefault mode	YES - both built-in and custom entries staged	None

Key invariant verification: The parallel path produces identical buffered_entries_ to the sync path because (1) FindShortestSeparator receives identical inputs, (2) FinishAddEntry runs serially in commit order so back() is always the correct preceding entry, and (3) tag assignment logic is duplicated identically.

Positive Observations

1. Clean Prepare/Finish split faithfully mirrors the built-in index builder pattern
2. Defensive valid flag prevents processing stale ring buffer data
3. Ring buffer flag reset (custom_entries_prepared = false) at top of EmitBlockForParallel prevents stale-flag bugs
4. ParallelMatchesSerialOutput test verifies byte-identical output
5. SkipCustomPrepare sync point is a thoughtful test hook for flag-reset safety

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 8d593ad

---

Summary

Solid refactoring that cleanly separates the built-in index behind the public IndexFactoryBuilder interface, enables parallel compression for custom indexes, and introduces the index_mode enum to replace legacy UDI booleans. The architecture is well-designed with clear thread-safety contracts. The parallel protocol split (prepare on emit thread, finish on BG writer) is correctly implemented with disjoint state for the trie builder.

High-severity findings (0):
No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. TrieIndexBuilder::EstimatedSize() relies on non-atomic reads of emit-thread-only counters -- trie_index_factory.cc:236

• Issue: EstimatedSize() reads estimated_separator_bytes_ and estimated_num_entries_ as plain uint64_t fields. The documentation says these are written only by emit-thread callbacks (PrepareAddEntry/AddIndexEntry), and EstimatedSize() is called on the emit thread too. This is correct and safe. However, the contract comment on IndexFactoryBuilder::EstimatedSize() in index_factory.h says "may be called on the emit thread concurrently with FinishAddEntry on the BG writer thread," which implies a cross-thread call is allowed. If a future caller invokes EstimatedSize() from the BG writer thread (which the interface comment seems to permit), the reads would be data races on these non-atomic fields.
• Root cause: The trie builder's implementation relies on a stronger invariant (emit-thread-only) than the base class documents.
• Suggested fix: Either tighten the base class contract to "called only on the emit thread" (since the table builder's EstimatedTailSize() always runs on the emit/compaction thread), or make the two counters std::atomic<uint64_t> with relaxed ordering. The latter costs nothing measurable.

M2. ForwardOnKeyAddedToAll / ForwardAddIndexEntryToAll continue after setting SetStatus(Corruption) -- block_based_table_builder.cc

• Issue: When ParseInternalKey fails in ForwardOnKeyAddedToAll, the method calls SetStatus(Corruption) and returns. But the built-in builder already received the key via OnKeyAddedInternal on the line above. In ForwardAddIndexEntryToAll, the built-in builder receives AddIndexEntryDirect before the parse for custom builders runs. If the parse fails, the built-in index has an entry that the custom index does not. This divergence is documented as acceptable ("surface as Corruption to avoid divergence"), but the built-in builder has already advanced its state.
• Root cause: The parse for custom builders (user key extraction) happens after the built-in builder receives the internal key. A parse failure is unreachable on well-formed keys, but the asymmetry means error recovery leaves the two builders out of sync.
• Suggested fix: This is defensive-only code for an unreachable path. The current approach (Corruption status halts the build) is sufficient. Consider adding a brief code comment noting the built-in builder may be one entry ahead on failure.

M3. kCustomOnly + CacheDependencies calls reader_->CacheDependencies on the stub index -- user_defined_index_wrapper.h:244

• Issue: In kCustomOnly mode, the standard index reader reads the minimal stub block. IndexFactoryReaderWrapper::CacheDependencies always forwards to reader_->CacheDependencies(). For a stub index this is a no-op (the block is a few bytes), but for a partitioned index, CacheDependencies attempts to read sub-index partitions from disk. Since kCustomOnly is incompatible with partitioned indexes (validated at options time), this is currently unreachable, but the forwarding is unconditioned.
• Root cause: The wrapper doesn't check standard_index_is_stub_ before forwarding cache dependency loading.
• Suggested fix: Guard with if (!standard_index_is_stub_) for defense in depth, or add a comment documenting why it's safe.

M4. GetEffectiveIndexMode cannot distinguish explicit kStandardDefault from the default value -- block_based_table_factory.cc

• Issue: GetEffectiveIndexMode() treats index_mode == kStandardDefault as "not explicitly set" and falls through to check the legacy booleans. But kStandardDefault is the default value AND a valid explicit setting. A caller who explicitly sets index_mode = kStandardDefault alongside use_udi_as_primary_index = true would have their explicit mode overridden to kCustomDefault.
• Root cause: The function uses the default value as a sentinel for "not set," which is ambiguous.
• Suggested fix: The ConfigureOptions() override handles this correctly by erasing legacy keys when index_mode is present in the map. The constructor path (GetEffectiveIndexMode) is the only remaining ambiguity. Document that the constructor-based path prefers the legacy booleans when index_mode is at its default, and callers who want explicit kStandardDefault should not set the legacy booleans. Alternatively, clear the legacy booleans after GetEffectiveIndexMode runs.

🟢 LOW / NIT

L1. FooterBuilder::Build adds a defaulted parameter -- table/format.h:442

• Issue: incompatible_features = 0 is added as a new defaulted parameter to FooterBuilder::Build. CLAUDE.md warns "Avoid new defaulted parameters. This is the Miss Spelling in README #1 trap on refactoring!" as it allows existing callers to silently change meaning without updating call sites.
• Suggested fix: This is a low-risk instance since the default (0) preserves existing behavior exactly. All current callers that need non-zero features pass it explicitly. Note for awareness.

L2. ParsePackedValueForValue called unconditionally for kTypeValuePreferredSeqno in ForwardOnKeyAddedToAll -- block_based_table_builder.cc

• Issue: The kTypeValuePreferredSeqno case extracts the user value via ParsePackedValueForValue, which is correct. However, this parsing happens for every key of that type, even if custom_indexes is empty (the early return above guards this). The branching is fine; this is just a minor readability note.

L3. Minor: #include <stdexcept> added in block_based_table_factory.cc for std::invalid_argument catch

• Issue: The parse_legacy_bool lambda catches std::invalid_argument thrown by ParseBoolean. This is an unusual pattern in RocksDB (which prefers Status-based error handling). The catch is correct but adds a C++ exception dependency on a code path.
• Suggested fix: Consider using a non-throwing boolean parser or checking the string value directly.

L4. standard_index_is_stub is a uint64_t for a boolean property -- table_properties.h:251

• Issue: The property is semantically boolean but stored as uint64_t to match the existing table properties serialization format (all properties are uint64_t). This is consistent with udi_is_primary_index and index_key_is_user_key.

L5. TrieIndexReader::ApproximateMemoryUsage no longer includes data_size_ -- trie_index_factory.cc

• Issue: The old implementation included the raw serialized data size in ApproximateMemoryUsage. The new version reports only auxiliary allocations, since "the serialized data is owned by the table reader or block cache." This is conceptually correct but changes the reported value. The ApproximateMemoryUsage of the wrapper in IndexFactoryReaderWrapper sums reader_ + udi_reader_, so the raw data is tracked elsewhere.
• Suggested fix: This is intentional and correct -- just noting the behavioral change.

Cross-Component Analysis

Context	Executes?	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES (uses standard SST pipeline)	YES -- index building is below the transaction layer	None
ReadOnly DB / SecondaryInstance	YES (reads SSTs)	YES -- reader changes are read-only	None
CompactionService / Remote compaction	YES (writes SSTs remotely)	YES -- Rep/builders are per-SST, no shared state	None
User-defined timestamps	Blocked by validation	N/A -- ts_sz > 0 + UDI factory is rejected	Safe
Concurrent writers	YES (each SST has own Rep)	YES -- no cross-SST shared mutable state	None
FIFO / Universal compaction	YES	YES -- index building is independent of compaction strategy	None
Partitioned index + kCustomDefault	Blocked by validation	N/A -- rejected in ValidateOptions and Rep constructor	Safe
Backup/Restore (OPTIONS serialization)	YES	kStandardDefault/kStandardRequired: safe (standard index works). kCustomDefault/kCustomOnly: caller must reattach factory	Documented
Old SSTs (no UDI block) + kStandardRequired	Reader falls back to standard with warning	YES -- zero-size block check handles legacy SSTs	Safe

Positive Observations

• Clean separation of concerns: The BuiltinIndexFactoryBuilder / IndexFactoryBuilder layering is well-designed. The built-in builder gets a fast direct path (AddIndexEntryDirect, OnKeyAddedInternal) that avoids parse/repack overhead, while custom builders go through the user-key-based public interface.

• Thread safety documentation is excellent. The IndexFactoryBuilder header clearly documents which callbacks run on which thread and what synchronization is provided. The trie builder's PreparedTrieEntry design keeps emit-thread state (estimated_*) cleanly disjoint from BG-writer state (buffered_entries_).

• Parallel protocol opt-in is well-guarded. The Rep constructor probes each custom builder with SupportsParallelAddEntry() + CreatePreparedAddEntry(), and falls back to single-threaded with a named log warning if any builder declines. The MaybeStartParallelCompression function adds a second guard for builders that start returning null after the probe.

• Legacy compatibility is carefully handled. The ConfigureOptions() override correctly resolves alias priority (explicit index_mode wins), and the C API SyncIndexModeFromLegacyUdiOptions() maintains backward compatibility for C callers.

• Footer incompatible feature flag is a clean forward-compatible design. Older readers reject unknown bits, and the flag is only set for kCustomOnly which requires format_version >= 6 (where the checked bytes exist).

• Good test coverage for the parallel protocol: the trie_index_test tests serial/parallel equivalence, size estimates, invalid prepared entries, and the SkipCustomPrepare test hook exercises the stale-flag reset path.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit d7119a8

---

Summary

Major refactoring PR (Part 8 of 9) that renames UDI types to IndexFactory, replaces boolean flags with IndexMode enum, introduces BuiltinIndexFactoryBuilder, and enables parallel compression for custom indexes. The design direction is sound but there are several correctness and compatibility concerns. Note: the diff is truncated (51 files, 7013 insertions), so some findings may be addressed in unseen portions.

High-severity findings (3):

• [block_based_table_builder.cc:3525] Buffered replay path still calls r->index_builder->OnKeyAdded() directly instead of r->ForwardOnKeyAddedToAll(), skipping custom index builders.
• [block_based_table_builder.cc:2200,2381] Parallel compression paths (EmitBlockForParallel and BGWorker::write_fn) only prepare/finish the built-in index entry; custom_prepared_entries fields are declared in BlockRep but never populated or consumed in the visible diff.
• [block_based_table_builder.cc:3207,3689] p_index_builder_ is removed as a Rep field but WritePropertiesBlock and EstimatedTailSize still dereference it.

Full review (click to expand)

Findings

🔴 HIGH

H1. Buffered replay path skips custom index OnKeyAdded -- block_based_table_builder.cc:3525

• Issue: In the buffered replay path (dictionary compression mode), r->index_builder->OnKeyAdded(key, iter->value()) at line 3525 calls only the built-in index builder. The diff changes line 2034 (unbuffered path) to r->ForwardOnKeyAddedToAll(...) but the truncated diff doesn't show a corresponding change at line 3525.
• Root cause: The buffered replay path is a separate code path from the normal Add() flow. When the table builder replays buffered data blocks after dictionary finalization, it iterates all keys and calls OnKeyAdded. If custom_indexes exist, they never receive OnKeyAdded callbacks for these keys.
• Suggested fix: Change line 3525 to r->ForwardOnKeyAddedToAll(key, iter->value());. Confirm ForwardOnKeyAddedToAll accepts Slice (from iter->value()) as its second argument (it takes const std::optional<Slice>&).

H2. Parallel compression ignores custom indexes -- block_based_table_builder.cc:2200,2381

• Issue: BlockRep gains custom_prepared_entries and custom_entries_prepared fields (diff lines ~440-445), but:
  1. EmitBlockForParallel (line 2200) only calls r->index_builder->PrepareIndexEntry() for the built-in index. No corresponding PrepareAddEntry calls are made for custom builders.
  2. BGWorker::write_fn (line 2381) only calls rep_->index_builder->FinishIndexEntry(). No corresponding FinishAddEntry calls are made for custom builders.
  3. The fields custom_prepared_entries and custom_entries_prepared are never used anywhere in the visible diff.
• Root cause: The diff is truncated and these changes likely exist in the unseen portion. However, the existing EmitBlockForParallel and BGWorker functions show no custom index handling in the visible diff.
• Suggested fix: Verify that EmitBlockForParallel calls PrepareAddEntry on each custom builder and write_fn calls FinishAddEntry. If this is in the unseen diff, ignore.

H3. p_index_builder_ references after removal -- block_based_table_builder.cc:3207,3689

• Issue: The diff removes p_index_builder_ as a separate Rep field (replaced by builtin_index_builder), but existing code at lines 3207-3210 (WritePropertiesBlock) and 3689-3690 (EstimatedTailSize) still dereference rep_->p_index_builder_. These will fail to compile unless updated in the unseen diff.
• Root cause: These sites are not visible in the truncated diff and may be handled.
• Suggested fix: Access the partitioned builder through builtin_index_builder->GetPartitionedIndexBuilder() or equivalent accessor.

🟡 MEDIUM

M1. ValidateOptions still uses legacy booleans -- block_based_table_factory.cc:798-822

• Issue: BlockBasedTableFactory::ValidateOptions() still rejects parallel compression unconditionally for UDI (line 799-802) and uses use_udi_as_primary_index for validation. The PR's core feature is enabling parallel compression for custom indexes, so this validation needs updating.
• Suggested fix: Remove the blanket parallel compression rejection. Replace use_udi_as_primary_index checks with index_mode checks. The Rep constructor already handles per-builder parallel support probing correctly.

M2. ValueType scoped enum breaks existing UDI subclasses -- index_factory.h

• Issue: The old UserDefinedIndexBuilder::ValueType was unscoped; the new IndexFactoryBuilder::ValueType is enum class. Existing implementations using case kValue: in switch statements won't compile. The compatibility constants help for non-switch usage but don't fix switch-case.
• Suggested fix: Document the migration requirement. Consider keeping unscoped for backward compatibility since this is experimental API.

M3. C API SyncIndexModeFromLegacyUdiOptions can override explicit index_mode -- db/c.cc:470-480

• Issue: The sync function unconditionally writes index_mode based on the legacy booleans. If a caller sets index_mode through the options string and then calls a legacy setter, the explicit mode is silently overridden.
• Suggested fix: Document precedence rules or add a guard.

M4. options_settable_test inconsistency -- options_settable_test.cc:248

• Issue: Legacy boolean fields are removed from the options string but added to kBbtoExcluded, yet they remain registered in the options type info map. The test should either exercise them or deregister them.
• Suggested fix: Keep the assignments in the options string, or remove from the type info map.

🟢 LOW / NIT

L1. ForwardOnKeyAddedToAll defensive parse check adds hot-path instructions for unreachable condition.

L2. ReadOptions::read_index padding placement static_asserts are good but fragile -- add a layout comment to ReadOptions.

L3. EmitBlock at line 2275 still calls r->index_builder->AddIndexEntry() directly. This should become r->ForwardAddIndexEntryToAll() if the diff's intent is to forward to all builders in the serial path. Verify in unseen diff.

Cross-Component Analysis

Context	Affected?	Assessment
WritePreparedTxnDB	No	Index building is independent of transaction semantics
ReadOnly DB	No	Write-path changes only
CompactionService	Possible	IndexMode enum needs serialization (present in type info map)
User-defined timestamps	Yes	Correctly rejected in Rep constructor
Partitioned index	Yes	Correctly rejected for kCustomDefault/kCustomOnly
Concurrent writers	No	Single-SST, single-writer

Positive Observations

• Separating built-in and custom indexes into independent builders is cleaner than the old UserDefinedIndexBuilderWrapper and enables per-builder parallel compression.
• The IndexMode enum is a significant improvement over boolean flags with clear migration path documentation.
• Per-builder parallel support probing (throwaway CreatePreparedAddEntry() in constructor) is practical.
• C API backward compatibility layer preserves existing caller behavior.
• Static_asserts for enum values and struct layout are excellent defensive measures.
• The IndexFactoryOptions documentation on UDT incompatibility is clear.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit f228a2c

---

Summary

Well-structured refactoring that cleanly separates built-in and custom index building, enables parallel compression for custom indexes, and introduces the IndexMode enum for controlling index build/read routing. The type renaming (UserDefinedIndex* -> IndexFactory*) with backward-compatible shims is done correctly. The C API compatibility wrapper with SyncIndexModeFromLegacyUdiOptions is a thoughtful approach.

High-severity findings (2):

• [block_based_table_builder.cc (diff ~line 2381)] BGWorker write path still calls index_builder->FinishIndexEntry() (old IndexBuilder method) instead of FinishAddEntry() on both built-in and custom builders -- custom index entries may never be committed in parallel mode.
• [block_based_table_builder.cc:3090-3169] WriteIndexBlock still calls rep_->index_builder->Finish() via the old IndexBuilder interface. With the new architecture where custom indexes are in Rep::custom_indexes, custom index meta blocks may not be written unless WriteIndexBlock is updated (truncated diff may contain these changes).

Full review (click to expand)

Findings

🔴 HIGH

H1. BGWorker parallel write path does not commit custom index entries — block_based_table_builder.cc (diff line ~2381)

• Issue: The BGWorker write_fn lambda (line 2381 in current main) calls rep_->index_builder->FinishIndexEntry(), which is the old IndexBuilder method on the monolithic wrapper. In the new architecture, index_builder is a BuiltinIndexFactoryBuilder and custom_indexes are separate. The diff adds custom_prepared_entries and custom_entries_prepared to BlockRep, and stages them in EmitBlockForParallel, but the BGWorker write path (which must call FinishAddEntry on each custom builder) does not appear in the truncated diff. If the BGWorker does not call FinishAddEntry on each custom_indexes[i].builder, custom index entries will be silently dropped during parallel compression.
• Root cause: The diff was truncated at 5164 lines, so the BGWorker changes may exist but were not shown. However, grepping the current codebase shows no references to custom_prepared_entries, custom_entries_prepared, or index_entry_prepared anywhere except in the diff. These new BlockRep fields are staged in EmitBlockForParallel but must be consumed in a matching write path.
• Suggested fix: Verify that the full PR includes BGWorker changes to call FinishAddEntry on each custom builder with the corresponding custom_prepared_entries[i]. The write_fn should iterate custom_indexes and commit each staged entry after the built-in entry. If missing, custom indexes will be empty after parallel compression SSTs.

H2. WriteIndexBlock must be updated for separated builders — block_based_table_builder.cc:3090

• Issue: WriteIndexBlock calls rep_->index_builder->Finish(&index_blocks). Previously, when index_builder was a UserDefinedIndexBuilderWrapper, this single call wrote both the standard index AND the UDI meta block (the wrapper's Finish method added the UDI contents to index_blocks.meta_blocks). With the new architecture, index_builder is a BuiltinIndexFactoryBuilder which only writes the standard index. Custom index blocks must be written separately.
• Root cause: Architectural change from monolithic wrapper to separated builders requires updating all Finish call sites. The diff was truncated and these changes may exist but were not shown.
• Suggested fix: WriteIndexBlock (or a new companion method) must: (1) Finish the built-in builder (or write an empty stub in kCustomOnly), (2) Finish each custom builder and write its contents as a kUserDefinedIndex meta block with key kIndexFactoryMetaPrefix + name, (3) Handle the kCustomOnly stub index property. Verify the full PR addresses this.

🟡 MEDIUM

M1. WritePropertiesBlock still references p_index_builder_ — block_based_table_builder.cc:3207

• Issue: WritePropertiesBlock accesses rep_->p_index_builder_ for partition count and top-level index size. The diff removes p_index_builder_ from Rep and replaces it with builtin_index_builder (a BuiltinIndexFactoryBuilder*). The properties block code must be updated to use builtin_index_builder->NumPartitions() etc.
• Root cause: Rep member rename without updating all references.
• Suggested fix: Replace rep_->p_index_builder_ with rep_->builtin_index_builder and guard with null check for kCustomOnly mode.

M2. Null dereference in kCustomOnly for properties — block_based_table_builder.cc:3213

• Issue: rep_->index_builder->separator_is_key_plus_seq() would dereference null in kCustomOnly mode where index_builder is not constructed (or is only the custom builder). Similarly rep_->index_builder->NumUniformIndexBlocks() at line 3162.
• Root cause: kCustomOnly mode skips built-in builder construction.
• Suggested fix: Guard with null checks on builtin_index_builder and default to sensible values.

M3. EstimatedSize for SST tail may undercount custom indexes

• Issue: Estimated tail size only accounts for index_builder->CurrentIndexSizeEstimate(). Custom index sizes from custom_indexes[i].builder->EstimatedSize() should be added. The custom builder's EstimatedSize contract requires an upper bound; undercounting risks compaction assertion failures.
• Suggested fix: Add custom index sizes to the tail estimation loop.

M4. C API SyncIndexModeFromLegacyUdiOptions doesn't consider factory presence — db/c.cc:472

• Issue: Setting use_udi_as_primary_index=true via C API maps to kCustomDefault regardless of whether user_defined_index_factory is set. This will fail at table build time, not at option-setting time. The old behavior set the boolean directly, deferring validation.
• Suggested fix: Acceptable since Rep constructor validates. Consider documenting the deferred error.

M5. ReadOptions field layout static_asserts are fragile — options_settable_test.cc:28-54

• Issue: New static_asserts enforce exact offsets in ReadOptions to pack read_index into padding before table_index_factory. Will break if any preceding field changes size. Reasonable for experimental API ABI stability.
• Suggested fix: Add comment explaining these should be revisited when experimental tag is lifted.

M6. Read path not wired for IndexMode/read_index — user_defined_index_wrapper.h:1860

• Issue: The reader still dispatches based on use_udi_as_primary_index boolean, not index_mode or read_index. This means kCustomDefault vs kStandardRequired vs kStandardDefault modes are not distinguished on reads.
• Suggested fix: Verify Part 9 wires these. If not, kStandardRequired's "hard error on missing UDI block" behavior won't be different from kStandardDefault.

🟢 LOW / NIT

L1. Redundant UserDefinedIndexBuilderWrapper still present

• user_defined_index_wrapper.h contains the old UserDefinedIndexBuilderWrapper which extends IndexBuilder. Since the new architecture uses BuiltinIndexFactoryBuilder + custom_indexes, the old wrapper should be removed once fully migrated. Document removal timeline.

L2. index_factory.h copyright comment style

• Uses // (single space) vs // (two spaces) used in other headers. Minor inconsistency.

L3. Tests change from table_index_factory to read_index — db_wide_blob_direct_write_test.cc

• Tests drop the name-matching guard that table_index_factory provided. kPreferCustom would silently use any installed custom index. Intentional per design; acceptable.

L4. ForwardOnKeyAddedToAll passes empty Slice when value is nullopt

• In the custom builder forwarding, when value is nullopt, user_value is default-constructed (empty Slice). This matches the old wrapper behavior for the value parameter and is correct.

Cross-Component Analysis

Context	Impact	Assessment
WritePreparedTxnDB	Index building only	Safe
ReadOnly DB	No writes	Safe - read path unchanged
User-defined timestamps	Rejected in Rep constructor	Safe
Partitioned index	kCustomDefault/kCustomOnly rejected	Safe
kCustomOnly + old readers	format_version >= 6 guard	Safe
Parallel compression	New capability for custom indexes	Needs H1 verification
Options serialization	index_mode serializes, factory doesn't	Documented in API

Positive Observations

• Clean separation of built-in and custom index builders improves maintainability and enables independent evolution.
• The SupportsParallelAddEntry / CreatePreparedAddEntry probe at construction time elegantly handles builders that don't support parallel compression.
• Backward compatibility is thorough: user_defined_index.h type aliases, C API wrapper with SyncIndexModeFromLegacyUdiOptions, and OPTIONS file parse aliases.
• static_asserts for enum values and struct layout provide compile-time safety nets.
• Good test coverage for BuiltinIndexFactoryBuilder covering each index type, error cases, and the parallel entry protocol.
• IndexMode enum design with documented migration path (kStandardDefault -> kStandardRequired -> kCustomDefault -> kCustomOnly) is well designed.
• The diff correctly validates ts_sz > 0 rejection in the Rep constructor, closing a gap where SstFileWriter and SetOptions could bypass BlockBasedTableFactory::ValidateOptions.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit fae9998

---

Summary

This PR (Part 8 of 9) makes the trie index builder support parallel compression by implementing the SupportsParallelAddEntry / PrepareAddEntry / FinishAddEntry protocol from IndexFactoryBuilder. The diff against main also includes changes from predecessor PRs #14962 and #14965 which refactored the table builder to separate built-in and custom index builders, introduced the IndexMode enum, and wired IndexFactory through block-based tables. The parallel trie implementation is well-designed with good test coverage. A few issues around C API validation, the static_cast usage in C API setters, and a potential estimated_separator_bytes_ overflow concern are noted.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. C API static_cast without validation in rocksdb_block_based_options_set_index_mode and rocksdb_readoptions_set_read_index -- db/c.cc

• Issue: Both new C API setters cast user-supplied int values directly to C++ enums via static_cast without any range validation. An out-of-range integer produces undefined behavior per C++ standard for scoped enums, and even if the compiler treats it as the underlying integral type, downstream code (switch statements, comparisons) will not handle unknown values correctly.
• Root cause: CLAUDE.md explicitly discourages static_cast; static_cast_with_check is preferred for internal use, though the C API boundary is inherently untyped. The IndexMode enum has values 0-4 and ReadIndex has 0-2.
• Suggested fix: Add bounds validation before the cast:

  if (v < 0 || v > 4) { return; } // or set an error

  Alternatively, document the valid range in the C header comment.

M2. estimated_separator_bytes_ and estimated_num_entries_ threading assumption is implicit -- utilities/trie_index/trie_index_factory.cc

• Issue: PrepareAddEntry (emit thread) mutates estimated_separator_bytes_ and estimated_num_entries_. EstimatedSize() reads them. Both are called from the emit thread (which is also the Add() caller thread), so this is currently safe. However, the invariant that EstimatedSize() is only called from the same thread as PrepareAddEntry is not enforced and could break if future callers invoke EstimatedSize() from a different thread.
• Suggested fix: Document the threading assumption more explicitly in the header, or make the counters RelaxedAtomic<uint64_t> for safety at negligible cost (these are updated once per data block, not per key).

M3. FinishAddEntry silently skips invalid prepared entries -- utilities/trie_index/trie_index_factory.cc

• Issue: When PrepareAddEntry fails (e.g., upstream ParseInternalKey failure sets p->valid = false), FinishAddEntry silently skips the entry. The trie index will be missing that data block's entry. In kStandardDefault mode this is tolerable (standard index is the fallback), but in kCustomDefault or kCustomOnly mode, this would make data unreachable through the trie.
• Root cause: The table builder already sets a Corruption status on the parse failure upstream, so the SST will be rejected. But the trie builder should also fail its own Finish() for defense in depth.
• Suggested fix: Track a count of skipped entries and have Finish() return an error if any entries were skipped.

M4. SyncIndexModeFromLegacyUdiOptions and GetEffectiveIndexMode are redundant precedence mechanisms -- db/c.cc, table/block_based/block_based_table_factory.cc

• Issue: The C API struct has its own SyncIndexModeFromLegacyUdiOptions() which directly writes rep.index_mode. The C++ BlockBasedTableFactory constructor has GetEffectiveIndexMode() which also resolves deprecated bools to index_mode. Both apply independently. The current design works correctly but the indirection is fragile -- the C API bools are local (not in rep), so GetEffectiveIndexMode() can't see them, but that's fine because SyncIndexModeFromLegacyUdiOptions already wrote rep.index_mode.
• Suggested fix: Document this interaction clearly. Not a bug, but a maintenance concern.

🟢 LOW / NIT

L1. static_cast used instead of static_cast_with_check -- db/c.cc

• Issue: CLAUDE.md says "Avoid static_cast; static_cast_with_check from cast_util.h is preferred." The C API setters for index_mode and read_index use bare static_cast.
• Suggested fix: Use static_cast_with_check or manual validation.

L2. #include <stdexcept> added but not used -- table/block_based/block_based_table_factory.cc

• Issue: The diff adds #include <stdexcept> but no exception-related code is visible. RocksDB does not use exceptions.
• Suggested fix: Remove the include if unused.

L3. Ring buffer flag reset pattern is correct -- table/block_based/block_based_table_builder.cc

• Issue: index_entry_prepared and custom_entries_prepared are reset to false at the top of EmitBlockForParallel before conditional staging. This correctly handles ring buffer slot reuse -- a stale true flag from a previous iteration would cause BGWorker to commit stale data.
• Suggested fix: No change needed. Positive observation.

L4. ForwardOnKeyAddedToAll per-key parse overhead -- table/block_based/block_based_table_builder.cc

• Issue: Adds ParseInternalKey per key when custom_indexes is non-empty. The previous UserDefinedIndexBuilderWrapper::OnKeyAdded also parsed per key, so no regression. Short-circuits when custom_indexes.empty().
• Suggested fix: No change needed.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES (table building)	YES (index building independent of txn visibility)	Safe
ReadOnly DB	NO (no table building)	N/A	Safe
SstFileWriter	YES (uses BlockBasedTableBuilder)	YES (same validation)	Safe
CompactionService	YES	index_mode serializes via OPTIONS string map	Safe
Backup/Restore	YES	factory shared_ptr lost; documented in table.h	Documented
User-defined timestamps	YES	ts_sz > 0 rejected with custom indexes	Validated
Concurrent writers	YES	OnKeyAdded on emit thread only	Safe

Meta block key compatibility verified: kIndexFactoryMetaPrefix == kUserDefinedIndexPrefix == "rocksdb.user_defined_index." (asserted in tests). Reader and writer use the same prefix.

The incompatible_features footer mechanism and kFooterFeatureRequireUserDefinedIndex are from predecessor PR a57af39e6. This PR correctly passes the feature flag only for kCustomOnly mode.

Positive Observations

1. Well-structured parallel protocol: The PreparedTrieEntry cleanly separates emit-thread state from writer-thread state. The valid flag reset after FinishAddEntry correctly handles ring buffer reuse.

2. Good test coverage: Five new unit tests (SupportsParallelAddEntry, ParallelMatchesSerialOutput, ParallelPipelineOverlapsPrepareAndFinish, ParallelEstimatedSizeStaysMonotonic, ParallelInvalidPreparedEntrySkipped) plus one integration test (ParallelCompressionWithHashStandardIndexAndTrieUdi) that exercises hash index + trie + 4 parallel threads.

3. Correct same-user-key detection: Two-phase detection (initial from inputs in Prepare, buffer-state recheck in Finish) correctly mirrors serial AddIndexEntry including the edge case where FindShortestSeparator fails to shorten.

4. Disjoint estimation counters: Moving size estimation out of buffered_entries_ eliminates the data race that would occur if EstimatedSize() read vector size concurrently with FinishAddEntry mutations.

5. Graceful degradation: Rep constructor probes each builder, falls back to serial with a logged warning if any declines parallel support.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit ad516ed

---

Summary

Substantial refactoring of block-based table builder index construction to decouple built-in and custom indexes, enabling parallel compression for custom index implementations. The design is sound: separate index_builder (built-in) + custom_indexes vector with forwarding helpers replaces the monolithic UserDefinedIndexBuilderWrapper. The index_mode enum provides a clean migration path. The parallel protocol extension (PrepareAddEntry/FinishAddEntry for custom builders) is well-structured.

High-severity findings (2):

• [db/c.cc:4748,4761] C API static_cast of unchecked int to IndexMode/ReadIndex enums -- out-of-range values produce undefined behavior.
• [block_based_table_factory.cc ValidateOptions] Timestamp incompatibility check rejects kStandardDefault + ts_sz > 0 + factory, which is overly conservative -- in that mode the custom index is secondary and reads use the standard index.

Full review (click to expand)

Findings

🔴 HIGH

H1. C API enum casts without range validation -- db/c.cc:4748,4761

• Issue: rocksdb_block_based_options_set_index_mode and rocksdb_readoptions_set_read_index use static_cast<BlockBasedTableOptions::IndexMode>(v) / static_cast<ReadOptions::ReadIndex>(v) on an unchecked int from C callers. An out-of-range value silently produces an invalid enum, which can propagate through the system undetected.
• Root cause: The C API takes int for extensibility but performs no bounds check before the cast.
• Suggested fix: Validate v against the known enum range before casting, returning early or asserting on invalid values. For example:

  if (v < 0 || v > static_cast<int>(BlockBasedTableOptions::IndexMode::kStandardRequired)) {
    return;
  }

  CLAUDE.md also discourages bare static_cast; prefer a pattern that validates.

H2. Timestamp incompatibility check is overly broad -- block_based_table_factory.cc ValidateOptions

• Issue: The check index_mode != kStandardOnly && user_defined_index_factory != nullptr && ts_sz > 0 rejects ALL non-kStandardOnly modes with timestamps. But kStandardDefault and kStandardRequired build the custom index as a secondary -- reads default to the standard index. Rejecting these modes prevents users from deploying a custom index alongside timestamps even in secondary/advisory mode.
• Root cause: The condition is index_mode != kStandardOnly rather than targeting only modes where the custom index serves reads (kCustomDefault, kCustomOnly).
• Suggested fix: Narrow the rejection to (index_mode == kCustomDefault || index_mode == kCustomOnly) so secondary custom indexes can coexist with timestamps. The table builder's duplicated check already uses builds_custom_index which is equally broad -- update both consistently.

🟡 MEDIUM

M1. Reader-side routing not updated for explicit index_mode -- block_based_table_reader.cc:1788-1838

• Issue: The reader still checks table_options.use_udi_as_primary_index and table_options.fail_if_no_udi_on_open directly. When index_mode is set explicitly via the new API, UpdateIndexMode() clears these bools to false. This means explicit index_mode = kCustomDefault via the new API will NOT route reads through the custom index, and explicit kStandardRequired will not enforce the missing-UDI error.
• Root cause: This PR updates the write path but defers reader updates (presumably to PR Add IndexFactory stress and benchmark flags #14968 in the series).
• Suggested fix: Document this limitation clearly or include reader updates in this PR. If deferred, ensure the EXPERIMENTAL marker on index_mode is prominent enough. Currently the only safe way to get kCustomDefault reads is through the legacy use_udi_as_primary_index = true path.

M2. ForwardOnKeyAddedToAll silently forwards empty value when optional is absent -- block_based_table_builder.cc Rep

• Issue: When value.has_value() is false, user_value is default-constructed (empty Slice) and custom builders receive OnKeyAdded(key, vt, Slice()). The old UserDefinedIndexBuilderWrapper::OnKeyAdded treated absent value as an error (assert + InvalidArgument status). The new code silently forwards an empty value that could corrupt custom index state.
• Root cause: Regression in defensiveness during the refactor.
• Suggested fix: Match the old wrapper's defensiveness: when value is absent, set an error status rather than forwarding an empty value.

M3. ConfigureOptions rollback is field-level, not transactional -- block_based_table_factory.cc

• Issue: ConfigureOptions saves five fields before calling TableFactory::ConfigureOptions. On failure, it restores them. But intermediate ParseOption calls invoke UpdateIndexMode() which modifies the same fields being saved. If a later option fails, the rollback restores the pre-call state from the saved copies -- which appears correct since all five modified fields are saved. However, the correctness is subtle and fragile.
• Root cause: UpdateIndexMode() side effects interleave with field saves.
• Suggested fix: Add a test that exercises partial-failure rollback to lock down this behavior.

M4. EstimatedSize() thread safety for custom builders undocumented at call site

• Issue: EstimatedTailSize() calls ci.builder->EstimatedSize() which may race with FinishAddEntry on the BG writer thread. The contract is documented in index_factory.h but not at the call site.
• Suggested fix: Add a brief comment at the call site noting the concurrency requirement, or consider TSAN annotations.

🟢 LOW / NIT

L1. custom_prepared_entries vector allocation per ring buffer slot

• Issue: Each ring buffer slot creates a vector<unique_ptr>. For the common case (one custom builder), this is a size-1 vector with a heap allocation. Runs once per SST, not per block.
• Suggested fix: Minor -- could use inline storage but negligible impact.

L2. Test coverage for kCustomOnly stub index (diff truncated)

• Issue: Cannot verify full test coverage for: reading an SST with a stub standard index, error behavior when kBuiltin read_index targets a kCustomOnly SST, and round-trip correctness.
• Suggested fix: Verify these scenarios exist in the truncated test files.

L3. IndexBlockWriterImpl comment slightly misleading

• Issue: Comment says "Defined out-of-line as a private nested class" but it's defined in the .cc file, which is normal.
• Suggested fix: Minor clarification or removal.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES (compaction/flush)	YES - index building is value-type-aware	Safe
ReadOnly DB	NO (no table building)	N/A	Safe
User-defined timestamps	YES - rejected at validation	YES but overly broad (H2)	Narrow check
CompactionService	YES (remote compaction)	PARTIALLY - IndexFactory survives only via registry	Document
FIFO / Universal compaction	YES	YES	Safe
Concurrent writers	YES (parallel compression)	YES - PrepareAddEntry on emit, FinishAddEntry on writer	Safe
Partitioned index + kCustomDefault	NO - rejected at validation	N/A	Safe
Old binaries reading new SSTs	kCustomOnly: DEPENDS on format_version >= 6 footer flag	YES with dependent PR #14965	Verify #14965
Snapshots	YES	YES - index building is snapshot-agnostic	Safe

Positive Observations

• Clean separation of concerns: The split from UserDefinedIndexBuilderWrapper to separate index_builder + custom_indexes vector is a good architectural improvement. Each builder has its own scratch buffer, avoiding contention.
• Defensive ring buffer flag resets: index_entry_prepared = false and custom_entries_prepared = false at the top of EmitBlockForParallel prevents stale data from previous ring buffer iterations.
• Graceful degradation: When any builder doesn't support parallel compression, the system falls back to single-threaded with a descriptive log message naming the declining builder.
• C API backwards compatibility: Legacy bools are preserved in the C wrapper struct with explicit precedence rules (index_mode_explicit flag), and c_test.c thoroughly exercises the precedence logic including the "explicit wins" scenario.
• Static asserts in options_settable_test.cc: Enum numeric stability and ReadOptions layout invariants are checked at compile time, catching ABI breaks.
• Parallel probe at construction time: The Rep constructor probes CreatePreparedAddEntry() for each custom builder and falls back immediately if it returns null, preventing a null dereference on the emit thread later.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase