Promote IndexFactory as UDI SPI

[zaidoon1]

Part 6 of 13 in the UDI split.

Stack order:

1. Add IndexFactory compatibility names #14954 Add IndexFactory compatibility names
2. Add UDI index mode API vocabulary #14959 Add UDI index mode API vocabulary
3. Add optional UDI builder protocols #14960 Add optional UDI builder protocols
4. Add built-in index factory wrappers #14961 Add built-in index factory wrappers
5. Add built-in index factory tests #14969 Add built-in index factory tests
6. Promote IndexFactory as UDI SPI #14962 Promote IndexFactory as UDI SPI
7. Wire IndexFactory through block-based tables #14963 Wire IndexFactory through block-based tables
8. Add IndexFactory table routing tests #14964 Add IndexFactory table routing tests
9. Add IndexFactory parallel routing tests #14970 Add IndexFactory parallel routing tests
10. Account UDI blocks as index blocks #14965 Account UDI blocks as index blocks
11. Support parallel AddIndexEntry in trie index #14966 Support parallel AddIndexEntry in trie index
12. Cover trie index modes #14967 Cover trie index modes
13. Add IndexFactory stress and benchmark flags #14968 Add IndexFactory stress and benchmark flags

Previous: #14969.
Next: #14963.

Depends on #14969. Until the earlier UDI PRs land, GitHub shows the cumulative diff against main. After those land, the intended review diff is:

zaidoon/udi-04-builtin-index-factory-tests..zaidoon/udi-05-promote-index-factory-spi

That final review delta is 11 files with 468 insertions and 432 deletions.

What changed:

• Promote IndexFactory into the public UDI SPI surface.
• Keep user_defined_index.h as a compatibility include path.
• Preserve the old UDI compatibility names and adapter methods.
• Keep concrete factories compatible with the old NewBuilder() and NewReader(Slice&) overloads.

Validation on the cumulative stack:

• git diff --check
• make check-sources
• python3 tools/c_api_gen/verify_generated_up_to_date.py
• AUTO_CLEAN=1 make -j14 builtin_index_factory_test table_test trie_index_test c_test options_settable_test
• AUTO_CLEAN=1 make -j14 builtin_index_factory_test table_test trie_index_test c_test options_settable_test block_fetcher_test partitioned_filter_block_test db_properties_test db_wide_blob_direct_write_test trie_index_db_test db_bench db_stress db_bench_tool_test
• ./builtin_index_factory_test
• ./table_test --gtest_filter='*UserDefinedIndex*:*IndexFactory*'
• ./trie_index_test --gtest_filter='TrieIndexFactoryTest.*:TrieIndexSSTTest.*'
• ./c_test

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 443.7s.

