ZSET B+ Tree PR 1: extract skiplist into dedicated module by rainsupreme · Pull Request #3579 · valkey-io/valkey

rainsupreme · 2026-04-28T20:57:14Z

This is PR 1 of the series introducing an abstraction layer over the sorted set skiplist, enabling a future swap to a B+ tree for memory and performance gains. (Issue #3166) It targets the zset-btree feature branch.

Planned PRs:

Skiplist extraction ← this PR
OrderedIndex interface + skiplist backend + unit tests
Convert t_zset.c to OrderedIndex
Convert module.c
Convert remaining files
Integration tests

Summary of changes:

Move the skiplist implementation out of t_zset.c into its own compilation unit (skiplist.c / skiplist.h), establishing a clean module boundary.

New files:

src/skiplist.h — struct definitions (zskiplistNode, zskiplist), inline helpers (span/height accessors), and function declarations
src/skiplist.c — all skiplist data structure operations extracted from t_zset.c

Changes to server.h:

Remove zskiplistNode and zskiplist struct definitions (moved to skiplist.h)
Remove ZSKIPLIST_MAXLEVEL, ZSKIPLIST_MAX_SEARCH defines (moved to skiplist.h)
Remove skiplist function declarations (moved to skiplist.h)
Add forward declaration struct zskiplist for the zset struct pointer

Changes to t_zset.c:

Remove skiplist implementation (now in skiplist.c)
Add #include "skiplist.h"
Retain range parsing helpers (zslParseRange, zslParseLexRangeItem, zsetParseLexRange, zsetFreeLexRange) as command-layer logic

Other files:

Add #include "skiplist.h" to files that dereference skiplist struct fields (defrag.c, geo.c, module.c, object.c, sort.c, aof.c, db.c, debug.c, rdb.c, server.c, lazyfree.c, valkey-check-rdb.c)
Update Makefile and CMakeLists.txt to compile skiplist.c

Functions that were static in t_zset.c and are now needed across translation units are made non-static. No behavioral changes.

rainsupreme · 2026-04-28T21:07:01Z

Preview of PR 2: rainsupreme#1

jjuleslasarte · 2026-05-01T23:34:03Z

+    iter->node = iter->node->backward;
+    if (iter->node == zslGetHeader(iter->zsl) || iter->node == NULL) iter->zsl = NULL;


This permanently "kills" the iterator no?. A subsequent zslNext call returns false. This means a bidirectional iterator cannot reverse direction at boundaries -- should this be a/ handled b/ documented?

That's true, and a good point. I decided to make it not kill the iterator, as this will make it easier to support range operations later. I added comments and UTs for the iterator.

rainsupreme · 2026-05-06T00:03:05Z

@jjuleslasarte thank you so much for the feedback! I've made some fixes and added UTs 😊

codecov · 2026-05-06T00:40:42Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.45%. Comparing base (ff80b2d) to head (480863e).

Additional details and impacted files

@@              Coverage Diff               @@
##           zset-btree    #3579      +/-   ##
==============================================
- Coverage       76.66%   76.45%   -0.22%     
==============================================
  Files             159      161       +2     
  Lines           80114    80111       -3     
==============================================
- Hits            61423    61248     -175     
- Misses          18691    18863     +172

Files with missing lines	Coverage Δ
src/aof.c	`80.37% <ø> (+0.06%)`	⬆️
src/db.c	`94.36% <ø> (ø)`
src/debug.c	`54.83% <ø> (ø)`
src/defrag.c	`81.12% <ø> (-1.13%)`	⬇️
src/geo.c	`94.79% <ø> (-0.42%)`	⬇️
src/lazyfree.c	`88.38% <ø> (ø)`
src/module.c	`25.31% <ø> (ø)`
src/object.c	`92.44% <ø> (+0.48%)`	⬆️
src/rdb.c	`77.54% <ø> (-0.27%)`	⬇️
src/server.c	`89.60% <ø> (+0.13%)`	⬆️
... and 6 more

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

PingXie · 2026-05-10T07:01:01Z

B+ tree is a great idea, and the OrderedIndex abstraction makes sense. I think the main thing to tighten in this first PR is the API boundary and sequencing.

Right now the new API surface is still explicitly skiplist-specific: zsl* names, zskiplist / zskiplistNode types, span/height helpers, node creation helpers, and iterator helpers. When we add the B+ tree backend, we would either need to duplicate this surface with another prefix/type and make callers branch on the concrete backend. That would weaken the abstraction we are trying to introduce.

I think the first step should be to move the skiplist implementation out of t_zset.c and into its own C module. Ideally the skiplist structs should also become opaque, or at least move in that direction. Today production code still depends on details like node->backward, node->level[0].forward, and node->score, while this PR adds new skiplist APIs, including reverse iterator traversal, before those production call sites are converted. So we are committing to future API shape before the abstraction exists and before the production call sites use it

More specifically, I would prefer this PR to establish a clean skiplist module boundary first, and let the next PR introduce the backend-neutral OrderedIndex interface with only the operations required by existing ZSET code. Extra iterator or construction APIs can be added when a real OrderedIndex method or B+ tree integration needs them.

rainsupreme · 2026-05-11T20:34:42Z

Hm, I suppose that would make things cleaner while reviewing PRs until I delete skiplist. I had raised a PR like that a while back to separate skiplist stuff into a module and it was rejected, but that was before I had a feature branch to work in. I'm revising this PR now :)

madolson · 2026-05-12T21:19:42Z

More specifically, I would prefer this PR to establish a clean skiplist module boundary first, and let the next PR introduce the backend-neutral OrderedIndex interface with only the operations required by existing ZSET code. Extra iterator or construction APIs can be added when a real OrderedIndex method or B+ tree integration needs them.

@PingXie I originally suggested to not do this. It's just moving code around for the sake of it until it's ultimately removed. I primarily didn't want to do it for 9.1 since that is unnecessary divergence between the versions. It's a little bit less relevant now that we did the code cut. I'm OK with an abstraction that covers both, but doing the work to make it opaque then delete it seems like unecessary ask.

Move the skiplist implementation out of t_zset.c into its own compilation unit. This establishes a clean module boundary in preparation for the OrderedIndex abstraction. Changes: - Create src/skiplist.c with all skiplist data structure operations - Create src/skiplist.h with struct definitions, inline helpers, and function declarations - Remove struct definitions and skiplist function declarations from server.h (replaced with forward declarations) - Add #include "skiplist.h" to files that dereference skiplist structs - Update Makefile and CMakeLists.txt Functions that were static in t_zset.c and are now needed across translation units (zslCreateNode, zslInsertNode, zslDeleteNode, zslDelete, zslFreeNode, zslRandomLevel, zslUpdateScore, zslDeleteRangeByScore/Lex/Rank, zslGetRank) are made non-static. Range parsing helpers (zslParseRange, zslParseLexRangeItem, zsetParseLexRange, zsetFreeLexRange) remain in t_zset.c as they are command-layer logic. No behavioral changes. All existing tests pass. Signed-off-by: Rain Valentine <rsg000@gmail.com>

coderabbitai · 2026-05-12T22:14:45Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 282089b3-47f7-4114-9ad1-928420b5e6aa

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

Generate code and open pull requests
Plan features and break down work
Investigate incidents and troubleshoot customer tickets together
Automate recurring tasks and respond to alerts with triggers
Summarize progress and report instantly

Built for teams:

Shared memory across your entire org—no repeating context
Per-thread sandboxes to safely plan and execute work
Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

rainsupreme · 2026-05-12T22:24:12Z

Well, I already reorganized it, and I did somewhat prefer to do it that way anyway, so in the end I have no complaints. I hope it makes the PRs more approachable and easier to review. @PingXie what do you think?

And, this is in a feature branch, so no worries about getting stuck with partial work in unstable either. ;)

