Cover trie index modes

[zaidoon1]

Superseded by #14963 and #14966.

The index mode matrix, custom-only downgrade, and prefetch coverage are now included with the table routing changes in #14963. The two parallel compression cases are included with the trie parallel builder in #14966 so those tests do not precede the implementation. This test-only branch is no longer part of the UDI stack.

[github-actions]

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

Completed in 997.9s.

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:5929: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 cbc3131

---

Summary

This is Part 12 of 13 in the UDI (User Defined Index) split. The cumulative diff is large (50 files, ~6000 insertions), but the PR-specific delta is 1 file (trie_index_db_test.cc) with ~707 insertions and ~280 deletions. The cumulative diff introduces the IndexFactory API refactoring (renaming UserDefinedIndexFactory to IndexFactory, adding IndexMode enum, ReadIndex enum, BuiltinIndexFactoryBuilder, parallel compression support for custom indexes, and standard_index_is_stub table property). The test file adds comprehensive DB-level coverage for trie index mode combinations. Overall this is a well-structured change with thorough test coverage.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. C API SyncIndexModeFromLegacyUdiOptions priority inversion -- db/c.cc:468

The SyncIndexModeFromLegacyUdiOptions() method checks use_udi_as_primary_index before fail_if_no_udi_on_open. If a caller sets both flags true then clears use_udi_as_primary_index, index_mode becomes kStandardRequired because fail_if_no_udi_on_open is still true. This is correct behavior but the C API test at db/c_test.c:3423 only tests each flag independently.

• Suggested fix: Add a C API test that sets both flags, then clears one, to verify the interaction.

M2. Test parameterization covers only secondary/primary but not kCustomOnly or kStandardRequired -- trie_index_db_test.cc:4727

The test is parameterized with ::testing::Bool() (secondary vs primary mode), corresponding to kStandardDefault and kCustomDefault. The new IndexMode enum adds kCustomOnly and kStandardRequired, but these are not exercised by the DB-level tests despite the PR title "Cover trie index modes."

• Suggested fix: Add parameterization or dedicated test cases for kCustomOnly and kStandardRequired modes, or add a TODO comment if planned for Part 13.

M3. OpenDBImpl and TrieIndexReadOptions() still use legacy APIs -- trie_index_db_test.cc:188,206

The test fixture uses table_options.use_udi_as_primary_index and ro.table_index_factory rather than the new IndexMode/ReadIndex enums directly. This tests the legacy->enum translation but not the new API paths. The db_wide_blob_direct_write_test.cc changes show migration to read_index = kPreferCustom, but the main test file doesn't follow suit.

• Suggested fix: Add test paths using the new API directly (index_mode = kCustomDefault, read_index = kPreferCustom).

🟢 LOW / NIT

L1. CF handle cleanup uses delete instead of DestroyColumnFamilyHandle -- trie_index_db_test.cc:4583

In MultiCFCoalescingIterator, delete cf1; delete cf2; should be db_->DestroyColumnFamilyHandle(cf1/cf2) to match the pattern in MultipleColumnFamilies at line 1999.

L2. IndexMode enum value ordering -- include/rocksdb/table.h

kStandardRequired = 4 placed after kCustomOnly = 3 with a backward-compatibility comment. Logically kStandardRequired sits between kStandardDefault and kCustomDefault in the migration path but numerically comes last, which could confuse users comparing enum values.

Positive Observations

• Comprehensive test coverage across flush, compaction, WAL replay, multi-CF, ingestion, prefix iteration, direction switching, same-user-key blocks, DeleteRange, MultiGet, coalescing iterators, and edge cases
• Dual-index verification pattern catches divergence between standard and trie paths
• Stress-like patterns (BatchedPrefixScanStressLike) mimic crash-test workloads
• Clean C API bridge via SyncIndexModeFromLegacyUdiOptions()
• All three build systems (Make, CMake, BUCK) updated consistently

---

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

---

Summary