[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 a2d5d2d

---

Summary

Well-structured refactoring that promotes IndexFactory as the public UDI SPI with good backward compatibility via type aliases. The design mirrors FilterPolicy (factory/builder/reader split) and the asymmetric build/read approach is well-justified. The PR is appropriately scoped for one step in a 13-PR stack.

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

Full review (click to expand)

Findings

🟡 MEDIUM

M1. PartitionCoordinator type does not exist on main — builtin_index_factory.h

• Issue: The diff introduces BuiltinIndexFactoryBuilder::GetPartitionCoordinator() which returns PartitionCoordinator*, and builtin_index_factory_test.cc tests it. However, PartitionCoordinator does not exist anywhere in the current codebase. This type must be introduced by an earlier PR in the stack (likely Add built-in index factory wrappers #14961 "Add built-in index factory wrappers").
• Root cause: Stacked PR depends on prerequisite changes not yet merged.
• Impact: The PR cannot compile against main without prerequisites. Expected for a stacked PR workflow.
• Suggested fix: No fix needed if prerequisites merge first.

M2. FinishAndWrite meta block compress flag may be incorrect — builtin_index_factory.cc

• Issue: In BuiltinIndexFactoryBuilder::FinishAndWrite, auxiliary meta blocks (hash index prefix blocks) are written with the same compress flag as the main index block. In the existing code path, hash index prefix/metadata meta blocks may be handled differently (they go through WriteRawBlock as meta blocks).
• Root cause: The IndexBlockWriter abstraction uses a single compress bool, which may not match existing behavior for all block types.
• Suggested fix: Verify compress handling matches BlockBasedTableBuilder::WriteIndexBlock when the wiring PR (Wire IndexFactory through block-based tables #14963) lands.

M3. static const BlockBasedTableOptions in standalone paths — builtin_index_factory.cc

• Issue: Standalone/test paths use static const BlockBasedTableOptions kDefaultBinarySearchOpts; etc. These won't pick up future default changes.
• Suggested fix: Acceptable for test paths. The has_config_ path handles production builds correctly.

🟢 LOW / NIT

L1. const_cast in separator_is_key_plus_seq() — builtin_index_factory.cc

• Issue: Safe but ugly. All implementations use RelaxedAtomic loads.
• Suggested fix: Consider making base class method const in a separate cleanup.

L4. IndexMode default is kStandardDefault — table.h

• Issue: Default could change behavior if accidentally consulted. Current behavior without index_mode is effectively kStandardOnly.
• Suggested fix: Consider kStandardOnly as default before the wiring PR activates it.

L6. Test helper deduplication opportunity — builtin_index_factory_test.cc

• Issue: Several tests repeat factory creation + NewBuilder + add entries + Finish pattern.
• Suggested fix: Consider parameterized test fixture per CLAUDE.md dedup guidelines.

Cross-Component Analysis

Context	Assumptions hold?	Action needed?
Partitioned index	Yes (IsPartitioned checks table_opts->index_type)	None
Parallel compression	Yes (SupportsParallelAddEntry=true)	None
User-defined timestamps	Yes (ts_sz passed through config)	None
TrieIndexFactory subclass	Yes (aliases + compat constants)	None
table_test TestUDI subclass	Yes (aliases work for all usage patterns)	None

Verified claims:

• Type aliases preserve source compatibility: Confirmed. No dynamic_cast<UserDefinedIndexFactory*> in codebase. Scoped enum + static constexpr constants handle ValueType change. All 15+ usage sites of UserDefinedIndexBuilder::kValue etc. work through aliases.
• On-disk format unchanged: Confirmed. Both constants equal "rocksdb.user_defined_index.". Test pins with hardcoded literal.
• IndexMode/ReadIndex not consulted: Confirmed. No code reads these fields outside options_settable_test.

Positive Observations

1. On-disk format pinning test using hardcoded literals (not constants) catches accidental changes
2. Direct path methods (AddIndexEntryDirect, PrepareAddEntryDirect) bypass user-key repack overhead on hot path
3. Stack-local factory objects in NewBuiltinIndexFactoryBuilder avoid shared_ptr heap allocation
4. Build system completeness: All three systems (Make, CMake, BUCK) properly updated
5. Well-documented asymmetric API explaining why standard index reads bypass IndexFactory

---

ℹ️ 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

Auto-triggered after CI passed — reviewing commit 9990bd8

---

Summary

This PR promotes IndexFactory as the public UDI SPI, replacing the old UserDefinedIndexFactory hierarchy with renamed types and a backward-compat shim. It introduces built-in index factory wrappers and a BuiltinIndexFactoryBuilder adapter. The design is sound and well-structured. The new code is not yet wired into table builder/reader (deferred to PR #14963), limiting blast radius.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. Scoped enum ValueType may break external plugins — index_factory.h:119

• Issue: The old UserDefinedIndexBuilder::ValueType was an unscoped enum. The new IndexFactoryBuilder::ValueType is enum class. While static constexpr compatibility names are provided for qualified access (IndexFactoryBuilder::kValue), external plugins that relied on implicit integer conversion (int x = builder->kValue;) will break.
• Suggested fix: Acceptable for an EXPERIMENTAL API. The PR description documents this clearly.

M2. PartitionCoordinator type used but not defined in this PR — builtin_index_factory.h:243

• Issue: GetPartitionCoordinator() returns PartitionCoordinator* which doesn't exist in the current codebase. Presumably from an earlier PR in the 13-PR stack.
• Suggested fix: Expected for stacked PRs. Ensure dependency chain is correct.

M3. static const BlockBasedTableOptions in function scope — builtin_index_factory.cc

• Issue: Standalone/test paths use function-local static BlockBasedTableOptions with raw pointer stored in builder. If builder outlives the static (e.g., stored globally), this is use-after-free. Safe for current test-only usage but a latent footgun.
• Suggested fix: Document the lifetime constraint or use file-scope statics.

M4. ReadOptions::ReadIndex added but not yet wired — options.h:2440

• Issue: Preparatory enum without consumers. Acceptable since ReadOptions isn't persisted to OPTIONS files.

🟢 LOW / NIT

L1. const_cast in separator_is_key_plus_seq() — builtin_index_factory.cc

• Safe today (RelaxedAtomic loads) but fragile if future IndexBuilder subclasses add mutation. Consider making the base method const.

Positive Observations

1. Excellent backward compatibility — shim preserves all type names, Type() returns "UserDefinedIndexFactory" for Customizable compat
2. On-disk format pinned — dedicated test prevents accidental meta prefix changes
3. Thorough tests — internal key reconstruction equivalence, FinishAndWrite error propagation, partition coordinator availability
4. All 4 build systems updated (Make, CMake, src.mk, BUCK)
5. Smart stack-local factory pattern in NewBuiltinIndexFactoryBuilder avoids shared_ptr overhead

---

ℹ️ 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