Rust Implementation of RSSortingVector - MOD-9985 by DarthB · Pull Request #6311 · RediSearch/RediSearch

DarthB · 2025-06-13T08:03:29Z

Describe the changes in the pull request

The PR introduces two changes:
Adds RSSortingVector Rust implementation + Unit Tests

More detailed changes

Implements the RSSortingVector as a Boxed Slice in Rust
Extracts C-Code normalizeStr which implements utf case folding based on nulibI
Add an RSValueTrait that is implemented by mocks and the final FFI implementation
Adds unit tests with mock type of RSValue

Referenced PRs

Previous PRs:

Relies on Refactorings in: MOD-9985 Refactoring of C Code for the RSSortingVector Port #6312
Relies on API changes in: String normalization becomes caller responsibility - MOD-9985 #6426
String normalization becomes caller's responsibility in the C API for RSSortingVector

Upcoming PRs:

FFI Layer: FFI Layer: Implement RSValueTrait for FFI and implement the C entry point - MOD-9985 #6459
Full swap of C with Rust Implementation: Swap the RSSortingVector Implementation - MOD-10714 | MOD-9985 #6397

Archived PRs:

Draft for a new-type usable to ensure sortable strings are normalized in: Type for normalized (utf case folded) Strings in Rust - MOD-9985 #6427
Implement the new type NormalizedString in Rust to ensure normalized Strings in RSSortingVector
Old Linking approach: MOD-9985 Bindings of C types to allow Rust Unit tests for RSSortingVector #6342

…ssues

JonasKruckenberg · 2025-07-03T15:05:38Z

src/redisearch_rs/sorting_vector/src/lib.rs

