find_first_key_by_prefix, find_last_key_by_prefix#6171
Closed
afck wants to merge 3 commits into
Closed
Conversation
Adds `find_first_key_by_prefix`, `find_last_key_by_prefix`, and the value variants to `ReadableKeyValueStore`, with default impls falling back to `find_keys_by_prefix(...).first()/.last()`. Native overrides: - memory: BTreeMap range, O(log N). - RocksDB: forward iterator first; for last, walk forward in the bounded range and keep the last entry. RocksDB's reverse-iteration primitives interact awkwardly with our prefix-bounded scans (seek_for_prev / seek_to_last with iterate bounds returned invalid for keys clearly in range), so the asymmetric forward-walk is a robust fallback for now. - ScyllaDB: prepared statements with `ORDER BY k ASC/DESC LIMIT 1`. - DynamoDB: `Query` with `Limit=1` and `ScanIndexForward=true/false`. - IndexedDB: `IDBCursor` with `Next`/`Prev` direction. - Wrappers (journaling, value_splitting, lru_caching, metering, dual): forward to the inner store; metering adds latency histograms; value_splitting strips the 4-byte segment index from the result. Adds view-level helpers: - `ByteMapView::first_key`, `last_key`, `first_key_value`, `last_key_value` - `MapView` / `CustomMapView::first_index`, `last_index`, `first_index_value`, `last_index_value` - `ByteSetView::first_key`, `last_key` - `SetView` / `CustomSetView::first_index`, `last_index` The view methods take a fast path straight to the store when there are no staged changes; otherwise they fall back to the existing `for_each_key_while` / `for_each_index` machinery, which already handles the merge of in-memory updates with persisted state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`LruCachingStore::find_{first,last}_key[_value]_by_prefix` now check
the existing find_keys / find_key_values cache before forwarding to the
inner store, returning `.first()` / `.last_back()` of the cached result
on hit. No new cache map is added; if the cache misses we still fall
through to the inner store so the inner-store optimization is preserved.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The database is opened with an 8-byte fixed-prefix extractor for prefix bloom-filter optimization. Without `total_order_seek` set in `ReadOptions`, RocksDB's `seek_for_prev` and `seek_to_last` only seek within the bloom-prefix scope and silently return invalid for keys whose extractor-prefix differs from the seek target — which is why the earlier reverse-iterator attempts returned `valid=false` for keys that clearly existed in the bounded range. With `total_order_seek(true)`, `seek_for_prev(upper_bound)` (stepping back if it lands exactly on the upper bound) gives us the largest key with the prefix in a single seek. Replaces the O(N) forward-walk fallback in `find_last_key_by_prefix_internal` and `find_last_key_value_by_prefix_internal`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Instruction Count Benchmark Results
Deterministic metrics — reproducible across runs (34 benchmarks)
Cache-dependent metrics — expect fluctuations between runs (34 benchmarks)
|
MathieuDutSik
left a comment
MathieuDutSik
left a comment
Contributor
There was a problem hiding this comment.
It is a nice feature.
But it should be noted, that while it is not exactly a subset, it is strongly related to the PR #4975
Comment on lines
+353
to
+359
| if let Some(keys) = cache.query_find_keys(key_prefix) { | ||
| #[cfg(with_metrics)] | ||
| metrics::FIND_KEYS_BY_PREFIX_CACHE_HIT_COUNT | ||
| .with_label_values(&[]) | ||
| .inc(); | ||
| return Ok(keys.into_iter().next()); | ||
| } |
Contributor
There was a problem hiding this comment.
If you match on the query_find_key_values then you can also prematurely end the search.
Comment on lines
+370
to
+376
| if let Some(keys) = cache.query_find_keys(key_prefix) { | ||
| #[cfg(with_metrics)] | ||
| metrics::FIND_KEYS_BY_PREFIX_CACHE_HIT_COUNT | ||
| .with_label_values(&[]) | ||
| .inc(); | ||
| return Ok(keys.into_iter().next_back()); | ||
| } |
Comment on lines
+381
to
+396
| async fn find_first_key_value_by_prefix( | ||
| &self, | ||
| key_prefix: &[u8], | ||
| ) -> Result<Option<(Vec<u8>, Vec<u8>)>, Self::Error> { | ||
| if let Some(cache) = self.get_exclusive_cache() { | ||
| let mut cache = cache.lock().unwrap(); | ||
| if let Some(key_values) = cache.query_find_key_values(key_prefix) { | ||
| #[cfg(with_metrics)] | ||
| metrics::FIND_KEY_VALUES_BY_PREFIX_CACHE_HIT_COUNT | ||
| .with_label_values(&[]) | ||
| .inc(); | ||
| return Ok(key_values.into_iter().next()); | ||
| } | ||
| } | ||
| self.store.find_first_key_value_by_prefix(key_prefix).await | ||
| } |
Contributor
There was a problem hiding this comment.
I guess we do not need a corresponding cache entry such as cache.query_find_first_key or similar just yet.
Comment on lines
+846
to
+857
| let mut view = CustomSetStateView::load(context.clone()).await?; | ||
| view.set.insert(&1u128)?; | ||
| view.set.insert(&256u128)?; | ||
| view.save().await?; | ||
| } | ||
| { | ||
| let view = CustomSetStateView::load(context.clone()).await?; | ||
| assert_eq!(view.set.first_index().await?, Some(1u128)); | ||
| assert_eq!(view.set.last_index().await?, Some(256u128)); | ||
| } | ||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
I would prefer actually to have all those tests in a single function fn unit_tests_first_last_map_view.
I would also add tests to run_map_view_mutability. It is a fuzzing test and it could catch some regression.
More importantly, we should have tests that exercise the pain points of the implementation. And that can be for example the ValueSplitting problems show up with large values. But there is a LimitedTestMemoryStore that can help dealing with that.
Comment on lines
+263
to
+271
| Ok(self | ||
| .store | ||
| .find_first_key_by_prefix(key_prefix) | ||
| .await? | ||
| .map(|mut key| { | ||
| key.truncate(key.len() - 4); | ||
| key | ||
| })) | ||
| } |
Contributor
There was a problem hiding this comment.
Unfortunately, this could be a leftover key, so I am not sure that even this construction works.
Comment on lines
+273
to
+285
| async fn find_last_key_by_prefix( | ||
| &self, | ||
| key_prefix: &[u8], | ||
| ) -> Result<Option<Vec<u8>>, Self::Error> { | ||
| // The largest stored key under any user-key prefix is `K|n` for the largest | ||
| // user key `K` and its highest segment index `n`. Strip the trailing 4 bytes. | ||
| Ok(self | ||
| .store | ||
| .find_last_key_by_prefix(key_prefix) | ||
| .await? | ||
| .map(|mut key| { | ||
| key.truncate(key.len() - 4); | ||
| key |
Contributor
There was a problem hiding this comment.
That is also broken by the leftover keys.
Scenario is the following:
- Write a big value that uses several keys.
- Remove that key with a
DeleteKeyin a batch. Since removing the other segment keys requires doing a read, they are left over. - Then those keys get a wrong result.
The solution is to:
- Read the index of the key in question. If zero, it is good, return.
- If non-zero, we know it is a segment key (leftover or not, no way to know with an async operation). So, revert to a
find_keys_by_prefix.
| key.truncate(key.len() - 4); | ||
| key | ||
| })) | ||
| } |
Contributor
There was a problem hiding this comment.
The implementation is actually using the default trait implementation, which uses a find_key_values_by_prefix.
That makes the implementation correct. However, in my opinion, it also makes it slow. All the containers are using this ValueSplitting. So, this is just about the same as using the find_keys_by_prefix.
Instead, I think that a better approach is:
- Look for the first (key, value). If reading it allows to prove it is not segment, then return it.
- Barring that, use
find_key_values_by_prefix.
Open
Contributor
Author
|
Closing in favor of #6183. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
As discussed in #6163, it's useful to be able to read the last key of a map view.
Proposal
Add
find_first_key_by_prefix,find_last_key_by_prefix, and the value variants toReadableKeyValueStore, with default impls falling back tofind_keys_by_prefix(...).first()/.last(). Native overrides:BTreeMaprange, O(log N)QuerywithLimit=1andScanIndexForward=true/falseIDBCursorwithNext/PrevdirectionAdds view-level helpers:
ByteMapView::first_key,last_key,first_key_value,last_key_valueMapView/CustomMapView::first_index,last_index,first_index_value,last_index_valueByteSetView::first_key,last_keySetView/CustomSetView::first_index,last_indexThe view methods take a fast path straight to the store when there are no staged changes; otherwise they fall back to the existing
for_each_key_while/for_each_indexmachinery, which already handles the merge of in-memory updates with persisted state.Test Plan
Tests were added.
Release Plan
Links