PingXie

I wasn't aware of the timeline on when we will remove skiplist but this looks great. Thanks @rainsupreme

github-actions Bot assigned rainsupreme Apr 28, 2026

rainsupreme requested review from PingXie and zuiderkwast April 28, 2026 21:21

jjuleslasarte reviewed May 1, 2026

View reviewed changes

rainsupreme force-pushed the oi/pr1-skiplist-refactor branch from 46f3b20 to 480863e Compare May 12, 2026 22:14

rainsupreme changed the title ~~ZSET B+ Tree PR 1: extract internal skiplist helpers, add iterator and accessors~~ ZSET B+ Tree PR 1: extract skiplist into dedicated module May 12, 2026

PingXie approved these changes May 18, 2026

View reviewed changes

		iter->node = iter->node->backward;
		if (iter->node == zslGetHeader(iter->zsl) \|\| iter->node == NULL) iter->zsl = NULL;

Conversation

rainsupreme commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Planned PRs:

Summary of changes:

Uh oh!

rainsupreme commented Apr 28, 2026

Uh oh!

Uh oh!

jjuleslasarte May 1, 2026

Choose a reason for hiding this comment

Uh oh!

rainsupreme May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rainsupreme commented May 6, 2026

Uh oh!

codecov Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PingXie commented May 10, 2026

Uh oh!

rainsupreme commented May 11, 2026

Uh oh!

madolson commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented May 12, 2026

Review skipped

Uh oh!

rainsupreme commented May 12, 2026

Uh oh!

PingXie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rainsupreme commented Apr 28, 2026 •

edited

Loading

codecov Bot commented May 6, 2026 •

edited

Loading

madolson commented May 12, 2026 •

edited

Loading