Skip to content

feat: add proper RDB serialization support for SortedInt type#3367

Closed
Zakir032002 wants to merge 2 commits intoapache:unstablefrom
Zakir032002:feature/sortedint-dump-proper
Closed

feat: add proper RDB serialization support for SortedInt type#3367
Zakir032002 wants to merge 2 commits intoapache:unstablefrom
Zakir032002:feature/sortedint-dump-proper

Conversation

@Zakir032002
Copy link
Contributor

📝 FULL PR DESCRIPTION (Complete Version with Tests)

## Summary
Add proper RDB serialization support for SortedInt type with full type preservation.

Alternative approach to #3366. Both approaches are valid - this PR provides full type preservation for internal Kvrocks operations.

Closes #3319

## Changes
- Implement custom RDB type 22 for SortedInt (Kvrocks-specific)
- Add `SaveSortedintObject()` and `LoadSortedintObject()` methods
- Update `RedisObjValue` variant to support `std::vector<uint64_t>`
- Integrate SortedInt into DUMP/RESTORE flow with proper type handling

## Approach Comparison

| Aspect | PR #3366 (Coercion) | This PR (Proper Format) |
|--------|---------------------|-------------------------|
| **DUMP works** | ✅ Yes | ✅ Yes |
| **RESTORE works** | ✅ Yes | ✅ Yes |
| **Type preserved** | ❌ Becomes "set" | ✅ Stays "sortedint" |
| **Commands work after** | ❌ SICARD/etc fail | ✅ All SI* commands work |
| **Redis compatible** | ✅ Yes (Set format) | ❌ No (type 22) |
| **Data preserved** | ✅ Yes | ✅ Yes |
| **Use case** | Cross-system migration | Internal Kvrocks operations |

### Why Two Approaches?

**Approach 1 (Coercion)**: Follows the existing Bitmap→String pattern. Simple, Redis-compatible, but loses type information.

**Approach 2 (This PR)**: Proper RDB format with custom type code. Complete type preservation, all functionality intact after restore. Matches the pattern shown in issue #3319 where Redis preserves BloomFilter as BloomFilter (not as another type).

## Implementation Details

**Custom RDB Type 22**:
- Format: `[type:22][length][id1][id2]...[idN]`
- Each ID stored as RDB-encoded string
- Compatible with existing RDB infrastructure

**Integration Points**:
- `SaveObjectType()`: Maps `kRedisSortedint``RDBTypeSortedint` (22)
- `SaveObject()`: Retrieves all IDs using `RangeByValue()` and serializes
- `loadRdbObject()`: Deserializes IDs from RDB stream
- `saveRdbObject()`: Creates SortedInt using `Add()` method

## Testing

### Manual Testing - Full DUMP/RESTORE Cycle

**Test 1: Basic DUMP/RESTORE with Type Preservation**
```bash
127.0.0.1:6666> SIADD testkey 111 222 333
(integer) 3

127.0.0.1:6666> TYPE testkey
sortedint

127.0.0.1:6666> DUMP testkey
"\x16\x03\xc0o\xc1\xde\x00\xc1M\x01\x06\x00}..."

127.0.0.1:6666> RESTORE restoredkey 0 "\x16\x03\xc0o\xc1\xde\x00\xc1M\x01\x06\x00}..."
OK

127.0.0.1:6666> TYPE restoredkey
sortedint                    # ✅ Type preserved (not "set")!
Test 2: Verify Data Integrity

bash
127.0.0.1:6666> SICARD restoredkey
(integer) 3                  # ✅ Count correct

127.0.0.1:6666> SIEXISTS restoredkey 111 222 333
1) (integer) 1
2) (integer) 1
3) (integer) 1               # ✅ All original values exist

127.0.0.1:6666> SIRANGEBYVALUE restoredkey 0 1000 LIMIT 0 10
1) "111"
2) "222"
3) "333"                     # ✅ All data retrieved correctly
Test 3: Verify Full Functionality After Restore

bash
127.0.0.1:6666> SIADD restoredkey 444 555
(integer) 2                  # ✅ Can add new values

127.0.0.1:6666> SICARD restoredkey
(integer) 5                  # ✅ Count updated

127.0.0.1:6666> SIEXISTS restoredkey 111 222 333 444 555
1) (integer) 1
2) (integer) 1
3) (integer) 1
4) (integer) 1
5) (integer) 1               # ✅ All values exist (old + new)

127.0.0.1:6666> SIRANGEBYVALUE restoredkey 0 1000 LIMIT 0 10
1) "111"
2) "222"
3) "333"
4) "444"
5) "555"                     # ✅ Complete dataset verified

- Implement custom RDB type 22 for SortedInt
- Add SaveSortedintObject() and LoadSortedintObject()
- Preserve SortedInt type in DUMP/RESTORE cycle
- Update RedisObjValue variant to support vector<uint64_t>
- All SortedInt commands work after RESTORE

This provides full type preservation as an alternative to the
coercion approach in PR apache#3366.

Closes apache#3319
@Zakir032002 Zakir032002 marked this pull request as ready for review February 8, 2026 03:53
@PragmaTwice
Copy link
Member

Hi, thank you for your contribution.

Please read our Guidelines for AI-assisted Contributions before we proceed with this PR : )

@Zakir032002
Copy link
Contributor Author

Thanks for sharing the guidelines @PragmaTwice .

I confirm that I used AI tools as an assistant for review, and improving the clarity of the PR description. All design decisions, code implementation, testing, and validation were done by me, and I fully understand and can justify the changes in this PR.
Im happy to revise the approach or split/adjust the patch based feedback.

@Zakir032002
Copy link
Contributor Author

Zakir032002 commented Feb 8, 2026

I’ve previously worked on a small Raft-based KV store with RocksDB-backed persistence (snapshots and log compaction), which gives me some context around serialization and recovery, but I’m very much here to learn Kvrocks’ design choices and would appreciate guidance on not only this approach or any other future contributions aligns with project expectations.

@PragmaTwice PragmaTwice requested a review from git-hulk February 8, 2026 06:46
@PragmaTwice PragmaTwice changed the title feat: Feature/sortedint dump proper feat: add proper RDB serialization support for SortedInt type Feb 8, 2026
@PragmaTwice
Copy link
Member

PragmaTwice commented Feb 9, 2026

Please don't open two same-purpose PRs for one task. You should open an issue and discuss with community members instead.

@PragmaTwice PragmaTwice closed this Feb 9, 2026
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.

2 participants