Skip to content

Swap the RSSortingVector Implementation - MOD-10714 | MOD-9985#6397

Closed
DarthB wants to merge 9 commits intomasterfrom
rp-rssortingvector-swap
Closed

Swap the RSSortingVector Implementation - MOD-10714 | MOD-9985#6397
DarthB wants to merge 9 commits intomasterfrom
rp-rssortingvector-swap

Conversation

@DarthB
Copy link
Copy Markdown
Contributor

@DarthB DarthB commented Jun 30, 2025 •
edited
Loading

Describe the changes in the pull request

THIS SWAPS THE C-Implementation with the Rust Implementation of RSSortingVector

Removing the old C-Code of RSSortingVector, and adding refactoring parts of the C code (renaming, direct field access)

  • Rename functions
  • Add direct field access
  • Add todos for open work (SortingVector_RdbLoad, normalizeStr)
  • Add missing defines
  • Adds Python Tests that check for bugs that occurred in the benchmarks and are related to handling the string type of RSValue in Rust.

Based on: #6311

  • This PR introduces API changes
  • This PR introduces serialization changes

@DarthB DarthB mentioned this pull request Jun 30, 2025
Merged
2 tasks
@DarthB DarthB force-pushed the rp-rssortingvector-swap branch from 7606fa3 to cc24b20 Compare June 30, 2025 11:22
@DarthB DarthB force-pushed the rp-rssortingvector-swap branch 2 times, most recently from ed9b5e7 to 8549484 Compare July 1, 2025 14:37
@fcostaoliveira
Copy link
Copy Markdown
Contributor

fcostaoliveira commented Jul 1, 2025 •
edited
Loading

Automated performance analysis summary

This comment was automatically generated given there is performance data available.

In summary:

  • Detected a total of 31 stable tests between versions.
  • Detected a total of 6 highly unstable benchmarks (6 baseline).
  • Latency analysis confirmed regressions in 1 of the unstable tests:
  • Detected a total of 3 regressions bellow the regression water line 8.0.

You can check a comparison in detail via the grafana link

Performance Regressions and Issues - Comparison between master and rp-rssortingvector-swap.

Time Period from 30 days ago. (environment used: oss-standalone)

Test Case Baseline master (median obs. +- std.dev) Comparison rp-rssortingvector-swap (median obs. +- std.dev) % change (higher-better) Note
search-numeric-sortby-desc 5276 +- 23.4% UNSTABLE (7 datapoints) 3580 -32.1% UNSTABLE (baseline high variance); server: FT.SEARCH p50 increased 50.0% (baseline CV=30.1%); client: Latency increased 47.4% (baseline CV=29.2%)
vecsim-arxiv-titles-384-angular-filters-m16-ef-128-numeric-filter 175 +- 6.2% (7 datapoints) 124 -29.2% REGRESSION
search-filtering-tag-numeric 344 +- 15.4% UNSTABLE (7 datapoints) 288 -16.1% UNSTABLE (baseline high variance); client: Latency increased 19.2% (baseline CV=13.7%); only client side confirms regression (server side stable) - insufficient evidence
search-ftsb-10K-enwiki_abstract-hashes-term-withsuffix-trie 52101 +- 3.3% (7 datapoints) 47304 -9.2% REGRESSION
search-numeric-sortby 3335 +- 37.2% UNSTABLE (7 datapoints) 3058 -8.3% UNSTABLE (baseline high variance); client: Latency increased 9.1% (baseline CV=25.3%); neither server nor client side confirms regression
search-filtering-tag-numeric-filter-pipeline 26037 +- 2.1% (7 datapoints) 23913 -8.2% REGRESSION
ftsb-1M-enwiki_abstract-hashes-fulltext-2word-intersection-query-non-sortable 28 +- 42.5% UNSTABLE (7 datapoints) 26 -6.8% UNSTABLE (baseline high variance); client: OverallQuantiles.allCommands.q50 decreased 15.8% (baseline CV=8.4%); neither server nor client side confirms regression
ftsb-1M-enwiki_abstract-hashes-fulltext-2word-union-query-non-sortable 1127 +- 13.9% UNSTABLE (7 datapoints) 1091 -3.2% UNSTABLE (baseline high variance); client: client latency stable; neither server nor client side confirms regression
search-numeric 3252 +- 38.6% UNSTABLE (7 datapoints) 3229 -0.7% UNSTABLE (baseline high variance); client: client latency stable; neither server nor client side confirms regression
Tests with No Significant Changes (31 tests)

Tests with No Significant Changes

