Replace fragile dict stored-key API with getKeyId callback#23
Conversation
WalkthroughThis refactoring replaces Redis's dictionary stored-key API mechanism. It removes the legacy Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/defrag.c (1)
274-283: Use the converted key for the lookup after stored-key API removal.
dictStoredKey2Key()is computed but not used, anddictFindLinkstill receivesptr(stored key). After removingdictUseStoredKeyApi, this can fail to find the entry or trip the assert for dicts that requirekeyFromStoredKey. Use the converted key for the lookup.🛠️ Proposed fix
- const void *key = dictStoredKey2Key(d, ptr); - link = dictFindLink(d, ptr, NULL); + const void *key = dictStoredKey2Key(d, ptr); + link = dictFindLink(d, key, NULL);src/dict.c (1)
887-915: Fix lookup mismatch whennewItem == 0(stored-key vs lookup key).
When*link == NULL,dictFindLinkis called withaddedKey(stored-key). IfkeyFromStoredKeyis used, this can fail to locate the entry and trigger the assert.🐛 Suggested fix (lookup with extracted key)
- *link = dictFindLink(d, addedKey, NULL); + *link = dictFindLink(d, dictStoredKey2Key(d, addedKey), NULL);
🤖 Fix all issues with AI agents
In `@src/dict.c`:
- Around line 97-108: Remove the unused helper validateStoredKeyConversion and
inline its validation into dictStoredKey2Key: call d->type->keyFromStoredKey
when present, store the result, and if d->type->keyFromStoredKey exists but
returns NULL then propagate NULL (or handle as the original validation
required); otherwise return the converted key, and when keyFromStoredKey is
absent return the original key. Delete the validateStoredKeyConversion function
to fix the -Werror=unused-function build breaker.
🧹 Nitpick comments (2)
src/t_hash.c (1)
1253-1260: TightenhashTypeDeletesignature tosdsfor type safety.
The function now always treatsfieldas an SDS (sdslen), so thevoid *parameter invites misuse. Making the type explicit will prevent accidental non-SDS callers as the oldisSdsFieldpath is gone.♻️ Proposed refactor
-int hashTypeDelete(robj *o, void *field) { +int hashTypeDelete(robj *o, sds field) { int deleted = 0; - int fieldLen = sdslen((sds)field); + int fieldLen = sdslen(field);src/server.c (1)
316-318: Document non‑NULL key invariant (and keep const).Line 316:
kvGetKeycasts awayconstand returnskvobjGetKey()without guarding against a NULL key, which would later be dereferenced indictSdsCompareKV. If non‑iskvobjentries are impossible here, make that invariant explicit (and preserve constness).🔧 Suggested tweak
-static const void *kvGetKey(const void *kv) { - sds sdsKey = kvobjGetKey((kvobj *) kv); - return sdsKey; -} +static const void *kvGetKey(const void *kv) { + const kvobj *kvo = (const kvobj *)kv; + sds sdsKey = kvobjGetKey(kvo); + serverAssert(sdsKey != NULL); + return sdsKey; +}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/defrag.csrc/dict.csrc/dict.hsrc/kvstore.csrc/memory_prefetch.csrc/module.csrc/rdb.csrc/server.csrc/server.hsrc/t_hash.c
💤 Files with no reviewable changes (2)
- src/kvstore.c
- src/rdb.c
🧰 Additional context used
🧬 Code graph analysis (6)
src/defrag.c (1)
src/dict.c (1)
dictStoredKey2Key(97-99)
src/server.h (1)
src/t_hash.c (1)
hashTypeDelete(1257-1297)
src/memory_prefetch.c (2)
src/object.c (1)
kvobjGetKey(234-246)src/dict.c (2)
dictGetNext(1081-1085)dictCompareKeys(502-506)
src/dict.h (1)
src/dict.c (6)
dictAdd(493-500)dictAddRaw(526-536)dictAddOrFind(618-622)dictReplace(587-609)dictSetKeyAtLink(887-932)dictSetKey(998-1004)
src/server.c (2)
src/object.c (1)
kvobjGetKey(234-246)src/sds.h (1)
sdslen(98-111)
src/module.c (1)
src/t_hash.c (1)
hashTypeDelete(1257-1297)
🪛 Cppcheck (2.19.0)
src/dict.c
[error] 341-341: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
🪛 GitHub Actions: CI
src/dict.c
[error] 102-102: static int validateStoredKeyConversion(dict *d, const void *key __stored_key) is defined but not used. ( -Werror=unused-function )
🪛 GitHub Actions: External Server Tests
src/dict.c
[error] 102-102: validateStoredKeyConversion defined but not used. (no-unused-function). This warning is treated as an error due to -Werror.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: code-coverage
- GitHub Check: Analyze (cpp)
- GitHub Check: code-coverage
🔇 Additional comments (34)
src/module.c (1)
5451-5456: LGTM — updated call matches the simplified hashTypeDelete signature.src/defrag.c (1)
489-492: LGTM: using the link to update hash dict keys.This matches the
no_value=1hash dict behavior and avoids re-looking up the entry.src/memory_prefetch.c (1)
233-238: LGTM: compare against the extracted key.Using
kvobjGetKey(kv)keepsdictCompareKeysoperating on the expected key type.src/server.h (1)
3656-3659: AllhashTypeDeletecall sites have been correctly updated to the new signature.The search confirms 7 call sites across
src/t_hash.candsrc/module.c, and all consistently use the 2-argument form matching the prototype atsrc/server.h:3658. No lingering calls with the old signature remain.src/t_hash.c (6)
533-539: LGTM: expiration-triggered deletion uses the unified delete path.
The updated call tohashTypeDeletealigns with the new API and preserves the prior behavior.
761-767: LGTM: expired-field lazy delete still guarded by assert.
The assert remains valid with the simplified signature.
1107-1114: LGTM: HT expiry deletion path now consistent with new API.
No behavioral change spotted beyond the signature update.
2718-2724: LGTM: HGETDEL uses the simplified delete path.
Deletion remains guarded byGETF_OK.
2927-2932: LGTM: HDEL now uses the unified delete API.
The semantics remain unchanged.
3499-3504: LGTM: active-expire deletion path aligned with new signature.
No issues spotted.src/server.c (2)
603-615: Good wiring ofkeyFromStoredKeyfor DB dicts.Line 603–629: the new
keyFromStoredKey = kvGetKeyhookup for both DB dict types aligns cleanly with the callback‑based API and keeps the dictType definitions straightforward.Also applies to: 617-629
321-333: The argument order concern is invalid—no fix needed.The dict layer consistently passes the lookup key as the first argument to the comparison function across all lookup paths (lines 651, 802, 973, 1770 in src/dict.c), which is exactly what
dictSdsCompareKVassumes when cachingsdsKey1. ThekeyFromStoredKeymigration does not change this behavior, and both dicts usingdictSdsCompareKV(dbDictType and dbExpiresDictType) properly set.keyFromStoredKey = kvGetKeyto extract the sds key from the stored kvobj.src/dict.c (15)
79-79: Prototype updated for stored-key input looks consistent.
124-126: Hash function wiring remains correct.
156-161: Stored-key annotation on no-value entry creation looks consistent.
337-365: Rehash path correctly uses stored-key conversion for hashing.
492-498: Signature aligns with stored-key API.
526-535: Lookup uses converted key as intended.
542-575: Stored-key annotation on insert helper is consistent.
587-595: Stored-key signature update looks good.
618-621: Stored-key signature update looks good.
633-651: Delete path uses lookup-key hashing and stored-key conversion correctly.
779-803: Find-link path correctly uses lookup hash + stored-key conversion.
964-973: Two-phase unlink lookup aligns with stored-key conversion.
998-1003: Stored-key annotation on dictSetKey matches updated API.
1752-1770: Insert-position lookup correctly compares via stored-key conversion.
1807-1809: Hash wrapper looks fine.src/dict.h (7)
55-67: Stored-key documentation and marker are clear.
94-98: Stored-key annotations on key callbacks are consistent.
128-137: New callbacks integrate cleanly into dictType.
156-158: pauseAutoResize type change looks fine.
239-243: Public API signatures align with stored-key semantics.
287-289: dictSetKeyAtLink declaration matches stored-key usage.
290-292: dictSetKey declaration aligns with stored-key annotation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| static const void *dictStoredKey2Key(dict *d, const void *key __stored_key) { | ||
| return (d->type->keyFromStoredKey) ? d->type->keyFromStoredKey(key) : key; | ||
| } | ||
|
|
||
| /* Validate that stored-key to key conversion works correctly */ | ||
| static int validateStoredKeyConversion(dict *d, const void *key __stored_key) { | ||
| const void *extracted = dictStoredKey2Key(d, key); | ||
| if (d->type->keyFromStoredKey) { | ||
| return extracted != NULL; | ||
| } | ||
| return extracted == key; | ||
| } |
There was a problem hiding this comment.
Fix unused validateStoredKeyConversion build breaker.
CI fails with -Werror=unused-function; the helper is never referenced. Consider folding validation into dictStoredKey2Key and removing the unused function.
🐛 Suggested fix (inline validation, remove unused helper)
-static const void *dictStoredKey2Key(dict *d, const void *key __stored_key) {
- return (d->type->keyFromStoredKey) ? d->type->keyFromStoredKey(key) : key;
-}
-
-/* Validate that stored-key to key conversion works correctly */
-static int validateStoredKeyConversion(dict *d, const void *key __stored_key) {
- const void *extracted = dictStoredKey2Key(d, key);
- if (d->type->keyFromStoredKey) {
- return extracted != NULL;
- }
- return extracted == key;
-}
+static const void *dictStoredKey2Key(dict *d, const void *key __stored_key) {
+ if (d->type->keyFromStoredKey) {
+ const void *extracted = d->type->keyFromStoredKey(key);
+ debugAssert(extracted != NULL);
+ return extracted;
+ }
+ return key;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static const void *dictStoredKey2Key(dict *d, const void *key __stored_key) { | |
| return (d->type->keyFromStoredKey) ? d->type->keyFromStoredKey(key) : key; | |
| } | |
| /* Validate that stored-key to key conversion works correctly */ | |
| static int validateStoredKeyConversion(dict *d, const void *key __stored_key) { | |
| const void *extracted = dictStoredKey2Key(d, key); | |
| if (d->type->keyFromStoredKey) { | |
| return extracted != NULL; | |
| } | |
| return extracted == key; | |
| } | |
| static const void *dictStoredKey2Key(dict *d, const void *key __stored_key) { | |
| if (d->type->keyFromStoredKey) { | |
| const void *extracted = d->type->keyFromStoredKey(key); | |
| debugAssert(extracted != NULL); | |
| return extracted; | |
| } | |
| return key; | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 102-102: static int validateStoredKeyConversion(dict *d, const void *key __stored_key) is defined but not used. ( -Werror=unused-function )
🪛 GitHub Actions: External Server Tests
[error] 102-102: validateStoredKeyConversion defined but not used. (no-unused-function). This warning is treated as an error due to -Werror.
🤖 Prompt for AI Agents
In `@src/dict.c` around lines 97 - 108, Remove the unused helper
validateStoredKeyConversion and inline its validation into dictStoredKey2Key:
call d->type->keyFromStoredKey when present, store the result, and if
d->type->keyFromStoredKey exists but returns NULL then propagate NULL (or handle
as the original validation required); otherwise return the converted key, and
when keyFromStoredKey is absent return the original key. Delete the
validateStoredKeyConversion function to fix the -Werror=unused-function build
breaker.
1 participant
Benchmark PR from agentic-review-benchmarks#1
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.