Clean, well-structured extension of test coverage from 2 modes (bool) to 3 modes (IndexMode). kCustomOnly guards are applied correctly throughout. The main issue is that SCOPED_TRACE labels in BothIndexReadOptions()-based loops produce swapped labels in kCustomDefault mode (kBuiltin labeled "custom", kDefault labeled "builtin") and a wrong label in kCustomOnly mode (trie labeled "builtin"). This makes test failure debugging misleading but does not affect correctness.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. SCOPED_TRACE labels are swapped in kCustomDefault mode -- trie_index_db_test.cc (multiple sites)

• Issue: The pattern ro.read_index != ReadOptions::ReadIndex::kDefault ? "custom index" : "builtin index" produces wrong labels in kCustomDefault mode. In that mode, StandardIndexReadOptions() returns read_index=kBuiltin (labeled "custom index" -- wrong), and TrieIndexReadOptions() returns read_index=kDefault (labeled "builtin index" -- wrong). In kCustomOnly, BothIndexReadOptions() returns only {kDefault RO} which is labeled "builtin index" but actually routes through the trie.
• Root cause: The SCOPED_TRACE condition checks whether read_index differs from kDefault, but the semantic meaning of kDefault depends on the mode: in kStandardDefault it means "use standard", in kCustomDefault/kCustomOnly it means "use custom".
• Impact: When a test assertion fails in kCustomDefault/kCustomOnly, the SCOPED_TRACE output will identify the wrong index, making debugging harder. Not a correctness issue.
• Suggested fix: Use a helper method that considers both read_index and IsPrimaryMode():

  std::string IndexLabel(const ReadOptions& ro) const {
    if (ro.read_index == ReadOptions::ReadIndex::kBuiltin) return "builtin index";
    if (ro.read_index == ReadOptions::ReadIndex::kPreferCustom) return "custom index";
    // kDefault: follows mode
    return IsPrimaryMode() ? "custom index" : "builtin index";
  }

  This also eliminates the repeated ternary expression (~20 occurrences).

M2. ROCKSDB_GTEST_SKIP used where ROCKSDB_GTEST_BYPASS is more appropriate -- trie_index_db_test.cc:4407,4559,4620,4935

• Issue: The new kCustomOnly-specific guards use ROCKSDB_GTEST_SKIP ("Not applicable in kCustomOnly mode"), but these are permanent parameterization exclusions, not temporary feature-unavailability skips. The existing IsPrimaryMode() guards in the same file correctly use ROCKSDB_GTEST_BYPASS.
• Root cause: ROCKSDB_GTEST_SKIP is for temporary conditions (e.g., "Zlib not linked"). ROCKSDB_GTEST_BYPASS is for permanent parameterization exclusions per the header comment in testharness.h.
• Suggested fix: Change to ROCKSDB_GTEST_BYPASS for consistency with the existing guards in this file.

🟢 LOW / NIT

L1. Inconsistent SCOPED_TRACE approach between VerifyScanBothIndexes and BothIndexReadOptions loops

• Issue: VerifyScanBothIndexes() uses hardcoded strings ("standard index forward", "trie index forward") that are always correct. Code using BothIndexReadOptions() + SCOPED_TRACE uses the read_index-based ternary that produces wrong labels. Two different SCOPED_TRACE strategies in the same file is confusing.
• Suggested fix: After implementing the IndexLabel() helper from M1, use it consistently everywhere.

L2. ParallelCompressionWithHashStandardIndexAndTrieUdi is TEST_F, not TEST_P

• Issue: This test is TEST_F (non-parameterized) while all other tests are TEST_P. The test hardcodes kStandardDefault mode. This means kCustomDefault + hash and kCustomOnly + hash are not tested.
• Assessment: This is intentional per the comment: kCustomDefault/kCustomOnly are incompatible with kHashSearch/kTwoLevelIndexSearch. Not a bug, but a comment explaining why it's TEST_F would help readability.

L3. PrefetchWithCustomIndexWrapper skips kStandardDefault but not kCustomOnly

• Issue: The test skips kStandardDefault ("Only applicable in primary modes") but runs in both kCustomDefault and kCustomOnly. In kCustomOnly, the SST properties check for index_key_is_user_key is skipped (guarded by if (GetIndexMode() == kCustomDefault)). The test still exercises the prefetch code path in kCustomOnly via the reopen + scan, which is correct.
• Assessment: Working as intended but the test could be clearer about what kCustomOnly exercises vs skips.