Test Case Baseline master (median obs. +- std.dev) Comparison rp-rssortingvector-swap (median obs. +- std.dev) % change (higher-better) Note
ftsb-10K-enwiki_abstract-hashes-fulltext-sortby N/A N/A 0.0%
ftsb-10K-enwiki_abstract-hashes-term-prefix N/A N/A 0.0%
ftsb-10K-enwiki_abstract-hashes-term-suffix 2744 +- 0.8% (7 datapoints) 2692 -1.9% No Change
ftsb-10K-enwiki_abstract-hashes-term-suffix-withsuffixtrie N/A N/A 0.0%
ftsb-10K-enwiki_abstract-hashes-term-wildcard N/A N/A 0.0%
ftsb-10K-enwiki_pages-hashes-fulltext-mixed_simple-1word-query_write_1_to_read_20.yml N/A N/A 0.0%
ftsb-10K-enwiki_pages-hashes-load 66115 +- 4.3% (7 datapoints) 61544 -6.9% potential REGRESSION
ftsb-10K-multivalue-numeric-json N/A N/A 0.0%
ftsb-10K-singlevalue-numeric-json N/A N/A 0.0%
ftsb-1K-enwiki_abstract-hashes-term-contains 2315 +- 1.7% (7 datapoints) 2219 -4.1% potential REGRESSION
ftsb-1M-enwiki_abstract-hashes-fulltext-2word-intersection-query N/A N/A 0.0% waterline=16.9%.
ftsb-1M-enwiki_abstract-hashes-fulltext-2word-union-query N/A N/A 0.0% waterline=8.8%.
ftsb-1M-enwiki_abstract-hashes-fulltext-simple-1word-query N/A N/A 0.0% waterline=15.5%.
ftsb-1M-enwiki_abstract-hashes-load N/A N/A 0.0%
ftsb-1M-nyc_taxis-ftadd-load 30429 +- 3.7% (7 datapoints) 28636 -5.9% potential REGRESSION
ftsb-1M-nyc_taxis-hashes-load N/A N/A 0.0%
search-aggregate-post-filter-simple.yml 132714 +- 4.6% (7 datapoints) 128826 -2.9% No Change
search-ftsb-10K-enwiki_abstract-hashes-term-withoutsuffix-trie N/A N/A 0.0%
search-ftsb-1700K-docs-union-iterators-q3 9.1 +- 1.7% (7 datapoints) 9.3 2.8% No Change
search-ftsb-1M-enwiki_abstract-hashes-fulltext-simple-1word-query-non-sortable N/A N/A 0.0% waterline=8.6%.
search-ftsb-370K-docs-union-iterators-q4 N/A N/A 0.0%
search-ftsb-5200K-docs-union-iterators-q1 N/A N/A 0.0%
search-ftsb-5500K-docs-union-iterators-q2 1.3 +- 1.7% (7 datapoints) 1.3 0.0%
search-geo 194 +- 6.0% (7 datapoints) 203 4.9% potential IMPROVEMENT
search-high-cardinality-negation-term-baseline 30 +- 6.4% (7 datapoints) 29 -2.2% No Change
search-high-cardinality-negation-term-comparison_union_all_other_terms 9.5 +- 4.2% (7 datapoints) 10 6.3% potential IMPROVEMENT
search-numeric-optimize 14578 +- 1.5% (7 datapoints) 14504 -0.5% No Change
search-numeric-sortby-desc-optimize N/A N/A 0.0%
search-numeric-sortby-optimize 29 +- 8.2% (7 datapoints) 31 6.8% waterline=8.2%. potential IMPROVEMENT
vecsim-arxiv-titles-384-angular-filters-m16-ef-128-fulltext-filter 638 +- 4.1% (7 datapoints) 648 1.6% No Change
vecsim-arxiv-titles-384-angular-filters-m16-ef-128-tag-filter 79027 +- 4.1% (7 datapoints) 82926 4.9% potential IMPROVEMENT