+        unimplemented!(
+            "We cannot yet use `NormalizedString` here, as the RSValue string allocations still happen\
+                in the C layer and the workaround for linking C code for allocations is not ready."


Suggested change

unimplemented!(

"We cannot yet use `NormalizedString` here, as the RSValue string allocations still happen\

in the C layer and the workaround for linking C code for allocations is not ready."

unimplemented!(

"We cannot yet allocate RSValues in Rust. See MOD-10347 for details."

LukeMathWalker · 2025-07-04T06:30:22Z

src/redisearch_rs/sorting_vector/src/lib.rs

+    pub fn try_insert_string_for_tests<S: AsRef<str>>(
+        &mut self,
+        _idx: usize,
+        _str: S,
+    ) -> Result<(), IndexOutOfBounds> {
+        self.in_bounds(_idx)?;
+        let casemapper = CaseMapper::new();
+        let normalized = casemapper.fold_string(_str.as_ref()).into_owned();
+        self.values[_idx] = T::create_string(normalized);
+        Ok(())
+    }


Can we avoid mixing test helpers with production code?

Due to RSValue and its C-side string allocations, this is a temporary fix. Nonetheless, I eliminated this second method.

386aec0

Copilot

Pull Request Overview

This PR adds a Rust-based RSSortingVector implementation with string normalization support, defines an RSValueTrait abstraction for value handling, and integrates C header updates and unit tests.

Introduces RSSortingVector<T> in Rust with methods for inserting numbers, strings (with ICU4X case folding), references, and nulls
Defines RSValueTrait to abstract over RSValue creation and inspection for both mocks and FFI
Updates src/sortable.h with normalizeStr prototype, extends the FFI build, and adds comprehensive unit tests

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/sortable.h	Added documentation and prototype for `normalizeStr`
src/redisearch_rs/value/src/lib.rs	Defined the `RSValueTrait` trait
src/redisearch_rs/value/Cargo.toml	Configured the new `value` crate in the workspace
src/redisearch_rs/sorting_vector/src/lib.rs	Implemented `RSSortingVector` and its APIs
src/redisearch_rs/sorting_vector/tests/sorting_vector.rs	Added unit tests covering creation, insertion, memory sizing, and normalization
src/redisearch_rs/sorting_vector/Cargo.toml	Configured the `sorting_vector` crate dependencies
src/redisearch_rs/ffi/build.rs	Included `sortable.h` and `value.h` in the bindgen build
src/redisearch_rs/Cargo.toml	Registered `sorting_vector` and `value` crates in the workspace

Comments suppressed due to low confidence (3)

src/redisearch_rs/sorting_vector/src/lib.rs:79

The doc reference Error::OutOfBounds does not match the actual error type IndexOutOfBounds. Update the link to [IndexOutOfBounds].

    /// Returns `Ok(())` if the index is in bounds, [`Error::OutOfBounds`] otherwise.

src/redisearch_rs/sorting_vector/src/lib.rs:115

[nitpick] Parameter name str shadows the built-in str type. Consider renaming it (e.g., input or value) for clarity.

        str: S,

src/redisearch_rs/sorting_vector/tests/sorting_vector.rs:187

[nitpick] Variable name str shadows the built-in str type. Consider renaming (e.g., input_str or original).

    let str = "Straße";

src/sortable.h

src/redisearch_rs/sorting_vector/tests/sorting_vector.rs

raz-mon

LGTM basically
A couple of questions:

I guess it would be good to have as full an API as possible at this point, such that we can test the most and add less additions later on when the RSValue is supported. That would basically require an extended mock for the RSValue - do you think that's worthwhile, or prefer to have bigger additions to the API and tests later on?
Some comments relate to the previous code. While that is comfortable for the review and the transition, we should at some point remove them. Probably the best time would be prior to merging, so that we don't forget it like that.

raz-mon · 2025-07-08T14:57:27Z

src/redisearch_rs/sorting_vector/src/lib.rs

+                sz += T::mem_size();
+            }
+
+            // the original behavior would by-pass references in the middle of the chain


When will the comments about the previous implementations be removed?

raz-mon · 2025-07-08T14:59:59Z

src/redisearch_rs/sorting_vector/src/lib.rs

+            }
+            if T::is_ptr_type() {
+                // count the size of the struct if not null, like in C
+                sz += T::mem_size();


Isn't this double-counting for the second case above?

DarthB · 2025-07-09T08:49:42Z

I guess it would be good to have as full an API as possible at this point, such that we can test the most and add less additions later on when the RSValue is supported. That would basically require an extended mock for the RSValue - do you think that's worthwhile, or prefer to have bigger additions to the API and tests later on?

I checked the RSValue API and propose the following:

add increment and decrement to the trait in this PR (we will need it soon for RLookupRow)
Have a follow-up PR for the methods that are needed in RLookup

We plan to port the RSValue type in MOD-10347 and attempt to minimize the workload associated with that trait workaround.

Some comments relate to the previous code. While that is comfortable for the review and the transition, we should at some point remove them. Probably the best time would be prior to merging, so we don't forget it.

I would like to address this size calculation when we port RSValue - MOD-10347.

raz-mon

LGTM
Note the rdbLoad function as well, that is not implemented here

github-actions bot added the size:M label Jun 13, 2025

DarthB force-pushed the rp-rssortingvector branch 2 times, most recently from 75fb169 to 50e444c Compare June 13, 2025 14:28

github-actions bot added the size:L label Jun 13, 2025

DarthB force-pushed the rp-rssortingvector branch 2 times, most recently from b15859c to a9b46e6 Compare June 13, 2025 14:59

DarthB changed the title ~~Rp rssortingvector~~ MOD-9985 - Rust Implementation of RPSortingVector Jun 13, 2025

DarthB mentioned this pull request Jun 13, 2025

MOD-9985 Refactoring of C Code for the RSSortingVector Port #6312

Merged

2 tasks

DarthB force-pushed the rp-rssortingvector branch 2 times, most recently from 5d7c5d6 to 1e7e21d Compare June 13, 2025 15:48

DarthB changed the title ~~MOD-9985 - Rust Implementation of RPSortingVector~~ MOD-9985 - Rust Implementation of RSSortingVector Jun 16, 2025

DarthB force-pushed the rp-rssortingvector branch 11 times, most recently from 08f5375 to 5f7722b Compare June 18, 2025 09:33

github-actions bot added the size:XL label Jun 18, 2025

DarthB mentioned this pull request Jun 18, 2025

MOD-9985 Bindings of C types to allow Rust Unit tests for RSSortingVector #6342

Closed

2 tasks

DarthB force-pushed the rp-rssortingvector branch 2 times, most recently from fbfddf3 to 784887d Compare June 25, 2025 14:28

DarthB force-pushed the rp-rssortingvector branch 4 times, most recently from 401002e to 12e1ec8 Compare June 30, 2025 10:43

DarthB added 4 commits July 3, 2025 14:52

forward error msg with transparent

1b205da

new type for string normalization, docs about libnu and icu_casemap i…

c853fde

…ssues

docs in tests

a290315

make normalize string an implementation detail for now

8814717

DarthB force-pushed the rp-rssortingvector branch from 4501bb2 to 8814717 Compare July 3, 2025 12:52

JonasKruckenberg reviewed Jul 3, 2025

View reviewed changes

LukeMathWalker reviewed Jul 4, 2025

View reviewed changes

DarthB added 2 commits July 7, 2025 08:57

address comments

386aec0

enhance tests

545c596

DarthB mentioned this pull request Jul 7, 2025

FFI Layer: Implement RSValueTrait for FFI and implement the C entry point - MOD-9985 #6459

Merged

extract crate for value wrapper containing the trait

a5a6e00

DarthB force-pushed the rp-rssortingvector branch from 8c2a92e to 460a463 Compare July 7, 2025 09:27

DarthB added 2 commits July 8, 2025 08:30

adapt implementation of sorting vector after crate extraction

7b9b119

enhance docs

3f9e3d8

DarthB force-pushed the rp-rssortingvector branch from 460a463 to 3f9e3d8 Compare July 8, 2025 06:46

remove into iterator trait as SortingVector is not consumed

4b1c3a1

raz-mon requested a review from Copilot July 8, 2025 11:03

Copilot AI reviewed Jul 8, 2025

View reviewed changes

address copilot review

8a6c970

raz-mon reviewed Jul 8, 2025

View reviewed changes

DarthB added 2 commits July 9, 2025 10:38

address review comments

9555ae6

add increment/decrement methods to RSValueTrait

ea43277

small fix for CI

e1025d7

DarthB removed the size:XL label Jul 9, 2025

raz-mon approved these changes Jul 10, 2025

View reviewed changes

DarthB added this pull request to the merge queue Jul 10, 2025

Merged via the queue into master with commit 3bb7bee Jul 10, 2025
16 of 17 checks passed

DarthB deleted the rp-rssortingvector branch July 10, 2025 14:47

Conversation

DarthB commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the changes in the pull request

More detailed changes

Referenced PRs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raz-mon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarthB commented Jul 9, 2025

Uh oh!

raz-mon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DarthB commented Jun 13, 2025 •

edited

Loading