L4. No test for kCustomOnly -> kCustomDefault rollback path

• Issue: The migration tests cover kStandardDefault -> kCustomDefault and kCustomDefault -> kStandardDefault rollback. There is no test for the kCustomOnly -> kCustomDefault rollback path (compacting to rewrite stubs with full standard indexes). The table.h docs say: "From kCustomOnly: switch to kCustomDefault and compact to rewrite all SSTs with both indexes before downgrading further."
• Assessment: This is a missing coverage gap but arguably belongs in a separate PR since the kCustomOnly -> kCustomDefault path requires compaction to rebuild the standard index, which is a more complex test.

L5. IngestExternalFile test hardcodes kStandardDefault for the SST writer

• Issue: The ingested SST is always built with kStandardDefault regardless of the DB's index mode. In kCustomOnly mode, the DB opens the ingested SST successfully because the SST has a UDI block, and the reader wrapper correctly routes through the custom index when standard_index_is_stub_=false and udi_is_primary_=true.
• Assessment: Correct behavior. The ingested SST has both indexes, so kCustomOnly reads work via udi_is_primary_. No fix needed, but a comment explaining this choice would help.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
kStandardDefault	YES	YES	None
kCustomDefault	YES	YES (routing correct)	Fix SCOPED_TRACE labels
kCustomOnly	YES	YES (guards applied)	Fix SCOPED_TRACE labels
kStandardRequired	NOT TESTED	N/A	Not in INSTANTIATE (intentional -- tested elsewhere in the stack)
kStandardOnly	NOT TESTED	N/A	No UDI factory involvement

Positive Observations

• Thorough kCustomOnly guards: Every test that uses StandardIndexReadOptions() or BothIndexReadOptions() has been audited for kCustomOnly compatibility. The guards are correctly placed.
• Good test decomposition: BothIndexReadOptions() helper cleanly abstracts the mode-dependent RO selection.
• Migration/rollback tests: The existing migration tests correctly skip in kCustomOnly mode where the mixed-mode path is inapplicable.
• New parallel compression tests: Good addition of ParallelCompressionWithTrieIndex covering all three modes, plus ParallelCompressionWithHashStandardIndexAndTrieUdi covering the hash + trie combination.
• PrefetchWithCustomIndexWrapper: Well-targeted test for the Prefetch boundary comparison issue with user_key() vs key().
• ScanAllKeys() no-arg version: Correctly routes through trie in kCustomOnly, standard in kStandardDefault/kCustomDefault.
• VerifyScanBothIndexes: Correctly uses hardcoded SCOPED_TRACE labels, avoiding the read_index-based issue.

---

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

---

Summary

Clean test infrastructure migration from bool-parameterized to IndexMode-parameterized tests, with good kCustomOnly coverage and new parallel compression tests. The kCustomOnly adaptations (stub-aware skips, BothIndexReadOptions helper) are well-structured.

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

Full review (click to expand)

Findings

🟡 MEDIUM

M1. SCOPED_TRACE labels are swapped in kCustomDefault mode -- trie_index_db_test.cc (58 occurrences)

• Issue: The SCOPED_TRACE pattern ro.read_index != ReadOptions::ReadIndex::kDefault ? "custom index" : "builtin index" produces wrong labels in kCustomDefault mode. In that mode, StandardIndexReadOptions() sets read_index = kBuiltin (non-default → labeled "custom index"), while TrieIndexReadOptions() leaves read_index = kDefault (labeled "builtin index"). Both labels are inverted.
• Root cause: The discriminator != kDefault worked when the old code used table_index_factory != nullptr as the differentiator, but with the new ReadIndex enum, kBuiltin is non-default too. The check should be read_index == ReadOptions::ReadIndex::kPreferCustom to identify the custom index path.
• Suggested fix: Either:
  (a) Change to ro.read_index == ReadOptions::ReadIndex::kPreferCustom ? "custom index" : "builtin index", or
  (b) Extract a small helper like static const char* IndexLabel(ReadOptions::ReadIndex ri) to centralize the logic and eliminate the 58 repetitions.