@DarthB DarthB force-pushed the rp-rssortingvector-swap branch from f58e355 to 35c8b70 Compare July 3, 2025 10:22
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jul 3, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@DarthB DarthB force-pushed the rp-rssortingvector-swap branch from 35c8b70 to a7430d8 Compare July 3, 2025 13:08
@DarthB DarthB changed the title MOD-9985 swap the RSSortingVector Implementation Swap the RSSortingVector Implementation - MOD-9985 Jul 8, 2025
@DarthB DarthB mentioned this pull request Jul 8, 2025
Merged
@DarthB DarthB force-pushed the rp-rssortingvector-swap branch 3 times, most recently from 766bf9e to 9cd12b3 Compare July 8, 2025 08:29
@DarthB DarthB added enforce:sanitize Run sanitizer flow even on draft pull request action:run-benchmark labels Jul 8, 2025
@DarthB DarthB changed the base branch from master to rp-rsvalue-crate July 9, 2025 10:07
@DarthB DarthB force-pushed the rp-rsvalue-crate branch from 3c2ed44 to ac79dd0 Compare July 9, 2025 10:09
@DarthB DarthB force-pushed the rp-rssortingvector-swap branch from 967a606 to 2a69e42 Compare July 9, 2025 10:13
@github-actions github-actions bot added the size:M label Jul 9, 2025
@DarthB DarthB force-pushed the rp-rssortingvector-swap branch from 2a69e42 to 64b6c71 Compare July 9, 2025 11:17
@DarthB DarthB removed the size:XL label Jul 10, 2025
@DarthB DarthB mentioned this pull request Jul 10, 2025
Merged
@DarthB DarthB force-pushed the rp-rsvalue-crate branch from ac79dd0 to 96156ef Compare July 14, 2025 13:43
@DarthB DarthB changed the base branch from rp-rsvalue-crate to master July 14, 2025 13:47
@DarthB DarthB changed the base branch from master to rp-rsvalue-crate July 14, 2025 13:48
@DarthB DarthB force-pushed the rp-rssortingvector-swap branch from 331313f to 60611fb Compare July 23, 2025 10:39
@DarthB DarthB requested a review from Copilot July 23, 2025 10:46
Copilot AI reviewed Jul 23, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces the C implementation of RSSortingVector with a Rust implementation, swapping out the existing C-based sorting vector functionality for improved performance and memory safety. The change involves replacing header includes, updating function calls, and providing C FFI bindings for the new Rust implementation.

  • Removes the old C-based RSSortingVector implementation and replaces it with Rust FFI bindings
  • Updates all C code to use the new Rust-backed sorting vector functions (RSSortingVector_New, RSSortingVector_Free, etc.)
  • Adds necessary constants and forward declarations to support the C-Rust interop

Reviewed Changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/spec.h Updates include from sortable.h to sorting_vector_rs.h
src/sortable.h Removes C struct definition and function declarations, keeps legacy RDB load function
src/sortable.c Removes C implementation functions, keeps normalizeStr and SortingVector_RdbLoad
src/redisearch_rs/headers/sorting_vector_rs.h Adds forward declarations and cleans up function signatures
src/redisearch_rs/c_entrypoint/sorting_vector_ffi/src/lib.rs Implements Rust FFI layer with constants and type definitions
src/document.c Updates function calls from C versions to Rust FFI versions
src/debug_commands.c Replaces direct field access with function calls
Comments suppressed due to low confidence (1)

src/redisearch_rs/c_entrypoint/sorting_vector_ffi/src/lib.rs:33

  • [nitpick] The struct name 'RSSortingVector2' is not descriptive and appears to be a workaround name. Consider using a more meaningful name like 'RSSortingVectorImpl' or 'RSSortingVectorWrapper' to better indicate its purpose as an implementation wrapper.
pub struct RSSortingVector2 {

@DarthB DarthB force-pushed the rp-rssortingvector-swap branch 2 times, most recently from 7dc429a to 2483223 Compare July 23, 2025 11:09
@DarthB DarthB removed the size:XS label Jul 23, 2025
@DarthB DarthB changed the title Swap the RSSortingVector Implementation - MOD-9985 Swap the RSSortingVector Implementation - MOD-10714 | MOD-9985 Aug 4, 2025
@DarthB DarthB force-pushed the rp-rssortingvector-swap branch 2 times, most recently from b900af3 to c41cdfd Compare August 7, 2025 09:15
@DarthB DarthB force-pushed the rp-rssortingvector-swap branch from c41cdfd to 2c110d9 Compare August 25, 2025 15:20
@DarthB DarthB force-pushed the rp-rssortingvector-swap branch from 2c110d9 to 899fb8d Compare September 4, 2025 10:26
@DarthB DarthB force-pushed the rp-rssortingvector-swap branch from 899fb8d to a8e28f7 Compare September 4, 2025 10:47
@DarthB DarthB force-pushed the rp-rssortingvector-swap branch from a378ab2 to a165736 Compare September 9, 2025 12:52
@DarthB DarthB force-pushed the rp-rssortingvector-swap branch from a165736 to 116fbff Compare September 10, 2025 09:47
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 11, 2025

This pull request is stale because it has been open for 60 days with no activity.

@github-actions github-actions bot added the stale label Nov 11, 2025
@DarthB DarthB closed this Nov 21, 2025
@DarthB
Copy link
Copy Markdown
Contributor Author

DarthB commented Nov 21, 2025

#6887

@JonasKruckenberg JonasKruckenberg mentioned this pull request Jan 21, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

action:run-benchmark action:run-micro-benchmark enforce:sanitize Run sanitizer flow even on draft pull request size:M stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@DarthB @fcostaoliveira @CLAassistant