Skip to content

Varint oxidization#5996

Closed
zeenix wants to merge 3 commits intomasterfrom
varint-oxidization
Closed

Varint oxidization#5996
zeenix wants to merge 3 commits intomasterfrom
varint-oxidization

Conversation

@zeenix
Copy link
Contributor

@zeenix zeenix commented Apr 23, 2025

  1. Current: The varint module is currently in C.
  2. Change: This change ports all the implementation of varint module to Rust, along with adding a more idiomatic Rust API.
  3. Outcome: This PR is the first step towards the complete port of the Inverted Index to Rust.

@zeenix zeenix requested a review from LukeMathWalker April 23, 2025 11:10
@CLAassistant
Copy link

CLAassistant commented Apr 23, 2025

CLA assistant check
All committers have signed the CLA.

@zeenix zeenix requested a review from DarthB April 23, 2025 11:10
@zeenix zeenix force-pushed the varint-oxidization branch from 5f3cd71 to 830d3a5 Compare April 23, 2025 11:14
@zeenix zeenix force-pushed the varint-oxidization branch from 39ada36 to d85db90 Compare April 24, 2025 08:24
@DarthB
Copy link
Contributor

DarthB commented Apr 28, 2025

We should rebase on master again - the changes in a0fb945 change the structure for our ffi modules (all are moved to the redissearch_rs crate)

@zeenix zeenix force-pushed the varint-oxidization branch 2 times, most recently from 6fb43c2 to 6c7b96e Compare April 28, 2025 15:55
@zeenix zeenix force-pushed the varint-oxidization branch 2 times, most recently from ef433f5 to 63b7535 Compare May 6, 2025 15:39
@zeenix zeenix mentioned this pull request May 8, 2025
2 tasks
@zeenix zeenix force-pushed the varint-oxidization branch from fcc00a1 to baafa85 Compare May 12, 2025 11:53
@codecov
Copy link

codecov bot commented May 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.17%. Comparing base (3127c69) to head (c6210b5).
Report is 104 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5996      +/-   ##
==========================================
- Coverage   87.23%   87.17%   -0.07%     
==========================================
  Files         198      196       -2     
  Lines       36052    35968      -84     
==========================================
- Hits        31449    31354      -95     
- Misses       4603     4614      +11     
Flag Coverage Δ
flow 82.60% <84.61%> (-0.17%) ⬇️
unit 40.34% <100.00%> (-0.13%) ⬇️

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.

zeenix added 3 commits May 12, 2025 17:02
C23 is now the default in gcc (version 15) and is being shipped by
default in some Linux distributions (e.g Fedora 42).
@zeenix zeenix force-pushed the varint-oxidization branch from 369fa76 to c6210b5 Compare May 12, 2025 15:50
@zeenix zeenix requested a review from LukeMathWalker May 12, 2025 16:03
@zeenix
Copy link
Contributor Author

zeenix commented May 12, 2025

I think this PR is ready for a review. I split some of the changes to separate PRs. However, this PR is still based on #6122 and #6102 so those should go first. They're both trivial anyway.

@LukeMathWalker
Copy link
Collaborator

Let's split this PR in 3 smaller ones:

  1. varintand VectorWriter pure Rust implementations, with docs, tests and microbenchmarks
  2. Buffer. Let's add tests and document our safety invariants, following the same style we use in trie_rs/low_memory_thin_vec (basically this style).
  3. The C integration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants