[MOD-10235] Polish varint implementation by LukeMathWalker · Pull Request #6346 · RediSearch/RediSearch

LukeMathWalker · 2025-06-18T13:00:56Z

Describe the changes in the pull request

This PR refactors the Rust varint implementation. There are no functional changes.
In particular:

We use a trait-based design, with macros to avoid code duplication.
We remove the direct dependency on FieldMask, and therefore on the ffi crate, by providing an encoding/decoding implementation for both u64 and u128. We can thus stop mirroring in the varint test suite the platform-specific knobs that are used, on the C side, to pick the correct size of FieldMask.
More extensive documentation all around.
More test cases, including errors and property-based tests. We now have 100% test coverage for varint encoding/decoding.
Rename encode_decode to varint, since we want to have small self-contained crates to minimize compile times. The existing varint crate is renamed to varint_ffi.

Benchmarks

Benchmarks have been refactored to measure performance based on the expected encoded size, rather than grouping everything together.
You can compare performance before/after the refactoring by executing benches for the first commit on this branch and then re-running them for the latest commit on this branch. No significant changes were measured on my local machine.

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

src/redisearch_rs/encode_decode/src/varint/mod.rs

src/redisearch_rs/encode_decode/src/varint/vector_writer.rs

src/redisearch_rs/encode_decode/tests/varint.rs

chesedo

Just small nit picky stuff

The base branch was changed.

codecov · 2025-06-24T16:37:08Z

Codecov Report

Attention: Patch coverage is 92.30769% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.76%. Comparing base (428d202) to head (b8ed78e).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
...earch_rs/c_entrypoint/varint_ffi/src/field_mask.rs	0.00%	2 Missing ⚠️
...redisearch_rs/c_entrypoint/varint_ffi/src/value.rs	0.00%	2 Missing ⚠️
src/redisearch_rs/varint/src/vector_writer.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6346      +/-   ##
==========================================
+ Coverage   88.72%   88.76%   +0.04%     
==========================================
  Files         251      251              
  Lines       41729    41606     -123     
  Branches     3748     3625     -123     
==========================================
- Hits        37022    36930      -92     
+ Misses       4658     4632      -26     
+ Partials       49       44       -5

Flag	Coverage Δ
flow	`81.34% <ø> (-0.15%)`	⬇️
unit	`46.66% <92.30%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

lerman25

Great, few questions

src/redisearch_rs/varint/tests/varint.rs

lerman25 · 2025-06-25T08:52:21Z

src/redisearch_rs/varint_bencher/benches/core_operations.rs


 fn benchmark_core_operations(c: &mut Criterion) {
-    let bencher = VarintBencher::new("Varint".to_owned(), Duration::from_secs(10));
+    let bencher = VarintBencher::new(Duration::from_secs(5));


Is the change from 10 to 5 intentional?

Yes, since the bench cases are now smaller we shouldn't need 10 seconds to achieve statistical significance.
I've just triggered a micro-benchmark run in CI to double-check that's the case in the CI environment too.

I can confirm that the change doesn't cause issues, given the logs in https://github.com/RediSearch/RediSearch/actions/runs/15872068421/job/44750799229?pr=6346

This reverts commit a08d223.

* Revert "[MOD-10235] Polish varint implementation (#6346)" This reverts commit a08d223. * change runner * Revert "[MOD-9403] Remove stale varint target (#6372)" This reverts commit f3aefd2. * Revert "MOD-9403: Replace varint.* with the Rust implementation (#6308)" This reverts commit 428d202.

LukeMathWalker changed the base branch from master to drop-c-varint June 18, 2025 13:01

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

LukeMathWalker force-pushed the clean-up-varint branch from 85066e6 to 8259983 Compare June 18, 2025 13:05

LukeMathWalker requested a review from chesedo June 18, 2025 13:07