Feat/reduce mapping layers #403
Conversation
|
Warning Rate Limit Exceeded@amimart has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 6 minutes and 6 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per repository. WalkthroughThe changes primarily focus on enhancing the functionality of the query engine and refactoring the code for better organization and readability. Key modifications include the introduction of new structs and methods, changes to function parameters and return types, and the removal of unused code. The updates aim to improve the handling of triples and atoms, namespace resolution, and error handling. Changes
Poem
TipsChat with CodeRabbit Bot (
|
size-limit report 📦
|
Codecov Report
@@ Coverage Diff @@
## main #403 +/- ##
==========================================
- Coverage 97.76% 96.93% -0.83%
==========================================
Files 42 48 +6
Lines 10243 9858 -385
==========================================
- Hits 10014 9556 -458
- Misses 229 302 +73
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
amimart
force-pushed
the
feat/reduce-mapping-layers
branch
from
fa085ff
to
6ebbf83
Compare
|
@ccamel I've open to PR to review to take benefits from your eventual remarks and those from @coderabbitai while I'm adding some tests 😎 |
amimart
force-pushed
the
feat/reduce-mapping-layers
branch
2 times, most recently
from
bdc737a
to
cc94b6d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- Cargo.lock
- contracts/okp4-cognitarium/Cargo.toml
Files selected for processing (9)
- contracts/okp4-cognitarium/src/contract.rs (6 hunks)
- contracts/okp4-cognitarium/src/querier/engine.rs (10 hunks)
- contracts/okp4-cognitarium/src/querier/mapper.rs (1 hunks)
- contracts/okp4-cognitarium/src/querier/mod.rs (1 hunks)
- contracts/okp4-cognitarium/src/querier/plan_builder.rs (5 hunks)
- contracts/okp4-cognitarium/src/rdf/atom.rs (3 hunks)
- contracts/okp4-cognitarium/src/rdf/mapper.rs (2 hunks)
- contracts/okp4-cognitarium/src/state/namespaces.rs (3 hunks)
- contracts/okp4-cognitarium/src/storer/engine.rs (5 hunks)
Files skipped from review due to trivial changes (3)
- contracts/okp4-cognitarium/src/querier/mod.rs
- contracts/okp4-cognitarium/src/rdf/atom.rs
- contracts/okp4-cognitarium/src/rdf/mapper.rs
Additional comments: 33
contracts/okp4-cognitarium/src/querier/mapper.rs (1)
- 1-84: The changes in this hunk are mostly about adding a new parameter
prefixesto the functionsnode_as_subject,node_as_predicate,node_as_object,literal_as_object, andiri_as_node. This parameter is used to resolve namespaces and handle prefixed IRIs. The functioniri_as_nodenow returns aStdResult<state::Node>instead ofstate::Node, which allows for better error handling. The changes seem to be consistent and logical. However, ensure that all calls to these functions throughout the codebase have been updated to match the new signatures.contracts/okp4-cognitarium/src/contract.rs (7)
99-115: The
delete_allmethod is used here to delete all triples. Ensure that this is the intended behavior and that it won't accidentally delete necessary data. Also, make sure that thedelete_allmethod handles errors properly and doesn't fail silently if it encounters an issue.140-147: The
querymodule has been modified. Ensure that all changes are compatible with the rest of the codebase and that all necessary updates have been made.173-176: The
selectfunction now uses themap_select_solutionsfunction. Ensure that this function is correctly implemented and that it handles errors properly.222-231: The
describefunction now uses theresolve_atomsmethod. Ensure that this method is correctly implemented and that it handles errors properly.276-285: The
constructfunction now uses theresolve_atomsmethod. Ensure that this method is correctly implemented and that it handles errors properly.321-322: The
as_select_variablesfunction is used here. Ensure that this function is correctly implemented and that it handles errors properly.341-370: The
map_select_solutionsfunction is used here. Ensure that this function is correctly implemented and that it handles errors properly.contracts/okp4-cognitarium/src/storer/engine.rs (5)
4-9: The import of
blake3::Hashhas been removed. Ensure that it is not used elsewhere in the code.52-63: The
triple_sizefunction now returns aStdResult<usize>instead ofusize. This change is likely due to the fact that the function now performs operations that can fail and return an error. This is a good practice as it allows for better error handling.108-111: The
delete_allfunction now takes a slice ofTriplestructs instead ofrdf::Atom. This change is likely due to the fact that the function now operates directly onTriplestructs instead ofrdf::Atom. This is a good practice as it simplifies the function's implementation and makes it more efficient.115-149: The
delete_triplefunction has been modified to use thetriples().may_loadandtriples().replacefunctions instead oftriples().remove. This change is likely due to the fact that the function now needs to check if the triple exists before deleting it and replace it withNoneif it does. This is a good practice as it prevents the function from attempting to delete non-existing triples and allows for better error handling.241-276: The
triple_size,subject_size,node_size, andobject_sizefunctions now take a mutable reference toself. This change is likely due to the fact that these functions now modify theStoreEnginestruct's state. This is a good practice as it allows these functions to modify the struct's state and ensures that the struct's state is always up-to-date.contracts/okp4-cognitarium/src/state/namespaces.rs (3)
194-205: The
resolve_from_keymethod is a good addition to theNamespaceBatchServicestruct. It provides a way to resolve aNamespacefrom its internal key, which can be useful in various scenarios. The implementation looks correct and it uses theresolve_from_keymethod from theNamespaceResolverstruct, which is the correct way to do it.238-261: The
free_refmethod has been updated to take akeyparameter instead of astorageparameter. This is a good change as it simplifies the method signature and makes it more intuitive to use. The implementation of the method has also been updated to decrement the reference count of the namespace and update thens_count_difffield accordingly. This is a logical change and it seems to be implemented correctly. However, it's important to ensure that thefree_refmethod is called correctly throughout the codebase with the new signature.286-288: The
allocatemethod is a private method used to allocate a newNamespacewith a given value. The implementation looks correct and it properly updates thens_key_incandns_count_difffields. However, it's important to note that theallocatemethod does not check if aNamespacewith the same value already exists. If this is a possibility, it might be a good idea to add a check for this to avoid creating duplicateNamespaceinstances.fn allocate(&mut self, value: String) -> Namespace { if self.ns_resolver.by_val.contains_key(&value) { panic!("Namespace with the same value already exists"); } let ns = Namespace { value, key: self.ns_key_inc, counter: 0u128, }; self.ns_key_inc += 1; self.ns_count_diff += 1; self.ns_resolver.insert(ns).borrow().clone() }contracts/okp4-cognitarium/src/querier/engine.rs (11)
1-24: The
QueryEngineandSelectResultsstructs are defined. TheQueryEnginestruct has astoragefield, and theSelectResultsstruct hasheadandsolutionsfields. TheQueryEnginestruct has anewmethod that takes astorageargument and returns a newQueryEngineinstance.47-55: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [34-52]
The
selectmethod of theQueryEnginestruct is defined. It takes aQueryPlanand a vector ofSelectItemas arguments, and returns aSelectResultsinstance. Theheadfield of theSelectResultsinstance is a vector of the keys of thebindingsmap, and thesolutionsfield is aSolutionsIteratorinstance.
- 372-418: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [375-439]
The
SolutionsIteratorstruct and its methods are defined. TheSolutionsIteratorstruct hasiterandbindingsfields. Thenewmethod takes aResolvedVariablesIteratorand aBTreeMapof bindings as arguments, and returns a newSolutionsIteratorinstance. Theresolve_triplesandresolve_atomsmethods take several arguments and return a vector ofTripleandAtominstances, respectively.
441-535: The
ResolvedTripleIteratorstruct and its methods are defined. TheResolvedTripleIteratorstruct hasiter,templates, andbufferfields. Thetry_newmethod takes several arguments and returns a newResolvedTripleIteratorinstance. Thenextmethod returns the nextTripleinstance from the buffer.502-631: The
TripleTemplatestruct and its methods are defined. TheTripleTemplatestruct hassubject,predicate, andobjectfields. Thetry_newmethod takes several arguments and returns a newTripleTemplateinstance. Theresolvemethod takes aBTreeMapof variables as an argument and returns aTripleinstance.633-856: The
ResolvedAtomIteratorstruct and its methods are defined. TheResolvedAtomIteratorstruct hasns_resolver,storage,iter,templates, andbufferfields. Thetry_newmethod takes several arguments and returns a newResolvedAtomIteratorinstance. Thenextmethod returns the nextAtominstance from the buffer.986-1082: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [858-1079]
The test module is defined. It contains several test cases for the
selectmethod of theQueryEnginestruct.
- 926-937: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [918-934]
This test case expects an error because the selected variable is not found in the query.
- 946-966: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [935-963]
This test case expects a successful result with a single variable in the result set.
- 986-1082: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [964-1066]
This test case expects a successful result with multiple variables in the result set.
- 1068-1079: The test cases are executed and the results are asserted.
contracts/okp4-cognitarium/src/querier/plan_builder.rs (6)
1-11: Imports have been updated to reflect the changes in the codebase. Ensure that these changes do not break any functionality.
1-11: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [12-80]
The
PlanBuilderstruct and its methods have been updated. The changes seem to be mostly refactoring and reorganizing the code. Ensure that these changes do not break any functionality.
115-138: The
build_subject_patternandbuild_predicate_patternmethods have been updated to use the newnode_as_subjectandnode_as_predicatefunctions. Ensure that these changes do not break any functionality.142-159: The
build_object_patternmethod has been updated to use the newnode_as_objectandliteral_as_objectfunctions. Ensure that these changes do not break any functionality.180-188: The test imports have been updated to reflect the changes in the codebase. Ensure that these changes do not break any functionality.
264-335: The
build_named_nodetest has been commented out. If this test is no longer relevant due to the changes in the codebase, consider removing it entirely. If it is still relevant, consider updating it to reflect the changes in the codebase.337-End:
Thebuild_triple_patternandbuild_plantests have been updated to reflect the changes in the codebase. Ensure that these changes do not break any functionality.
amimart
force-pushed
the
feat/reduce-mapping-layers
branch
from
cc94b6d
to
f08f7d2
Compare
Closes #404
Proposed solution
To avoid unnecessary mapping layers, it is mandatory that the
QueryEnginedoesn't return already mapped data, its design is thus altered to provide a solutions iterator by exposing the resolved variables from the state directly.As a common usage of the
QueryEngine::selectin the processing ofDeleteData,DescribeandConstructmessages is to recompose triples or atoms from a set of patterns applied on resolved variables, I propose to introduce new iterator implementations (i.e.ResolvedTripleIterator&ResolvedAtomIterator) which based on a set of patterns and a solutions iterator offer those recomposition.The
StoreEnginehas been in consequence updated to manage triple deletion directly fromstate::Triple, this way there is no mapping layer between the select query and the actual deletion.Bug fixes
StoreEnginenow doesn't decrease counters (i.e. triple count & total byte size) when deleting non existing triples.DeleteData,DescribeandConstructwhen a resolved triple pattern leads to an impossible triple construction (e.g. a variable from the object term representing a simple literal used as a predicate) it doesn't fail anymore, the solution is ignored.Summary by CodeRabbit
Refactor:
executeandquerymodules incontract.rsfor better code readability and efficiency.QueryEnginestruct and related functions inengine.rs.PlanBuilderstruct and its methods inplan_builder.rsfor better code structure.TriplePatternstruct fromatom.rsto clean up the codebase.New Features:
mappermodule inmod.rsfor better code organization.resolve_from_keymethod innamespaces.rsto resolve aNamespacefrom its internal key.Bug Fixes:
contract.rsfor correct program execution.mapper.rsto handle prefixed IRIs and resolve namespaces.Chores:
StoreEnginestruct inengine.rsto use more appropriate data types and methods.