Skip to content

refactor: give encoded ZSet pages a dedicated loader - #360

Merged
thweetkomputer merged 1 commit into
eloqdata:mainfrom
thweetkomputer:perf/zset-explicit-loader-20261019
Oct 19, 2026
Merged

thweetkomputer merged 1 commit into
eloqdata:mainfrom
thweetkomputer:perf/zset-explicit-loader-20261019

Conversation

@thweetkomputer

@thweetkomputer thweetkomputer commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Context

LoadOrderedGroupSnapshot accepted a ZSet-specific boolean that changed its return representation: callers could receive either owned member strings or an encoded page with empty entries_. That choice was hidden in a loader shared by List, String, Stream, ZSet and maintenance paths.

Behavior before and after

The common loader now always returns materialized owned entries. Single-member ZSet writes explicitly call LoadEncodedSortedSetGroupSnapshot to obtain a validated encoded page. Existing command behavior and page formats are preserved.

Implementation

  • Remove the encoded-page boolean from LoadOrderedGroupSnapshot.
  • Add the named ZSet loader and select it explicitly at both point-write loading sites.
  • Reuse LoadOrderedGroup<Result, Decode> for physical I/O, version checks, GC retries and lease handling. The new loader always performs full member validation and verifies score bounds.
  • Update Bycorf from 587a9e776f9f64729456d6772596d53597f4a983 to 08fa44dad5c695f6b2bfedc47e37708c1eeaffae, which migrates software regression tests to GoogleTest (Convert software regressions to individual GoogleTest cases bycorf#18).

Design decisions and alternatives

Separate function names expose the ownership representation without duplicating the shared I/O lifecycle. Encoded snapshots retain the existing worker-local ownership contract.

Documentation and comments

The new API comment explicitly states that encoded_sorted_set_ is populated, entries_ is empty, and snapshots remain on the key owner worker. The existing architectural model remains accurate.

Test plan

  • Pinned clang-format hook and git diff --check: passed.
  • GCC 13 Debug and Release/SPDK builds passed for head 9a3d375c with Bycorf 08fa44da.
  • 190 selected Lavik tests passed: 132 codec/collection cases, 11 local-ownership/affinity cases, 13 compact write cases, and 34 grouped ZSet/demotion cases.
  • 49 Bycorf software tests passed across the new unit, task and runtime GoogleTest targets. CTest discovery and JUnit results matched, with no failures or skips.
  • 239 tests passed in total, completed on 2026-10-19 at 14:08 UTC.

Risk assessment

The main risk is selecting the wrong representation at a ZSet call site. Both point-write sites now name the encoded loader explicitly; other callers retain the existing materialized loader. Regression coverage exercises point edits, fallback, ownership, TTL and recovery.

Rollback plan

Revert this PR; no data conversion is needed.

Reviewer guide

Review the two loader contracts in impl.h, their shared-I/O adapters in ordered_io.cpp, and the two selections in zset.cpp. The Bycorf gitlink is a separate dependency refresh.

Follow-up work

None required for this refactor.

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c3a7bbbb-1431-4cef-b216-6cc65c5cd768
📥 Commits

Reviewing files that changed from the base of the PR and between 6e09bed and 9a3d375.

📒 Files selected for processing: 4
  • bycorf
  • src/storage/engine/grouped/ordered_io.cpp
  • src/storage/engine/grouped/zset.cpp
  • src/storage/engine/impl.h

Walkthrough

The change separates regular ordered-group snapshots from encoded sorted-set snapshots. Sorted-set point-write reads and mutation planning now use the encoded snapshot loader. The bycorf subproject reference also advances to a new commit.

Changes

Grouped Sorted-Set Snapshot Loading

Layer / File(s) Summary
Snapshot loader contracts and decoding
src/storage/engine/impl.h, src/storage/engine/grouped/ordered_io.cpp
LoadOrderedGroupSnapshot now decodes ordered groups and checks sorted-set bounds from decoded entries. LoadEncodedSortedSetGroupSnapshot decodes encoded sorted-set groups and checks their endpoint scores against route bounds.
Point-write loader routing
src/storage/engine/grouped/zset.cpp
Point-write page reads and mutation planning use LoadEncodedSortedSetGroupSnapshot. Other cases use LoadOrderedGroupSnapshot.

bycorf Subproject Reference

Layer / File(s) Summary
Subproject reference update
bycorf
The subproject reference changes from commit 587a9e776f9f64729456d6772596d53597f4a983 to 08fa44dad5c695f6b2bfedc47e37708c1eeaffae.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Refactor

Merge Risk: ⚪ Minimal · up to 9a3d3

No actionable issue is established. Merge after the planned build and test checks complete.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: adding a dedicated loader for encoded ZSet pages.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The pull request description follows all required sections and clearly explains the context, behavior change, implementation, design decisions, documentation impact, testing, risks, rollback, reviewer…
Full details: Description check

Explanation

The pull request description follows all required sections and clearly explains the context, behavior change, implementation, design decisions, documentation impact, testing, risks, rollback, reviewer guidance, and follow-up work. The test plan reports results and dates, but it does not list the exact commands used.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit checks the sorted scores,
Then hops through freshly split loader doors.
Encoded groups pass bounds with care,
While point writes find the path they share.
A new commit rests in bycorf’s burrow,
And all the rows line up tomorrow.

Comment @coderabbitai help to get the list of available commands.

@thweetkomputer
thweetkomputer merged commit bd78f9f into eloqdata:main Oct 19, 2026
20 checks passed
@thweetkomputer
thweetkomputer deleted the perf/zset-explicit-loader-20261019 branch October 19, 2026 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant