ZSET B+ Tree PR 2: ordered index interface and full conversion#3840
Open
rainsupreme wants to merge 44 commits into
Open
ZSET B+ Tree PR 2: ordered index interface and full conversion#3840rainsupreme wants to merge 44 commits into
rainsupreme wants to merge 44 commits into
Conversation
zslIsInLexRange called zslGetNodeElement on the tail node before checking if it was NULL, crashing on an empty skiplist. Move the NULL checks before the element access. Same issue existed for the head node check. Signed-off-by: Rain Valentine <rsg000@gmail.com>
Add the OrderedIndex abstraction layer for sorted set data structures: - ordered_index.h: compile-time dispatched interface with inline wrappers for lifecycle, modification, query, iteration, memory, defrag, and debug - skiplist_ordered_index.h/c: skiplist backend implementation - Thin wrappers casting between opaque types and zskiplist/zskiplistNode - Range deletion by score, rank, and lex with on_delete callback - Count range by score and lex - Cursor-based batched defrag scan - Structural integrity verification - ordered_index_test.h: parameterized C++ test fixture for future backends - test_ordered_index.cpp: comprehensive unit tests - Deterministic tests for all interface operations - Randomized property tests (insert, traverse, delete, update, pop) - On-delete callback tests for range deletion - Hashtable consistency tests for range deletion No existing call sites are converted — the interface is only exercised by the new unit tests. Call-site conversion follows in subsequent PRs. Signed-off-by: Rain Valentine <rsg000@gmail.com>
- Remove static inline wrappers; add ordered_index.c with regular functions that delegate to the backend (LTO handles inlining) - Add comprehensive header comment to ordered_index.h explaining what an OrderedIndex is, its relationship to the companion hashtable, and why it does not enforce uniqueness - Add per-function doc comments for all API functions - Add header comment to skiplist_ordered_index.h - Rename 'idx' parameter to 'oi' throughout to avoid confusion with numeric array indices - Drop orderedIndexInsert(sds) convenience wrapper; rename orderedIndexInsertRaw to orderedIndexInsert (callers pass ptr+len) - Remove debug functions (skiplistGetHeight, skiplistVerifyIntegrity) from ordered_index.h public API; they remain in skiplist_ordered_index.h for unit tests and DEBUG command - Fix include ordering: skiplist_ordered_index.h now includes ordered_index.h for type visibility Feedback from JimB123 on PR valkey-io#3552. Signed-off-by: Rain Valentine <rsg000@gmail.com>
Fix two bugs in the skiplist OrderedIndex backend: 1. zslDetachNode: use zslCompareNodes (score-based search) instead of pointer equality (x->level[i].forward != node). Pointer equality overshoots at higher skiplist levels because higher-level forward pointers skip over intermediate nodes. The score-based search correctly stops at the predecessor at each level. 2. skiplistDeleteRangeByScore/Lex/Rank: when an on_delete callback is provided, the callback receives ownership of the item (per the API contract in ordered_index.h). The backend must not call zslFreeNode after invoking the callback — that would double-free. Also fix a stale 2-arg next() call in ordered_index_test.h that didn't get updated when the iterator API changed to pointer-return style. Signed-off-by: Rain Valentine <rsg000@gmail.com>
- Replace 'backend' with 'implementation' throughout - Fix header comment claiming only skiplist exists - Add consistent min_ex/max_ex documentation on all range functions - Soften iterator init comment to describe behavior not mandate - Improve memory estimation and defrag descriptions - Rename zslDeleteNode -> zslUnlinkNode (detach semantics) - Remove unnecessary section header in skiplist.c - Add SPDX copyright header to skiplist_ordered_index.c - Remove spurious empty lines in skiplist_ordered_index.c - Fix test header to say 'active implementation' not 'active backend' Signed-off-by: Rain Valentine <rainval@amazon.com>
- skiplist.c: reformat one-liner if statements (clang-format-check) - test_ordered_index.cpp: use NEG_INF/POS_INF instead of bare INFINITY to avoid -Wdouble-promotion on macOS where INFINITY is float - test_ordered_index.cpp: use TEST_ASSERT_SCORE_EQ for score comparisons to avoid -Wfloat-equal from gtest ASSERT_EQ on doubles - test_ordered_index.cpp: free items in on_delete callbacks to fix LeakSanitizer failures (callback takes ownership per API contract) Signed-off-by: Rain Valentine <rsg000@gmail.com> Signed-off-by: Rain Valentine <rainval@amazon.com>
- Expand 3 one-liner if/else blocks in skiplist_ordered_index.c to satisfy clang-format-18 - Use TEST_ASSERT_SCORE_EQ instead of ASSERT_EQ for double comparison in RandomizedForwardBackwardMirror test (gtest 1.17 on macOS triggers -Werror,-Wfloat-equal) Signed-off-by: Rain Valentine <rainval@amazon.com>
Convert all sorted set command implementations in t_zset.c to use the OrderedIndex abstraction instead of direct skiplist calls. Changes: - zset struct: zskiplist *zsl -> OrderedIndex *oi (in server.h) - All zsl* calls in t_zset.c replaced with orderedIndex* equivalents - Range iteration converted to OrderedIndex iterator API - Range deletion uses on_delete callback (zsetIndexDeleteCallback) - orderedIndexCountScoreRange/LexRange replaces rank-difference counting - orderedIndexGetElementRaw used for element access (cast to sds where needed for hashtable compatibility) - Other files (geo.c, defrag.c, module.c, etc.) use (zskiplist *) cast on zs->oi until converted in subsequent PRs Bug fixes: - zslDetachNode: fixed search to use zslCompareNodes (score-based) instead of pointer equality which overshoots at higher skiplist levels - skiplistDeleteRangeBy*: fixed double-free when on_delete callback is provided (backend no longer frees after invoking callback) ZUNION/ZINTER/ZDIFF detached-item path still uses raw skiplist calls with casts — will be converted in a follow-up commit. All 326 zset integration tests pass. Signed-off-by: Rain Valentine <rsg000@gmail.com>
Convert the ZUNION SET_OP_UNION path from raw skiplist calls to the OrderedIndex detached-item API: - zslCreateNode + zslRandomLevel -> orderedIndexCreateDetached - Direct node->score mutation -> orderedIndexDetachedSetScore - zslInsertNode bulk insert -> orderedIndexInsertDetached Convert the zuiNext/zuiInitIterator skiplist path from raw backward pointer traversal to OrderedIndex iterator (orderedIndexPrev). After this commit, t_zset.c has zero references to zskiplistNode, zskiplist, or any zsl* function (except range-parsing helpers which operate on zrangespec/zlexrangespec, not the data structure). All 326 integration tests and all unit tests pass. Signed-off-by: Rain Valentine <rsg000@gmail.com>
Convert module.c, object.c, rdb.c, aof.c, geo.c, sort.c, debug.c, db.c, server.c, lazyfree.c, defrag.c, and valkey-check-rdb.c to use the OrderedIndex abstraction instead of direct skiplist access. Add orderedIndexItemNext/Prev to the interface for item-level navigation (needed by module API which stores current position across calls). After this commit, zero raw skiplist references remain outside of: - skiplist.c/skiplist.h (the implementation) - skiplist_ordered_index.c (the backend adapter) - defrag.c (skiplist-internal node relocation, inherently impl-specific) - t_zset.c range utilities (zslParseRange, zslValueGteMin, etc. operate on zrangespec structs, not the skiplist data structure) Signed-off-by: Rain Valentine <rsg000@gmail.com> Signed-off-by: Rain Valentine <rainval@amazon.com>
Add integration tests exercising code paths that changed during the OrderedIndex abstraction, specifically for skiplist-encoded sorted sets. tests/unit/sort.tcl: - SORT BY nosort with skiplist encoding (asc, desc) - SORT BY nosort + LIMIT with skiplist encoding - SORT with BY pattern on skiplist encoding tests/unit/type/zset.tcl: - ZUNIONSTORE with skiplist inputs (basic + WEIGHTS) - ZINTERSTORE with skiplist inputs (basic + AGGREGATE MIN) - ZRANGEBYSCORE with LIMIT on skiplist sets - ZRANGEBYLEX with LIMIT on skiplist sets - ZPOPMIN/ZPOPMAX on skiplist sets (single + multiple) - ZPOPMIN/ZPOPMAX empty skiplist set - ZCOUNT on skiplist sets - ZLEXCOUNT on skiplist sets Signed-off-by: Rain Valentine <rsg000@gmail.com> Signed-off-by: Rain Valentine <rainval@amazon.com>
The ZUNIONSTORE/ZINTERSTORE tests use multi-key operations (DEL src1 src2 dst, ZUNIONSTORE dst 2 src1 src2) that hit different hash slots in cluster mode, causing CROSSSLOT errors. Skip these tests in external-cluster CI. Signed-off-by: Rain Valentine <rainval@amazon.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces an ChangesOrderedIndex Abstraction, Skiplist Implementation, and ZSET Migration
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## zset-btree #3840 +/- ##
==============================================
+ Coverage 76.92% 77.08% +0.16%
==============================================
Files 164 168 +4
Lines 80707 82692 +1985
==============================================
+ Hits 62082 63743 +1661
- Misses 18625 18949 +324
🚀 New features to boost your workflow:
|
orderedIndexGetElementRaw returns an opaque pointer+length pair. Several call sites incorrectly cast the result to sds and called sdslen() on it, violating the interface abstraction. Fix all category 1 violations (pure abstraction correctness): t_zset.c: - zsetTypeRandomElement: use ele_len_tmp directly - zsetHashtableGetMaxElementLength: use ele_len_tmp directly - ZUNION element tracking: use ele_len_tmp directly - ZRANDMEMBER reply (2 sites): use addReplyBulkCBuffer(c, ptr, len) db.c: - hashtableScanCallback: track zset_ele_len for pattern matching, use sdsnewlen(ptr, len) instead of sdsdup((sds)ptr) for the copy Signed-off-by: Rain Valentine <rainval@amazon.com>
rainsupreme
force-pushed
the
oi/pr2-interface-and-conversion
branch
from
dbcb878 to
8b9797c
Compare
Replace the skiplist-internal defrag code (activeDefragZsetNode, which manually walked skiplist levels to relink pointers) with the interface method orderedIndexScanDefrag. The backend handles node relocation internally; the callback just updates the hashtable pointer via hashtableReplaceReallocatedEntry. This removes the last skiplist dependency from defrag.c: - Remove #include "skiplist.h" - Remove zslUpdateNode helper - Remove activeDefragZsetNode (40 lines of skiplist internals) - Replace with 5-line defragZsetNodeCallback using the interface Signed-off-by: Rain Valentine <rainval@amazon.com>
No code outside skiplist.c/skiplist_ordered_index.c references zskiplist directly anymore. The forward declaration in server.h is dead code. Signed-off-by: Rain Valentine <rainval@amazon.com>
Match the orderedIndexNext/Prev interface convention: return the node pointer (NULL = end) instead of bool + out-param. This simplifies the adapter to a direct cast. Signed-off-by: Rain Valentine <rainval@amazon.com>
Replace the raw oi->level access (which dereferences an opaque type) with a call to skiplistGetHeight via a local extern declaration. This makes the dependency explicit and compiles correctly regardless of UNSAFE_CRASH_REPORT. Signed-off-by: Rain Valentine <rainval@amazon.com>
No bool types remain in the header after zslNext/zslPrev signature change. Signed-off-by: Rain Valentine <rainval@amazon.com>
Remove the early NULL check and do-while pattern. A plain while loop naturally short-circuits when orderedIndexNext returns NULL, producing the same return value (0) with less code. Signed-off-by: Rain Valentine <rainval@amazon.com>
Signed-off-by: Rain Valentine <rainval@amazon.com>
Replace all comment references to 'skiplist' with 'ordered index' to reflect the current abstraction. The skiplist.h include remains because t_zset.c uses range utility functions (zslParseRange, zslValueGteMin, etc.) that operate on zrangespec structs. Signed-off-by: Rain Valentine <rainval@amazon.com>
Move zset range comparison functions out of skiplist.c into t_zset.c and rename with zset prefix since they are generic utilities used by both listpack and ordered index encodings: zslValueGteMin -> zsetScoreGteMin zslValueLteMax -> zsetScoreLteMax sdscmplex -> zsetLexCompare zslLexValueGteMin -> zsetLexGteMin zslLexValueLteMax -> zsetLexLteMax Declarations move from skiplist.h to server.h. This removes the need for t_zset.c, geo.c, and module.c to include skiplist.h. Signed-off-by: Rain Valentine <rainval@amazon.com>
Match the declaration in the header file. Signed-off-by: Rain Valentine <rainval@amazon.com>
Eliminates a temporary sds allocation+free in the ordered index to listpack conversion path. The function only needs a pointer and length for lpAppend/lpInsertString. Signed-off-by: Rain Valentine <rainval@amazon.com>
rainsupreme
force-pushed
the
oi/pr2-interface-and-conversion
branch
3 times, most recently
from
fe75bd4 to
4e0f1ae
Compare
…x unit tests - Add fixture helpers: insert(), populateSequential(), assertElement, assertNextScore/assertPrevScore, assertAllElements, ScopedIter RAII wrapper - Add lex range helpers (deleteLexRange, countLexRange, seekToLexRange) that accept const char* and handle sds lifecycle internally - Replace all insertSds boilerplate with insert() helper - Replace ASSERT_TRUE(x == y) with ASSERT_EQ for better failure diagnostics - Replace raw getElementRaw+memcmp with assertElement helper - Replace inline element arrays with shared FRUITS[]/NATO[] constants - Split multi-scenario DeleteRangeByLexBoundaryCases into 3 focused tests - Run clang-format for consistent style 77 test cases (48 parameterized + 29 fixture) introduced in this PR. Signed-off-by: Rain Valentine <rainval@amazon.com>
rainsupreme
force-pushed
the
oi/pr2-interface-and-conversion
branch
from
4e0f1ae to
1e28aa1
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.c (1)
629-640: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftPreserve the ordered-index
(ptr,len)contract here.Line 632 drops the length returned by
orderedIndexGetElementRaw(), but Lines 638-640 still hash and compare viasdsHashConfigurableSeed/dictSdsKeyCompare. Thesrc/debug.c:223caller already treats this API as raw bytes plus explicit length, so this path now only works if everyOrderedIndexbackend also exposes ansdsbuffer. That bakes a skiplist/SDS storage detail back into the supposedly backend-agnostic interface. Please either keep an explicitsdsaccessor for this hashtable path or make the zset hashtable callbacks operate on(ptr,len)instead of assumingsds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.c` around lines 629 - 640, The current zsetHashtableGetKey drops the length from orderedIndexGetElementRaw and then the hashtable still uses sds-based sdsHashConfigurableSeed/dictSdsKeyCompare, which assumes sds storage; instead implement hashtable callbacks that preserve the (ptr,len) contract: replace entryGetKey usage with new functions (e.g., zsetHashtableHash and zsetHashtableKeyCompare) that call orderedIndexGetElementRaw((const OrderedIndexItem *)element, &ptr, &len) and then compute the hash and equality using the raw byte buffer+length (or call a raw-bytes hash/compare helper), and set zsetHashtableType.hashFunction and .keyCompare to those new functions while removing the sds assumption from zsetHashtableGetKey/entryGetKey. Ensure orderedIndexGetElementRaw, zsetHashtableGetKey, sdsHashConfigurableSeed and dictSdsKeyCompare are referenced so reviewers can find and update the related code paths.
🧹 Nitpick comments (4)
src/unit/test_ordered_index.cpp (1)
1501-1514: 🏗️ Heavy liftKeep callback/range-delete tests backend-agnostic.
These fixtures hardcode
SkiplistOrderedIndexandskiplistGetElementRaw, so future ordered-index backends won’t be covered by these callback-heavy paths. Consider parameterizing these fixtures onOrderedIndexTestApi*(likeOrderedIndexTest) and using API-level accessors in callbacks.Also applies to: 1835-1847
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/unit/test_ordered_index.cpp` around lines 1501 - 1514, The tests hardcode SkiplistOrderedIndex and skiplistGetElementRaw (see testOnDeleteCallback and OnDeleteCallbackTest) which prevents exercising other backends; change the fixture to accept an OrderedIndexTestApi* (or use the existing OrderedIndexTest parameterized API) instead of SkiplistOrderedIndex, store its pointer in the fixture, and update testOnDeleteCallback to call API-level accessors (e.g., OrderedIndexTestApi::GetElementRaw or equivalent) and the API's free routine instead of skiplistGetElementRaw/orderedIndexFreeItem so the callbacks become backend-agnostic.src/server.h (1)
3369-3374: ⚡ Quick winAdd contract comments for the shared ZSET range helpers.
These helpers are now the common comparison API across encodings, but the declarations do not spell out the tricky parts: that
valueis length-delimited/binary-safe, and thatzlexrangespec::{min,max}may use sentinel values likeshared.minstring/shared.maxstring. That contract belongs at the declaration site.📝 Suggested header docs
-/* Zset range comparison utilities (used by both listpack and ordered index encodings) */ +/* Zset range comparison utilities shared by listpack and ordered-index encodings. + * `value` is binary-safe and length-delimited. `spec->min` / `spec->max` may be + * regular SDS values or the shared min/max sentinels. */ int zsetScoreGteMin(double value, zrangespec *spec); int zsetScoreLteMax(double value, zrangespec *spec); int zsetLexCompare(const char *a, size_t alen, sds b); int zsetLexGteMin(const char *value, size_t len, zlexrangespec *spec); int zsetLexLteMax(const char *value, size_t len, zlexrangespec *spec);As per coding guidelines, "Document why code exists, not just what it does; document all functions in C code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.h` around lines 3369 - 3374, Add clear contract comments above the shared ZSET range helper declarations (zsetScoreGteMin, zsetScoreLteMax, zsetLexCompare, zsetLexGteMin, zsetLexLteMax) describing (1) that the 'value'/'a'/'value' parameters are length-delimited and binary-safe (use the provided length parameters, may contain NULs), (2) that zlexrangespec::min and ::max can be sentinel pointers (e.g., shared.minstring/shared.maxstring) and must be interpreted accordingly, and (3) the expected return semantics (what non-zero/zero means) and any preconditions on zrangespec/zlexrangespec being non-NULL. Place these comments at the declaration site so all encodings using these APIs see the contract.src/skiplist.h (1)
159-178: ⚡ Quick winDocument iterator and detach-node contracts.
This adds a sizeable public API surface, but the header still leaves the important parts implicit: iterator start/reset semantics, whether structural mutations invalidate an iterator, and whether a node returned by
zslDetachNode()becomes caller-owned and must later be freed. Those are the bits future callers are most likely to get wrong.📝 Suggested header docs
/* Iterator */ typedef struct { zskiplist *zsl; /* The skiplist being iterated */ zskiplistNode *node; /* Current node (NULL before first call) */ } zslIter; +/* Initializes an iterator positioned before the first element. + * Any structural mutation of `zsl` invalidates the iterator state. */ void zslInitIterator(zslIter *iter, zskiplist *zsl); + +/* Resets an existing iterator back to the pre-first-element position. */ void zslResetIterator(zslIter *iter); zslIter *zslCreateIterator(zskiplist *zsl); void zslReleaseIterator(zslIter *iter); zskiplistNode *zslNext(zslIter *iter); zskiplistNode *zslPrev(zslIter *iter); @@ /* Additional accessors */ zskiplistNode *zslGetFirst(const zskiplist *zsl); double zslGetScore(const zskiplistNode *node); +/* Unlinks `node` from `zsl` and returns it detached. + * The caller becomes responsible for freeing the returned node. */ zskiplistNode *zslDetachNode(zskiplist *zsl, zskiplistNode *node);As per coding guidelines, "Document why code exists, not just what it does; document all functions in C code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/skiplist.h` around lines 159 - 178, The header declares iterator and detach APIs but lacks contract docs; update the comments above the iterator typedef and functions (zslInitIterator, zslResetIterator, zslCreateIterator, zslReleaseIterator, zslNext, zslPrev, zslSeekToRank, zslSeekToScoreRange, zslSeekToLexRange) to clearly state: iterator start/reset semantics (iterator is positioned before first element until zslNext/zslPrev is called), whether structural mutations (inserts/removes) invalidate the iterator (document that any structural change invalidates existing iterators), and usage/ownership rules for zslDetachNode (detached node is transferred to caller and must be freed by caller; detaching removes node from zsl and updates lengths/levels). Also annotate additional accessors (zslGetFirst, zslGetScore) with their return/value lifetime and whether they may return NULL.src/debug.c (1)
1189-1193: ⚡ Quick winKeep OrderedIndex debug introspection behind the public abstraction.
This function-local
externreintroduces a backend-specific contract intodebug.c. If a second OrderedIndex backend is added, this file still has to know about its private debug symbol, which undercuts the PR’s backend-agnostic interface goal. Please expose a generic debug hook fromordered_index.hor skip the depth field when the backend does not provide it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/debug.c` around lines 1189 - 1193, The debug.c code currently declares a function-local extern orderedIndexGetDepth and directly depends on OrderedIndex internals; instead either add a public debug accessor in ordered_index.h (e.g., declare a function like int orderedIndexGetDepth(OrderedIndex *oi) or a generic orderedIndexDebugDepth(OrderedIndex *oi, int *out_depth)) and call that from debug.c (using the existing OBJ_ENCODING_SKIPLIST check and the zset->oi field), or avoid calling private symbols by skipping/logging depth when the public API does not provide it; update ordered_index.h to expose the chosen accessor and remove the local extern from debug.c so debug.c uses only the public ordered index API.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/defrag.c`:
- Around line 242-245: In defragZsetNodeCallback check the return value of
hashtableReplaceReallocatedEntry and assert it succeeds instead of ignoring it:
after calling hashtableReplaceReallocatedEntry(ht, old_item, new_item) verify it
returned true and if not invoke the project's assertion/panic routine (e.g.
serverAssert/serverPanic) so the code loudly fails rather than leaving zs->ht
pointing at the old OrderedIndexItem; update defragZsetNodeCallback to perform
this check and fail fast on remap failure.
In `@src/ordered_index.h`:
- Around line 66-69: The header says orderedIndexUpdateScore returns NULL when
the item stayed in place but the skiplist backend (skiplist_ordered_index.c)
currently returns the original pointer; update the skiplist implementation of
orderedIndexUpdateScore (function in src/skiplist_ordered_index.c) to return
NULL when the item did not move (i.e., score updated in-place) and only return a
new pointer when the item was repositioned, preserving the existing contract so
callers can detect when companion references must be refreshed.
In `@src/skiplist_ordered_index.c`:
- Around line 519-520: The code unconditionally writes a NUL terminator to
errmsg[0] which is out-of-bounds when errmsg_len == 0; update the placement
after `#undef` FAIL to only write the terminator if the buffer can hold it by
checking errmsg_len (and errmsg if needed) before assigning—i.e., guard the
write with a condition that verifies errmsg_len > 0 so the NUL is only written
when safe (referencing the errmsg and errmsg_len variables and the surrounding
FAIL macro area).
- Around line 11-12: Guard all writes to errmsg in skiplistVerifyIntegrity():
before doing errmsg[0] = '\0'; or calling snprintf() from the FAIL(...) macro,
check that errmsg is non-NULL and errmsg_len > 0 (e.g., if (errmsg && errmsg_len
> 0) { ... }). Update the FAIL(...) macro usage inside skiplistVerifyIntegrity()
to avoid calling snprintf with a NULL or zero-length buffer (branch to skip
formatting or use a safe no-op when errmsg is unavailable). Leave the existing
static_assert line for OrderedIndexIterator as-is.
In `@src/t_zset.c`:
- Around line 1529-1533: The reverse zset init in the OBJ_ENCODING_SKIPLIST
branch risks underflow by calling
orderedIndexSeekToIndex(orderedIndexLength(...) - 1) on an empty index; instead
either skip the explicit seek and rely on orderedIndexPrev() to start from the
end or guard the seek with a length>0 check. Update the code around the
OBJ_ENCODING_SKIPLIST handling (symbols: op->encoding, it->sl.zs,
orderedIndexInitIterator, orderedIndexSeekToIndex, orderedIndexLength,
orderedIndexPrev) to remove the unnecessary seek or add a conditional so no
subtraction is performed when orderedIndexLength(it->sl.zs->oi) == 0.
- Around line 1643-1650: The code stores a borrowed pointer from
orderedIndexGetElementRaw() into val->ele (an sds), which is wrong; instead
duplicate the raw bytes into a proper sds. Replace assigning (sds)val_ele_ptr
with creating an sds from the borrowed bytes (e.g., sdsnewlen(val_ele_ptr,
val_ele_len)) so val->ele is an owned sds; keep using
orderedIndexPrev(&it->sl.iter) / orderedIndexGetElementRaw(...) to get bytes and
length, then sds-allocate a copy and assign that to val->ele, ensuring callers
expect and free an sds.
In `@tests/unit/sort.tcl`:
- Around line 174-220: The tests that modify the zset-max-ziplist-entries config
(the test blocks "SORT sorted set skiplist BY nosort should retain ordering",
"SORT sorted set skiplist BY nosort + LIMIT", and "SORT sorted set skiplist with
BY pattern") must guarantee restoration of the original value even on failure;
capture the original via [lindex [r config get zset-max-ziplist-entries] 1], set
r config set zset-max-ziplist-entries 0 as now, then wrap the body that mutates
the DB (r del/zadd/asserts) in a try/finally (or call a small helper proc) that
executes r config set zset-max-ziplist-entries $original_max in the
finally/cleanup block so the config is always restored regardless of assertion
failures.
In `@tests/unit/type/zset.tcl`:
- Around line 2954-3146: Each skiplist zset test that mutates
zset-max-ziplist-entries (e.g., tests "ZUNIONSTORE with skiplist-encoded
inputs", "ZINTERSTORE with skiplist-encoded inputs", etc.) must ensure
restoration of the original value even on failure: after capturing original_max
and setting r config set zset-max-ziplist-entries 0, wrap the main test body in
a try/finally style pattern using Tcl catch so that in both success and error
paths you always execute r config set zset-max-ziplist-entries $original_max (or
use the test framework's cleanup hook if available); reference the variable
original_max and the command r config set zset-max-ziplist-entries when applying
the fix.
---
Outside diff comments:
In `@src/server.c`:
- Around line 629-640: The current zsetHashtableGetKey drops the length from
orderedIndexGetElementRaw and then the hashtable still uses sds-based
sdsHashConfigurableSeed/dictSdsKeyCompare, which assumes sds storage; instead
implement hashtable callbacks that preserve the (ptr,len) contract: replace
entryGetKey usage with new functions (e.g., zsetHashtableHash and
zsetHashtableKeyCompare) that call orderedIndexGetElementRaw((const
OrderedIndexItem *)element, &ptr, &len) and then compute the hash and equality
using the raw byte buffer+length (or call a raw-bytes hash/compare helper), and
set zsetHashtableType.hashFunction and .keyCompare to those new functions while
removing the sds assumption from zsetHashtableGetKey/entryGetKey. Ensure
orderedIndexGetElementRaw, zsetHashtableGetKey, sdsHashConfigurableSeed and
dictSdsKeyCompare are referenced so reviewers can find and update the related
code paths.
---
Nitpick comments:
In `@src/debug.c`:
- Around line 1189-1193: The debug.c code currently declares a function-local
extern orderedIndexGetDepth and directly depends on OrderedIndex internals;
instead either add a public debug accessor in ordered_index.h (e.g., declare a
function like int orderedIndexGetDepth(OrderedIndex *oi) or a generic
orderedIndexDebugDepth(OrderedIndex *oi, int *out_depth)) and call that from
debug.c (using the existing OBJ_ENCODING_SKIPLIST check and the zset->oi field),
or avoid calling private symbols by skipping/logging depth when the public API
does not provide it; update ordered_index.h to expose the chosen accessor and
remove the local extern from debug.c so debug.c uses only the public ordered
index API.
In `@src/server.h`:
- Around line 3369-3374: Add clear contract comments above the shared ZSET range
helper declarations (zsetScoreGteMin, zsetScoreLteMax, zsetLexCompare,
zsetLexGteMin, zsetLexLteMax) describing (1) that the 'value'/'a'/'value'
parameters are length-delimited and binary-safe (use the provided length
parameters, may contain NULs), (2) that zlexrangespec::min and ::max can be
sentinel pointers (e.g., shared.minstring/shared.maxstring) and must be
interpreted accordingly, and (3) the expected return semantics (what
non-zero/zero means) and any preconditions on zrangespec/zlexrangespec being
non-NULL. Place these comments at the declaration site so all encodings using
these APIs see the contract.
In `@src/skiplist.h`:
- Around line 159-178: The header declares iterator and detach APIs but lacks
contract docs; update the comments above the iterator typedef and functions
(zslInitIterator, zslResetIterator, zslCreateIterator, zslReleaseIterator,
zslNext, zslPrev, zslSeekToRank, zslSeekToScoreRange, zslSeekToLexRange) to
clearly state: iterator start/reset semantics (iterator is positioned before
first element until zslNext/zslPrev is called), whether structural mutations
(inserts/removes) invalidate the iterator (document that any structural change
invalidates existing iterators), and usage/ownership rules for zslDetachNode
(detached node is transferred to caller and must be freed by caller; detaching
removes node from zsl and updates lengths/levels). Also annotate additional
accessors (zslGetFirst, zslGetScore) with their return/value lifetime and
whether they may return NULL.
In `@src/unit/test_ordered_index.cpp`:
- Around line 1501-1514: The tests hardcode SkiplistOrderedIndex and
skiplistGetElementRaw (see testOnDeleteCallback and OnDeleteCallbackTest) which
prevents exercising other backends; change the fixture to accept an
OrderedIndexTestApi* (or use the existing OrderedIndexTest parameterized API)
instead of SkiplistOrderedIndex, store its pointer in the fixture, and update
testOnDeleteCallback to call API-level accessors (e.g.,
OrderedIndexTestApi::GetElementRaw or equivalent) and the API's free routine
instead of skiplistGetElementRaw/orderedIndexFreeItem so the callbacks become
backend-agnostic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 00db8569-69bf-4133-a39a-8c037b09abcb
📒 Files selected for processing (26)
cmake/Modules/SourceFiles.cmakesrc/Makefilesrc/aof.csrc/db.csrc/debug.csrc/defrag.csrc/geo.csrc/lazyfree.csrc/module.csrc/object.csrc/ordered_index.csrc/ordered_index.hsrc/rdb.csrc/server.csrc/server.hsrc/skiplist.csrc/skiplist.hsrc/skiplist_ordered_index.csrc/skiplist_ordered_index.hsrc/sort.csrc/t_zset.csrc/unit/ordered_index_test.hsrc/unit/test_ordered_index.cppsrc/valkey-check-rdb.ctests/unit/sort.tcltests/unit/type/zset.tcl
👮 Files not reviewed due to content moderation or server errors (1)
- src/module.c
Replace C++ patterns with C-style equivalents in deterministic tests: - Remove ScopedIter RAII wrapper; use explicit initIterator/resetIterator - Replace std::initializer_list with C array + variadic macro - Remove default parameter values from lex helper functions - Replace std::mt19937 and std::uniform_*_distribution with xorshift32 PRNG Fuzz/randomized tests retain std::vector/string/set for bookkeeping as rewriting those provides no practical benefit. Signed-off-by: Rain Valentine <rainval@amazon.com>
- Assert hashtableReplaceReallocatedEntry succeeds in defragZsetNodeCallback (matches pattern used by other defrag sites in the same file) - Guard errmsg[0] write with length check in skiplistVerifyIntegrity (prevents out-of-bounds write when errmsg_len == 0) Signed-off-by: Rain Valentine <rainval@amazon.com>
Add a with_config proc to tests/support/util.tcl that wraps a test body with automatic config save/restore, ensuring the original value is restored even if the body raises an error. Convert all skiplist-encoding tests in sort.tcl and zset.tcl to use it. Signed-off-by: Rain Valentine <rainval@amazon.com>
Store ordered index element as estr/elen (borrowed pointer + length) instead of casting to sds. This matches the listpack path and avoids leaking the skiplist's internal sds storage into the generic ordered index interface. Callers that need an sds (zuiSdsFromValue, zuiNewSdsFromValue) will create one from the borrowed bytes, same as they do for listpack. Signed-off-by: Rain Valentine <rainval@amazon.com>
The function always returns a valid pointer (never NULL). Callers can compare old vs returned to detect whether the item was repositioned. Simplify the ZADD caller to unconditionally update the hashtable ref. Signed-off-by: Rain Valentine <rainval@amazon.com>
Fuzz tests now use the framework's --seed flag if provided, defaulting to 42 for deterministic CI. The seed is printed on each fuzz test run so failures can be reproduced with: valkey-unit-gtests --seed <N> Signed-off-by: Rain Valentine <rainval@amazon.com>
…free Change OrderedIndexOnDelete semantics from 'callback takes ownership' to 'notification before free'. The index always frees items after the callback returns. This is simpler and avoids ownership confusion when multiple backends exist. Update skiplist backend to always free the node after calling the callback (previously it skipped the free when callback was provided). Remove orderedIndexFreeItem from zsetIndexDeleteCallback since the index handles freeing. Signed-off-by: Rain Valentine <rsg000@gmail.com> Signed-off-by: Rain Valentine <rainval@amazon.com>
Test that countLexRange, seekToLexRange, and deleteRangeByLex handle the shared.minstring (-inf) and shared.maxstring (+inf) sentinel pointers correctly. These are distinct from literal sds strings with the same content — the interface uses pointer identity to detect them. Initialize shared.minstring/maxstring in the test fixture SetUp since createSharedObjects is not called in the unit test binary. Signed-off-by: Rain Valentine <rsg000@gmail.com> Signed-off-by: Rain Valentine <rainval@amazon.com>
rainsupreme
force-pushed
the
oi/pr2-interface-and-conversion
branch
from
731b01a to
e3fdcbd
Compare
eifrah-aws
reviewed
Jun 11, 2026
Comment on lines
+7
to
+9
| /* OrderedIndex implementation — delegates to the active backend. | ||
| * Currently only the skiplist backend exists. When a B+ tree backend is added, | ||
| * a compile-time or link-time switch will select the implementation. */ |
Contributor
There was a problem hiding this comment.
This entire file is a 1:1 abstraction layer. The cost is another function call. We can achieve the same by placing these methods in a header file marking the functions "static inline" (or even "always inline")
Contributor
Author
There was a problem hiding this comment.
However, the function call is inlined away by LTO, so the end result is the same either way. After discussing with @JimB123 I did it this way as a style choice to avoid forced inlining (or establishing inlining as a codebase pattern that others might follow/reference).
Skiplist will ultimately be removed. Perhaps in the end I'll just use and unit test the adapter directly without a dispatch layer.
madolson
reviewed
Jun 11, 2026
- sdsnew("charlie") was called 3 times inline in assertions without
ever being freed. Use a local sds variable and free it at end.
- shared.minstring/maxstring were allocated in SetUp() but never freed
on process exit. Add destructor function to clean them up.
Signed-off-by: Rain Valentine <rainval@amazon.com>
- t_zset.c: guard orderedIndexSeekToIndex with length > 0 check to prevent unsigned underflow on empty sets (eifrah-aws comment). - module.c: avoid casting away const from orderedIndexGetElementRaw by using ele/ele_len directly with createStringObject and returning early (eifrah-aws const nit). Signed-off-by: Rain Valentine <rainval@amazon.com>
ZPOP was using orderedIndexGetByIndex(len-1) which is O(log n) in skiplist. The old code used direct zsl->tail access which was O(1). Add orderedIndexGetFirst/GetLast to restore O(1) head/tail access through the abstraction layer (eifrah-aws review comment). Signed-off-by: Rain Valentine <rainval@amazon.com>
Replace std::vector, std::string, std::set, std::sort, std::reverse, std::min/max with C-style equivalents: - Fixed-size C arrays (size always known from orderedIndexLength or insert count) instead of std::vector - sds instead of std::string for dynamic strings - snprintf instead of std::to_string - qsort + custom comparator instead of std::sort - Manual swap loop instead of std::reverse - Sorted sds arrays instead of std::set for unordered comparison - ASSERT_SDS_ARRAY_EQ macro for concise array-vs-literal assertions The only remaining std::string is in the GTest name generator function, which is required by the GTest INSTANTIATE_TEST_SUITE_P API. Addresses review comment from madolson regarding C++ construct usage in tests (per previous direction from Viktor). Signed-off-by: Rain Valentine <rainval@amazon.com>
Signed-off-by: Rain Valentine <rainval@amazon.com>
rainsupreme
force-pushed
the
oi/pr2-interface-and-conversion
branch
from
20d9d3c to
a6ec45f
Compare
Replace sdsHashConfigurableSeed/dictSdsKeyCompare with zsetHashFunction/zsetKeyCompare that go through zsetExtractElement to get (ptr, len) before hashing/comparing. Currently zsetExtractElement is trivial (just sdslen + return ptr) since the skiplist backend stores elements as plain sds. The fbtree backend (next PR) will extend it to skip an 8-byte score prefix on packed items using the sds aux bit. Signed-off-by: Rain Valentine <rainval@amazon.com>
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
3 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.
This is the second PR for implementing B+ tree for ZSET (#3166)
This PR:
Introduces the OrderedIndex abstraction — a backend-agnostic interface (
ordered_index.h) with iterator, seek, count, and delete-range operations. Includes a skiplist backend implementation and 77 unit tests (parameterized to run against any backend).Converts all ZSET call sites to use the OrderedIndex interface — t_zset.c, module.c, object.c, rdb.c, aof.c, geo.c, sort.c, db.c, server.c, lazyfree.c, defrag.c, debug.c, and valkey-check-rdb.c. Zero raw skiplist references remain outside the skiplist backend.
Adds 14 integration tests covering skiplist-encoded zset paths exercised through the new interface.
The interface and unit tests serve as a regression guard — when the B+ tree backend is added in a follow-up PR, the same test suite runs against both backends, ensuring behavioral equivalence without duplicating test logic.
After this PR, adding a new ordered index backend (e.g., B+ tree) requires only implementing the interface functions — no changes to any ZSET command logic.