Add UDI index mode API vocabulary

[zaidoon1]

Part 2 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: #14954.
Next: #14960.

Depends on #14954. Until #14954 lands, GitHub shows the cumulative diff against main. After that lands, the intended review diff is:

zaidoon/udi-01-index-factory-compat-shim..zaidoon/udi-02-index-mode-api

That final review delta is 4 files with 52 insertions.

What changed:

• Add inert IndexMode and ReadIndex vocabulary to the public options headers.
• Block those experimental enum fields from automatic C API generation until the explicit C API mapping lands later in the stack.
• Keep behavior unchanged. Runtime wiring comes in later PRs.

Validation:

• git diff --check
• make check-sources
• python3 tools/c_api_gen/verify_generated_up_to_date.py
• AUTO_CLEAN=1 make check-c-api-gen
• ./c_test

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 390.8s.

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

---

Summary

Clean, well-scoped preparatory PR that adds inert API vocabulary for the UDI index mode refactor. No runtime behavior changes — verified by codebase-wide grep. Two medium-severity and several low-severity findings.

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

Full review (click to expand)

Findings

🟡 MEDIUM

M1. IndexMode missing explicit underlying type — table.h:~603

• Issue: All existing enum class types in BlockBasedTableOptions use : char as the underlying type (IndexShorteningMode : char, PrepopulateBlockCache : char). The new IndexMode omits this, defaulting to int (4 bytes). This increases BlockBasedTableOptions struct size by 4 bytes instead of 1, wastes 3 bytes of padding, and is inconsistent with the established BBTO pattern.
• Root cause: Oversight in following BBTO conventions.
• Suggested fix: Change enum class IndexMode { to enum class IndexMode : char { to match sibling enums.

M2. Default value kStandardDefault (1) instead of kStandardOnly (0) — table.h:~629

• Issue: index_mode defaults to IndexMode::kStandardDefault (value 1), which means "build both indexes, read standard by default." However, the existing default behavior when no UDI factory is set is "standard index only" — matching kStandardOnly (value 0). When later PRs wire this up, users who never set index_mode explicitly will get kStandardDefault behavior, which implies building both indexes even when no custom index factory is configured. This could be confusing or require extra guard logic.
• Root cause: Design choice that may not align with the existing default behavior of use_udi_as_primary_index=false + no user_defined_index_factory.
• Suggested fix: Consider whether kStandardOnly (0) is the more appropriate default to match current behavior when no UDI factory is present. If kStandardDefault is intentional (e.g., because later PRs will always have a factory configured when index_mode is consulted), document this reasoning.

🟢 LOW / NIT

L1. ReadIndex uses uint8_t, IndexMode uses implicit int — options.h:~2441, table.h:~603

• Issue: ReadIndex : uint8_t is explicit about its size. IndexMode is not. This inconsistency between the two new enums introduced in the same PR is jarring.
• Suggested fix: Make both consistent. Either both use : char/: uint8_t or explain the difference.

L2. Release note doesn't mention ReadIndex or IndexMode — unreleased_history/public_api_changes/index_factory_aliases.md

• Issue: The release note says "Add IndexFactory compatibility names for the existing user-defined index API" but this PR also adds ReadOptions::ReadIndex enum and BlockBasedTableOptions::IndexMode enum, which are new public API additions beyond just compatibility names.
• Suggested fix: Update the release note to mention the new enum types, e.g.: "Add IndexFactory compatibility names and experimental IndexMode/ReadIndex enums for the user-defined index API."

L3. using IndexFactory::NewBuilder; using IndexFactory::NewReader; in test — table_test.cc:~126-127

• Issue: These using declarations in LegacyOnlyFactory are needed to unhide the base class's 2-argument overloads of NewBuilder and NewReader (which are hidden by the 0/1-argument overrides). This is correct but non-obvious — a brief comment would help future readers understand why they're needed.
• Suggested fix: Nit — add a comment like // Unhide base class overloads hidden by our overrides.

L4. Test uses TEST() while surrounding tests use TEST_F() — table_test.cc:~98

• Issue: TEST(IndexFactoryCompatibilityTest, ...) uses TEST() (no fixture) while most tests in this file use TEST_F() with fixtures like BlockBasedTableTest. This is fine for a standalone compatibility test, but worth noting the pattern difference.
• Suggested fix: None needed — TEST() is appropriate for this isolated test.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
All runtime contexts	NO	N/A	None — fields are inert
Options serialization (BBTO)	YES — index_mode is serializable	YES — string map is complete	None
Options serialization (ReadOptions)	NO — read_index not registered	N/A	Intentional — ReadOptions doesn't use OptionTypeInfo
C API auto-generation	Blocked	YES — both fields blocklisted	None
Options settable test	YES — index_mode added	YES — test string includes it	None

Inertness verification:

• Grep for .index_mode and .read_index (as ReadOptions field) across the codebase returned zero matches outside the diff.
• No memcmp or byte-level comparison of ReadOptions or BlockBasedTableOptions exists.
• The new fields are compile-time additions with no runtime readers.

String map completeness:

• block_base_table_index_mode_string_map contains all 5 IndexMode values: kStandardOnly, kStandardDefault, kCustomDefault, kCustomOnly, kStandardRequired. Complete.

Naming consistency:

• Map name block_base_table_index_mode_string_map follows the existing block_base_table_ prefix pattern. Consistent.

Positive Observations

• Clean separation of API vocabulary from runtime wiring — good incremental approach.
• Proper C API blocklist entries prevent accidental auto-generation of experimental bindings.
• The static_assert checks for type alias correctness are a nice compile-time safety net.
• Options settable test updated correctly for the new BBTO field.
• Comments clearly document the experimental/preparatory nature of the new fields.

---

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

---

Summary

Clean, well-scoped API vocabulary PR. The inert enum additions and type aliases introduce no runtime behavioral changes. No high-severity issues found.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. IndexMode missing underlying type specifier -- include/rocksdb/table.h

• Issue: IndexMode is declared as enum class IndexMode with no underlying type, defaulting to int (4 bytes). Other scoped enums in BlockBasedTableOptions use : char (e.g., IndexShorteningMode : char at table.h:820, PrepopulateBlockCache : char at table.h:876). This wastes 3 bytes in the struct and deviates from the established pattern.
• Root cause: Omitted underlying type specifier.
• Suggested fix: Change to enum class IndexMode : char { ... }; for consistency and to minimize struct padding.

M2. const qualifier inconsistency on the string map -- table/block_based/block_based_table_factory.cc

• Issue: The new block_base_table_index_mode_string_map is declared static const std::unordered_map<...>, while all existing sibling maps (e.g., block_base_table_index_type_string_map, block_base_table_index_shortening_mode_string_map, pinning_tier_type_string_map) are declared static std::unordered_map<...> (non-const). While const is arguably better practice, it breaks the file's local convention.
• Root cause: Style inconsistency.
• Suggested fix: Either drop const to match existing maps, or (preferably as a separate PR) add const to all existing maps.

🟢 LOW / NIT

L1. Naming mismatch: IndexFactoryOptions vs UserDefinedIndexOption -- include/rocksdb/index_factory.h

• Issue: The alias using IndexFactoryOptions = UserDefinedIndexOption; maps a plural name ("Options") to a singular name ("Option"). This may cause confusion about whether IndexFactoryOptions refers to a single options struct or a collection.
• Suggested fix: Consider either renaming the alias to IndexFactoryOption (singular) or renaming the source type to UserDefinedIndexOptions (plural) in a future cleanup PR.

L2. ReadIndex enum not registered in options framework -- include/rocksdb/options.h

• Issue: IndexMode is registered in the options string map (serializable/deserializable), but ReadIndex has no corresponding registration. This is intentional since ReadOptions is not serializable in the same way as BBTO. Noting for documentation purposes.
• Suggested fix: None needed now.

L3. Test only validates type aliases, not the new enums -- table/table_test.cc

• Issue: The IndexFactoryCompatibilityTest test validates the IndexFactory* type aliases and kIndexFactoryMetaPrefix, but does not exercise the new IndexMode or ReadIndex enums. ReadIndex has no test coverage at all.
• Suggested fix: Acceptable since enums are inert and will be tested when wiring lands in later PRs.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
Options serialization (BBTO)	YES - IndexMode is registered	YES - round-trip works	None
Options serialization (ReadOptions)	NO - ReadIndex not registered	N/A	None (intentional)
JNI/Java bindings	NO - no Java mapping for IndexMode	YES	None until wiring lands
C API auto-generation	Blocked by blocklist entries	YES	None
options_settable_test	YES - IndexMode covered	YES	None
Zero-initialized structs	Possible concern	IndexMode default is kStandardDefault (1), not kStandardOnly (0). If struct is zero-initialized, index_mode would be kStandardOnly. Since the field is inert, no runtime effect yet.	Document when wiring lands

Positive Observations

1. Good scope discipline: The PR strictly adds vocabulary without wiring, making it easy to review independently.
2. Proper C API blocklist entries: Both new enum fields are correctly blocked from auto-generation with clear reasons.
3. options_settable_test updated: The byte-counting test is updated to cover the new index_mode field, preventing silent regressions.
4. ReadIndex uses uint8_t underlying type: Good for minimizing ReadOptions struct size (ReadOptions is copied frequently in hot paths).
5. Scoped enums (enum class): Consistent with CLAUDE.md guidance preferring scoped enums.
6. Clear EXPERIMENTAL documentation: Both enums are documented as experimental with notes about future wiring.

---

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