[MOD-9999] Migrate RSAggregateResult to Rust#6255
Conversation
| pub num_children: c_int, | ||
|
|
||
| /// The capacity of the records array. Has no use for extensions | ||
| pub children_cap: c_int, |
There was a problem hiding this comment.
Do we need to keep it as a c_int? Considering what we are tracking, I would assume that a u16 would do just fine?
There was a problem hiding this comment.
I would say yes for the time being. We need the C output to be int (to match the old declaration) since it can be 16 or 32 bits depending on the compiling machine. Using i16 / u16 could therefore change the alignment
There was a problem hiding this comment.
I know that it would be a change—my argument here is that it should not be an int in C either. I'm also OK with doing the change in another PR if we want to keep this a pure refactoring.
There was a problem hiding this comment.
I see. Let me make another PR to change the C to always be a u16. Would like to keep this one focused on the refactor
chesedo
force-pushed
the
migrate_rs_numeric_record_to_rust
branch
from
7a51007 to
4ed89e4
Compare
chesedo
force-pushed
the
migrate_rs_aggregate_result_to_rust
branch
from
13009f3 to
dffd860
Compare
chesedo
force-pushed
the
migrate_rs_numeric_record_to_rust
branch
2 times, most recently
from
ccbe1e1 to
c6ececc
Compare
chesedo
force-pushed
the
migrate_rs_aggregate_result_to_rust
branch
from
3a698a9 to
ceb3eef
Compare
This is needed to move `RSIndexResult` to Rust which will make it easier to port the inverted index to Rust.
This just makes the C compiler happy when "reading" values of the vector.
This will makes the usages of `RSAggregateResult` more correct inside the Rust code.
The latest code has a fix we need for the Rust code to compile. But it is not released yet so just using it directly for now.
chesedo
force-pushed
the
migrate_rs_aggregate_result_to_rust
branch
from
d97568e to
6e6a1a4
Compare
| * ```ignore | ||
| * struct BitFlags<T, N = <T as BitFlag>::Numeric> { | ||
| * value: N, | ||
| * marker: PhantomData<T>, | ||
| * } |
There was a problem hiding this comment.
This practice is not needed for us in RSResultTypeMask?
There was a problem hiding this comment.
Yeah, we probably won't use any const functions. Note that this documentation comes from the enumflags2 crate via its BitFlags type.
It's coming from this line
RediSearch/src/redisearch_rs/inverted_index/src/lib.rs
Lines 74 to 76 in e04cf6b
There was a problem hiding this comment.
Turning it off is also a no go. cbindgen only allows turning all documentation off and not the documentation of a single type.
chesedo
force-pushed
the
migrate_rs_aggregate_result_to_rust
branch
from
e04cf6b to
0dadce0
Compare
This is needed to make the doc tests pass since the FFI is going to pull in a comment which shows how to use `BitFlags`.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6255 +/- ##
==========================================
- Coverage 88.89% 88.82% -0.07%
==========================================
Files 240 240
Lines 40524 40524
Branches 3165 3165
==========================================
- Hits 36023 35995 -28
- Misses 4466 4494 +28
Partials 35 35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
Describe the changes in the pull request
This moves the
RSAggregateResultdefinition to Rust in preparation of moving theRSIndexResultto Rust too. This will make it easier to port the inverted index to Rust.The Rust definition is exactly the same as the old C definition, so nothing in the C code needs to be updated.
Mark if applicable