Support all operation types in User Defined Index (UDI) interface by zaidoon1 · Pull Request #14399 · facebook/rocksdb

zaidoon1 · 2026-02-27T21:49:13Z

Remove the restriction that limited UDI to ingest-only, Puts-only use cases. This enables UDI plugins (including the trie index from #14310) to work with all operation types: Put, Delete, Merge, SingleDelete, PutEntity, etc.

Part of #12396

github-actions · 2026-02-27T21:56:54Z

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

Completed in 411.3s.

Summary by check

Check	Count
`concurrency-mt-unsafe`	1
Total	1

Details

db_stress_tool/db_stress_tool.cc (1 warning(s))

db_stress_tool/db_stress_tool.cc:235:5: warning: function is not thread safe [concurrency-mt-unsafe]

zaidoon1 · 2026-02-27T22:40:42Z

some notes: I don't have db level tests, currently i'm ussing the sstfilewriter for tests which doesn't support all operations so that's not great.

also we are missing things like seekToFirst, etc.. so i'll see if i can add that too as part of this

zaidoon1 · 2026-02-28T00:29:25Z

[ ]

some notes: I don't have db level tests, currently i'm ussing the sstfilewriter for tests which doesn't support all operations so that's not great.

also we are missing things like seekToFirst, etc.. so i'll see if i can add that too as part of this

this is done for now. I think what i have here is good for this PR.

Remove the restriction that limited UDI to ingest-only, Puts-only use cases. This enables UDI plugins (including the trie index from facebook#14310) to work with all operation types: Put, Delete, Merge, SingleDelete, PutEntity, etc. Part of facebook#12396

zaidoon1 · 2026-03-05T04:56:08Z

FYI, still working on some bugs i found. please ignore this PR for now.

…null comparator Fix three bugs in the trie-based User Defined Index (UDI): 1. BFS overflow reordering in louds_trie.cc Finish(): overflow handles and seqnos were not BFS-reordered, causing wrong block handles when BFS traversal order differs from key-sorted insertion order. Refactored leaf emission into an emit_leaf lambda that also reorders overflow arrays. 2. NextAndGetResult in user_defined_index_wrapper.h returned a user key instead of an internal key, violating the InternalIteratorBase contract. Now sets result->key = key() (internal key with 8-byte suffix) and copies bound_check_result/value_prepared individually. 3. NewReader in trie_index_factory.cc crashed on null comparator even though NewBuilder already handled it. Now defaults to BytewiseComparator(). Also fixes minor inconsistencies: overflow_run_size_=1 initialization in NewBuilder, SCOPED_TRACE brace style, and a stale comment in table_test.cc. Tests: OverflowBfsReordering (verified fails without fix), WrapperNextAndGetResultReturnsInternalKey (verified fails without fix), expanded NullComparator test. facebook#12396

zaidoon1 · 2026-03-05T06:39:07Z

the current changes look good to me. I've run a 10 minute blackbox crash test without any issues. Currently running a 1 hour blackbox crash test then 1 hour whitebox crash test to really make sure we are good

The trie index builder incorrectly called FindShortSuccessor() on the last block's separator key, producing a wider key range than the standard ShortenedIndexBuilder (which uses the last key as-is with the default kShortenSeparators mode). This caused the trie to claim coverage of keys between the actual last key and its shortened successor, leading to iterator desynchronization in test_batches_snapshots with deletes. Fix: use last_key_in_current_block directly as the last block's separator, matching the standard builder's behavior. Also use comparator->Compare() instead of string equality for same-key-run detection. Add regression tests (LastBlockSeparatorNotShortened, LastBlockSeparatorWithDeletes) and BatchedPrefixScan tests that reproduce the crash-test failure pattern. Remove leftover debug cross-validation scaffolding from the UDI wrapper's NewIterator. Fix all clang-tidy warnings across UDI/trie files: missing override, uninitialized members, redundant virtual, missing destructors, braces around statements, unnecessary copies, and static string init. Ref: facebook#12396

zaidoon1 · 2026-03-05T17:06:17Z

utilities/trie_index/trie_index_factory.cc

+    // Last block: use the last key itself as the separator, NOT a shortened
+    // successor. The standard index builder (ShortenedIndexBuilder) only
+    // calls FindShortInternalKeySuccessor when shortening_mode ==
+    // kShortenSeparatorsAndSuccessor, which is not the default. With the


this was causing the crash tests to fail. I'm not sure what we want to do in general and whether we want to pass the block options that sets the mode and change behaviour depending on that

Could you elaborate more on the issue?

zaidoon1 · 2026-03-05T18:43:26Z

@xingbowang this is now ready for review whenever you have a chance.

xingbowang · 2026-03-10T14:24:23Z

M1 — key_type_log_ thread safety (2 agents agree): The TestUserDefinedIndexFactory::key_type_log_ shared vector has no synchronization. Not a production bug, but a latent data race in test infrastructure.

M2 — SeekToFirstAndGetResult default assumes BytewiseComparator (2 agents agree): The default implementation SeekAndGetResult(Slice()) is incorrect for non-bytewise comparators. Current risk is low (trie UDI overrides this and only supports BytewiseComparator), but third-party UDI implementations could hit silent bugs. Consider making it pure-virtual.

M3 — Simultaneous removal of all stress test restrictions (2 agents agree): All operation-type restrictions removed at once. The targeted DB-level tests (22 test cases) are thorough, but the crash test explores a much wider combinatorial space. Acceptable for EXPERIMENTAL, but worth monitoring.

M4 — prev_key_scratch_ string copy in NextAndGetResult: Could use std::move instead of copy to avoid per-Next() heap allocation for keys exceeding SSO threshold. One-line change, zero risk.

M5 — Missing degenerate SST edge cases at DB level: No DB-level tests for single-entry SST, deletion-only SST, or all-same-key SST. SST-level tests exist but don't exercise the full DB path.

xingbowang · 2026-03-10T13:12:39Z

tools/db_crashtest.py

-        dest_params["use_merge"] = 0
-        dest_params["use_put_entity_one_in"] = 0
-        dest_params["use_timed_put_one_in"] = 0
-        dest_params["use_get_entity"] = 0
-        dest_params["use_multi_get_entity"] = 0
-        # TransactionDB ROLLBACK writes DELETE entries to WAL to undo
-        # uncommitted changes. These DELETEs violate UDI's Put-only restriction.
-        dest_params["use_txn"] = 0
        # Trie UDI uses zero-copy pointers into block data, which is
        # incompatible with mmap_read.
        dest_params["mmap_read"] = 0
-        # Redistribute delete/delrange percents to write percent
-        dest_params["writepercent"] += dest_params["delpercent"]
-        dest_params["writepercent"] += dest_params["delrangepercent"]
-        dest_params["delpercent"] = 0
-        dest_params["delrangepercent"] = 0
-        # Ingestion with standalone range deletions is incompatible
-        dest_params["test_ingest_standalone_range_deletion_one_in"] = 0
        # Parallel compression is incompatible with UDI
        dest_params["compression_parallel_threads"] = 1
-        # Trie UDI has a known issue with prefix scanning where certain prefix
-        # patterns cause "SeekToFirst not supported" errors. Disable prefix
-        # scanning and redistribute its percentage to reads.
-        dest_params["readpercent"] += dest_params.get("prefixpercent", 0)
-        dest_params["prefixpercent"] = 0


We are removing quite a lot of these. Could you update the PR with stress test results that have these configuration on? These configuration could be overridden in the db_crashtest.py command line. I want to make sure we have these well tested before removing them. It caused bunch of stress test internally before which created lots of work for us to identify the root cause and add these configs.

xingbowang · 2026-03-10T14:29:47Z

utilities/trie_index/trie_index_factory.cc

+    // Last block: use the last key itself as the separator, NOT a shortened
+    // successor. The standard index builder (ShortenedIndexBuilder) only
+    // calls FindShortInternalKeySuccessor when shortening_mode ==
+    // kShortenSeparatorsAndSuccessor, which is not the default. With the


Could you elaborate more on the issue?

xingbowang · 2026-03-10T14:37:20Z

table/block_based/user_defined_index_wrapper.h

@@ -260,8 +295,10 @@ class UserDefinedIndexIteratorWrapper
      valid_ = result_.bound_check_result == IterBoundCheck::kInbound;
      if (valid_) {
        ikey_.Set(result_.key, 0, ValueType::kTypeValue);


Could you add a comment to explain this? Essentially set result with internal key using user key. This is a general for multiple other APIs. Maybe extract a function helper to make this more explicit.

meta-cla bot added the CLA Signed label Feb 27, 2026

xingbowang self-requested a review February 27, 2026 21:58

zaidoon1 force-pushed the zaidoon/udi-index-full-operation-support branch 2 times, most recently from ef0dd67 to 59b6943 Compare February 28, 2026 00:24

zaidoon1 force-pushed the zaidoon/udi-index-full-operation-support branch 3 times, most recently from efb01b7 to 5bf0770 Compare March 1, 2026 08:12

zaidoon1 force-pushed the zaidoon/udi-index-full-operation-support branch from 5bf0770 to 83c30bc Compare March 5, 2026 03:28

zaidoon1 force-pushed the zaidoon/udi-index-full-operation-support branch from db5bacc to e8931c3 Compare March 5, 2026 05:54

zaidoon1 commented Mar 5, 2026

View reviewed changes

xingbowang reviewed Mar 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support all operation types in User Defined Index (UDI) interface#14399

Support all operation types in User Defined Index (UDI) interface#14399
zaidoon1 wants to merge 3 commits intofacebook:mainfrom
zaidoon1:zaidoon/udi-index-full-operation-support

zaidoon1 commented Feb 27, 2026

Uh oh!

github-actions bot commented Feb 27, 2026 •

edited

Loading

Uh oh!

zaidoon1 commented Feb 27, 2026

Uh oh!

zaidoon1 commented Feb 28, 2026

Uh oh!

zaidoon1 commented Mar 5, 2026

Uh oh!

zaidoon1 commented Mar 5, 2026

Uh oh!

zaidoon1 Mar 5, 2026

Uh oh!

xingbowang Mar 10, 2026

Uh oh!

zaidoon1 commented Mar 5, 2026

Uh oh!

xingbowang commented Mar 10, 2026

Uh oh!

xingbowang Mar 10, 2026

Uh oh!

xingbowang Mar 10, 2026

Uh oh!

xingbowang Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

zaidoon1 commented Feb 27, 2026

Uh oh!

github-actions bot commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Summary by check

Details

Uh oh!

zaidoon1 commented Feb 27, 2026

Uh oh!

zaidoon1 commented Feb 28, 2026

Uh oh!

zaidoon1 commented Mar 5, 2026

Uh oh!

zaidoon1 commented Mar 5, 2026

Uh oh!

zaidoon1 Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

xingbowang Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

zaidoon1 commented Mar 5, 2026

Uh oh!

xingbowang commented Mar 10, 2026

Uh oh!

xingbowang Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

xingbowang Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

xingbowang Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Feb 27, 2026 •

edited

Loading