MOD-10187 Refactor Refactor RLookup_GetKey* C interface for porting#6345
Merged
JonasKruckenberg merged 5 commits intomasterfrom Jun 23, 2025
Merged
MOD-10187 Refactor Refactor RLookup_GetKey* C interface for porting#6345JonasKruckenberg merged 5 commits intomasterfrom
JonasKruckenberg merged 5 commits intomasterfrom
Conversation
JonasKruckenberg
force-pushed
the
push-tvzmkrozsotw
branch
from
0683a02 to
9948a14
Compare
JonasKruckenberg
force-pushed
the
push-tvzmkrozsotw
branch
from
ffced68 to
469548b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6345 +/- ##
==========================================
+ Coverage 88.92% 88.93% +0.01%
==========================================
Files 247 250 +3
Lines 41291 41366 +75
Branches 3483 3556 +73
==========================================
+ Hits 36718 36789 +71
+ Misses 4530 4528 -2
- Partials 43 49 +6
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:
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the RLookup C interface to simplify porting to Rust by replacing the overloaded RLookup_GetKey functions with mode‐specific variants.
- Replaces RLookup_GetKey/RLookup_GetKeyEx with RLookup_GetKey_Read/Write (and their Ex variants) across the codebase
- Updates documentation and implementation in header and source files to reflect the new API design
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cpptests/test_cpp_rlookup.cpp | Updated test cases to use the new read/write aliases |
| tests/cpptests/test_cpp_resultprocessor.cpp | Replaced function calls with the mode-specific write alias |
| tests/cpptests/test_cpp_expr.cpp | Adjusted key retrieval functions to use the new API variant |
| tests/cpptests/test_cpp_agg.cpp | Modified test functions to use new API variants in aggregation tests |
| src/spec.c | Replaced RLookup_GetKeyEx with its read-specific variant |
| src/rlookup.h | Updated function declarations and documentation for the new API |
| src/rlookup.c | Modified function implementations to call the common helper with proper mode flags |
| src/coord/dist_plan.cpp | Changed key retrieval calls to use the write alias |
| src/aggregate/reducer.c | Replaced read mode API call with the new read-specific function |
| src/aggregate/expr/expression.c | Updated property lookup to use the read alias |
| src/aggregate/aggregate_request.c | Adjusted both source and destination key lookups using new API |
| src/aggregate/aggregate_exec.c | Updated key calls in serialization to use the read-specific variant |
Comments suppressed due to low confidence (1)
src/rlookup.h:209
- [nitpick] The documentation for the new mode-specific functions (RLookup_GetKey_Read, RLookup_GetKey_Write, etc.) is very similar. Consider adding clarifying details that explicitly describe the behavioral differences between these functions to improve readability and maintainability.
* Get a RLookup key for a given name.
raz-mon
reviewed
Jun 23, 2025
Collaborator
raz-mon
left a comment
raz-mon
left a comment
There was a problem hiding this comment.
My main question: Why is this better than binding the enum and using it in the Rust code?
Also, one typo that existed before..
Co-authored-by: Raz Monsonego <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the changes in the pull request
This refactor this RLookup C interface to make porting to Rust easier. In particular it
replaces the
RLookup_GetKey/RLookup_GetKeyExfunctions which have their behavior controlled through themodeargument with mode-specific aliases:RLookup_GetKey_Read/RLookup_GetKey_ReadExandRLookup_GetKey_Write/RLookup_GetKey_WritEx.Mark if applicable