M2. Repeated SCOPED_TRACE boilerplate (58 occurrences) -- trie_index_db_test.cc

• Issue: The 3-line SCOPED_TRACE pattern is copy-pasted 58 times throughout the file. Per CLAUDE.md unit test dedup guidelines: "Extract helper functions for repeated patterns."
• Suggested fix: Add a helper method to the test fixture:

  static std::string IndexLabel(const ReadOptions& ro) {
    switch (ro.read_index) {
      case ReadOptions::ReadIndex::kPreferCustom: return "custom index";
      case ReadOptions::ReadIndex::kBuiltin: return "builtin index";
      default: return "default index";
    }
  }

  Then replace all 58 occurrences with SCOPED_TRACE(IndexLabel(ro)).

🟢 LOW / NIT

L1. ScanAllKeys() comment is slightly misleading -- trie_index_db_test.cc:240

• Issue: Comment says "standard index for kStandardDefault/kCustomDefault" but in kCustomDefault mode StandardIndexReadOptions() returns kBuiltin, which forces the standard (builtin) index. The comment is technically accurate (standard index IS used) but the mechanism isn't via the "default" routing -- it's via an explicit override. Could confuse future maintainers.
• Suggested fix: Clarify: "standard index for kStandardDefault, forced-builtin for kCustomDefault, trie for kCustomOnly".

L2. IngestExternalFileWithTrieUDI always uses kStandardDefault for ingested SST -- trie_index_db_test.cc:1172

• Issue: When the test runs in kCustomOnly mode, the DB-level SSTs have only a stub standard index, but the ingested SST is built with kStandardDefault (both indexes). This mixed-mode scenario is actually a valid test case, but the test doesn't explicitly document this intentional mismatch.
• Suggested fix: Add a brief comment explaining why kStandardDefault is used for the ingested SST even in kCustomOnly mode (e.g., "SstFileWriter doesn't know the DB's index_mode; kStandardDefault ensures the ingested SST has both indexes for maximum compatibility").

L3. ParallelCompressionWithTrieIndex uses GetParam() directly instead of GetIndexMode() -- trie_index_db_test.cc:5107

• Issue: One instance uses GetParam() directly (if (GetParam() != BlockBasedTableOptions::IndexMode::kCustomOnly)) while all other similar checks use GetIndexMode(). Minor inconsistency.
• Suggested fix: Replace with GetIndexMode() for consistency.

L4. Missing ROCKSDB_GTEST_SKIP in PrefetchWithCustomIndexWrapper -- trie_index_db_test.cc:4981

• Issue: This test returns early for kStandardDefault with ROCKSDB_GTEST_SKIP which is correct, but uses a bare string "Only applicable in primary modes" while other tests use more specific skip messages. Very minor inconsistency, no functional impact.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
kStandardDefault	YES	YES	N/A
kCustomDefault	YES	YES (labels are wrong but tests pass)	Fix labels
kCustomOnly	YES	YES	N/A
WritePreparedTxnDB	YES (TransactionDB tests exist)	YES	N/A
Parallel compression	YES (new tests)	YES	N/A
kHashSearch index_type	YES (new TEST_F)	YES	N/A

Positive Observations

• The BothIndexReadOptions() helper is a clean abstraction for handling kCustomOnly's single-index constraint.
• New CustomOnlySSTsSurviveLoweredIndexMode test (TEST_F, not parameterized) is excellent -- it validates the critical downgrade path.
• ParallelCompressionWithTrieIndex and ParallelCompressionWithHashStandardIndexAndTrieUdi provide end-to-end coverage of the parallel compression pipeline with custom indexes, which was previously untestable.
• Tests that are inherently incompatible with kCustomOnly (migration paths, rollback-without-compact) are properly skipped with ROCKSDB_GTEST_SKIP.
• The test correctly verifies that kBuiltin reads on kCustomOnly SSTs fail with IsInvalidArgument() (not silent empty results).

---

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

[zaidoon1]

Split between #14963 and #14966. The mode, downgrade, and prefetch cases now accompany table routing, while the parallel compression cases accompany the trie parallel builder.