Replace builtin map by ordered map for deterministic iteration #2
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce several upgrades and enhancements, including updating the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
bdeneux
force-pushed
the
feat/sorted-map-procedures
branch
from
c198adc
to
b4c92d8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (7)
- engine/builtin.go (7 hunks)
- engine/builtin_test.go (31 hunks)
- engine/dcg_test.go (1 hunks)
- engine/text.go (2 hunks)
- engine/text_test.go (3 hunks)
- engine/vm.go (5 hunks)
- engine/vm_test.go (10 hunks)
Additional comments not posted (81)
engine/dcg_test.go (1)
14-22: LGTM!The initialization of the
VMstruct with an ordered map usingbuildOrderedMapensures consistent procedure order. The changes are appropriate.engine/text.go (4)
6-6: LGTM!The import of
github.com/wk8/go-ordered-map/v2is necessary for the changes and is correctly added.
32-32: LGTM!The initialization of
vm.procedureswith an ordered map ensures consistent procedure order.
35-36: LGTM!The usage of
getProcedureto retrieve procedures aligns with the new ordered map structure.
41-41: LGTM!The usage of
setProcedureto set procedures aligns with the new ordered map structure.engine/vm_test.go (10)
16-16: LGTM!The usage of
Getto retrieve the procedure forarity: 0aligns with the new ordered map structure.
38-38: LGTM!The usage of
Getto retrieve the procedure forarity: 1aligns with the new ordered map structure.
58-58: LGTM!The usage of
Getto retrieve the procedure forarity: 2aligns with the new ordered map structure.
78-78: LGTM!The usage of
Getto retrieve the procedure forarity: 3aligns with the new ordered map structure.
98-98: LGTM!The usage of
Getto retrieve the procedure forarity: 4aligns with the new ordered map structure.
118-118: LGTM!The usage of
Getto retrieve the procedure forarity: 5aligns with the new ordered map structure.
138-138: LGTM!The usage of
Getto retrieve the procedure forarity: 6aligns with the new ordered map structure.
158-158: LGTM!The usage of
Getto retrieve the procedure forarity: 7aligns with the new ordered map structure.
178-178: LGTM!The usage of
Getto retrieve the procedure forarity: 8aligns with the new ordered map structure.
196-203: LGTM!The initialization of the
VMstruct with an ordered map usingbuildOrderedMapensures consistent procedure order. The changes are appropriate.engine/vm.go (14)
6-6: LGTM!The import of
github.com/wk8/go-ordered-map/v2is necessary for the changes and is correctly added.
55-55: LGTM!The
VMstruct now uses an ordered map for procedures, ensuring consistent order.
80-82: LGTM!The initialization of
vm.procedureswith an ordered map inRegister0ensures consistent procedure order.
88-90: LGTM!The initialization of
vm.procedureswith an ordered map inRegister1ensures consistent procedure order.
96-98: LGTM!The initialization of
vm.procedureswith an ordered map inRegister2ensures consistent procedure order.
104-106: LGTM!The initialization of
vm.procedureswith an ordered map inRegister3ensures consistent procedure order.
112-114: LGTM!The initialization of
vm.procedureswith an ordered map inRegister4ensures consistent procedure order.
120-122: LGTM!The initialization of
vm.procedureswith an ordered map inRegister5ensures consistent procedure order.
128-130: LGTM!The initialization of
vm.procedureswith an ordered map inRegister6ensures consistent procedure order.
136-138: LGTM!The initialization of
vm.procedureswith an ordered map inRegister7ensures consistent procedure order.
144-146: LGTM!The initialization of
vm.procedureswith an ordered map inRegister8ensures consistent procedure order.
181-181: LGTM!The usage of
getProcedureinArrivealigns with the new ordered map structure.
306-311: LGTM!The
getProceduremethod ensures safe retrieval of procedures from the ordered map.
313-318: LGTM!The
setProceduremethod ensures safe assignment of procedures to the ordered map.engine/text_test.go (15)
32-32: LGTM!The change to use
orderedmapfor theresultfield ensures consistent ordering of procedures.
54-70: EnsurebuildOrderedMapis correctly implemented.The code snippet uses the
buildOrderedMapfunction to initialize theorderedmap. Verify the correctness of this function to ensure it constructs the map as expected.
74-98: EnsurebuildOrderedMapis correctly implemented.The code snippet uses the
buildOrderedMapfunction to initialize theorderedmap. Verify the correctness of this function to ensure it constructs the map as expected.
102-190: EnsurebuildOrderedMapis correctly implemented.The code snippet uses the
buildOrderedMapfunction to initialize theorderedmap. Verify the correctness of this function to ensure it constructs the map as expected.
195-221: EnsurebuildOrderedMapis correctly implemented.The code snippet uses the
buildOrderedMapfunction to initialize theorderedmap. Verify the correctness of this function to ensure it constructs the map as expected.
226-259: EnsurebuildOrderedMapis correctly implemented.The code snippet uses the
buildOrderedMapfunction to initialize theorderedmap. Verify the correctness of this function to ensure it constructs the map as expected.
265-305: EnsurebuildOrderedMapis correctly implemented.The code snippet uses the
buildOrderedMapfunction to initialize theorderedmap. Verify the correctness of this function to ensure it constructs the map as expected.
308-339: EnsurebuildOrderedMapis correctly implemented.The code snippet uses the
buildOrderedMapfunction to initialize theorderedmap. Verify the correctness of this function to ensure it constructs the map as expected.
342-373: EnsurebuildOrderedMapis correctly implemented.The code snippet uses the
buildOrderedMapfunction to initialize theorderedmap. Verify the correctness of this function to ensure it constructs the map as expected.
376-393: EnsurebuildOrderedMapis correctly implemented.The code snippet uses the
buildOrderedMapfunction to initialize theorderedmap. Verify the correctness of this function to ensure it constructs the map as expected.
396-413: EnsurebuildOrderedMapis correctly implemented.The code snippet uses the
buildOrderedMapfunction to initialize theorderedmap. Verify the correctness of this function to ensure it constructs the map as expected.
489-506: EnsurebuildOrderedMapis correctly implemented.The code snippet uses the
buildOrderedMapfunction to initialize theorderedmap. Verify the correctness of this function to ensure it constructs the map as expected.
514-514: LGTM!The function
TestVM_Consultis unchanged.
Line range hint
561-561: LGTM!The function
TestDiscontiguousError_Erroris unchanged.
36-52: EnsurebuildOrderedMapis correctly implemented.The code snippet uses the
buildOrderedMapfunction to initialize theorderedmap. Verify the correctness of this function to ensure it constructs the map as expected.Verification successful
Let's inspect the implementation of the
buildOrderedMapfunction to verify its correctness.
Verification Successful:
buildOrderedMapis correctly implemented.The function correctly constructs an ordered map using the provided
procedurePairelements.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of buildOrderedMap function. # Test: Search for the function implementation. Expect: Correct implementation of buildOrderedMap. rg --type go 'func buildOrderedMap'Length of output: 161
Script:
#!/bin/bash # Fetch the implementation details of the buildOrderedMap function rg --type go --context 10 'func buildOrderedMap'Length of output: 1039
engine/builtin.go (12)
7-7: Import statement for ordered map.The import statement for
orderedmapis necessary for the changes.
705-705: Initialize procedures with an ordered map.The
vm.proceduresmap is correctly initialized with an ordered map.
707-707: Retrieve procedure using getProcedure method.The use of the
getProceduremethod abstracts the retrieval process, enhancing maintainability.
710-710: Set procedure using setProcedure method.The use of the
setProceduremethod abstracts the setting process, enhancing maintainability.
1070-1070: Initialize slice with the length of the ordered map.The slice is correctly initialized with the length of the ordered map, ensuring optimal performance.
1071-1071: Iterate over the ordered map using Oldest method.The iteration over the ordered map using the
Oldestmethod ensures consistent and predictable order.
1074-1074: Append functions based on ordered map's values.The functions are correctly appended based on the ordered map's values, ensuring the correct order.
1095-1095: Retrieve procedure using getProcedure method.The use of the
getProceduremethod abstracts the retrieval process, enhancing maintainability.
1146-1146: Retrieve procedure using getProcedure method.The use of the
getProceduremethod abstracts the retrieval process, enhancing maintainability.
1150-1150: Delete procedure from ordered map.The procedure is correctly deleted from the ordered map.
1987-1987: Retrieve procedure using getProcedure method.The use of the
getProceduremethod abstracts the retrieval process, enhancing maintainability.
2754-2754: Retrieve procedure using getProcedure method.The use of the
getProceduremethod abstracts the retrieval process, enhancing maintainability.engine/builtin_test.go (25)
10-10: Import statement for ordered map library looks good.The import of
github.com/wk8/go-ordered-map/v2is necessary for the changes.
21-22: Type aliasprocedurePairlooks good.Defining a type alias for
orderedmap.Pair[procedureIndicator, procedure]simplifies the code.
97-102: Initialization ofVMwith ordered map looks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
129-134: Initialization ofVMwith ordered map for arity 3 looks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
161-166: Initialization ofVMwith ordered map for arity 4 looks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
193-198: Initialization ofVMwith ordered map for arity 5 looks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
225-230: Initialization ofVMwith ordered map for arity 6 looks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
257-262: Initialization ofVMwith ordered map for arity 7 looks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
289-294: Initialization ofVMwith ordered map for arity 8 looks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
304-317: Initialization ofVMwith ordered map inTestCallNthlooks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
2459-2466: Initialization ofVMwith ordered map inTestCurrentPredicatelooks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
2483-2497: Initialization ofVMwith ordered map inTestCurrentPredicatewith multiple procedures looks good.The use of
buildOrderedMapto initialize theVMstruct with multiple procedures is correct.
2525-2573: Initialization ofVMwith ordered map inTestCurrentPredicatewith order checks looks good.The use of
buildOrderedMapto initialize theVMstruct with multiple procedures and the order checks are correct.
2693-2696: Retrieval of procedure pair usingGetPairinTestCurrentPredicatelooks good.The use of
GetPairto retrieve the procedure pair is correct.
2760-2765: Initialization ofVMwith ordered map inTestAbolishlooks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
2820-2820: Retrieval of procedure pair usingGetPairinTestAbolishlooks good.The use of
GetPairto retrieve the procedure pair is correct.
2894-2894: Retrieval of procedure pair usingGetPairinTestAbolishlooks good.The use of
GetPairto retrieve the procedure pair is correct.
2966-2971: Initialization ofVMwith ordered map inTestAbolishlooks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
3002-3011: Initialization ofVMwith ordered map inTestRetractlooks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
3029-3038: Initialization ofVMwith ordered map inTestRetractwith multiple procedures looks good.The use of
buildOrderedMapto initialize theVMstruct with multiple procedures is correct.
3056-3065: Initialization ofVMwith ordered map inTestRetractwith multiple procedures looks good.The use of
buildOrderedMapto initialize theVMstruct with multiple procedures is correct.
3074-3075: Retrieval of procedure usingGetinTestRetractlooks good.The use of
Getto retrieve the procedure is correct.
3105-3108: Initialization ofVMwith ordered map inTestRetractlooks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
3121-3128: Initialization ofVMwith ordered map inTestRetractlooks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
3149-3158: Initialization ofVMwith ordered map inTestAbolishlooks good.The use of
buildOrderedMapto initialize theVMstruct with procedures is correct.
bdeneux
force-pushed
the
feat/sorted-map-procedures
branch
from
814c243
to
e8116d8
Compare
bdeneux
changed the title
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
engine/term.go (1)
23-23: Add a comment to explain_opsfield.Consider adding a comment to explain the purpose of
_opsfield in theWriteOptionsstruct for better clarity._ops *operators // Ordered map for storing operators
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- .github/workflows/go.yml (1 hunks)
- engine/atom.go (1 hunks)
- engine/atom_test.go (2 hunks)
- engine/builtin.go (12 hunks)
- engine/builtin_test.go (38 hunks)
- engine/compound.go (1 hunks)
- engine/compound_test.go (2 hunks)
- engine/parser.go (9 hunks)
- engine/parser_test.go (2 hunks)
- engine/term.go (3 hunks)
- engine/text.go (7 hunks)
- engine/text_test.go (3 hunks)
- engine/vm.go (6 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/go.yml
Files skipped from review as they are similar to previous changes (3)
- engine/builtin.go
- engine/builtin_test.go
- engine/text_test.go
Additional context used
GitHub Check: codecov/patch
engine/text.go
[warning] 286-286: engine/text.go#L286
Added line #L286 was not covered by tests
[warning] 293-293: engine/text.go#L293
Added line #L293 was not covered by testsengine/parser.go
[warning] 300-300: engine/parser.go#L300
Added line #L300 was not covered by tests
Additional comments not posted (27)
engine/term.go (2)
61-66: LGTM! Proper initialization of_ops.The
getOpsmethod ensures_opsis always initialized before use.
69-85: LGTM! Proper initialization of ordered map indefaultWriteOptions.The ordered map in
defaultWriteOptionsis correctly initialized with the provided data.engine/atom_test.go (1)
27-28: LGTM! Proper initialization of ordered map in test cases.The test cases correctly initialize and use ordered maps.
engine/text.go (5)
31-38: LGTM! Proper usage of ordered map inCompile.The
Compilefunction correctly uses and initializes ordered maps for clauses.
81-81: LGTM! Proper initialization of ordered map incompile.The
compilefunction correctly initializes ordered maps for clauses.
223-223: LGTM! Proper initialization of ordered map intextstruct.The
textstruct correctly initializes ordered maps for clauses.
246-249: LGTM! Proper usage of ordered map inforEachUserDefined.The
forEachUserDefinedmethod correctly uses ordered maps for clauses.
271-274: LGTM! Proper usage of ordered map inflush.The
flushmethod correctly uses ordered maps for clauses.engine/compound_test.go (1)
Line range hint
17-53: LGTM! Proper initialization of ordered map in test cases.The test cases correctly initialize and use ordered maps.
engine/vm.go (4)
6-6: Import ofgo-ordered-maplibrary.The import statement for the
github.com/wk8/go-ordered-map/v2package is correctly added.
Line range hint
55-64:
UpdatedVMstruct fields.The
proceduresfield is now an ordered map and theoperatorsfield is updated to_operators.
154-154: Updated procedure retrieval inArrivemethod.The
Arrivemethod now usesvm.getProcedure(pi)to retrieve procedures from the ordered map.
279-298: New methods for managing ordered maps.The new methods
getProcedure,setProcedure, andgetOperatorsencapsulate the logic for accessing and modifying the ordered map.engine/parser_test.go (1)
25-25: Initialization ofoperatorsfield.The
operatorsfield is now initialized using thenewOperatorsfunction.engine/atom.go (1)
244-244: UpdatedWriteTermmethod.The
WriteTermmethod now callsopts.getOps()to retrieve operators.engine/compound.go (1)
48-52: UpdatedWriteCompoundfunction.The
WriteCompoundfunction now callsopts.getOps().Get(c.Functor())to retrieve operators.engine/parser.go (11)
6-6: Import of ordered map library is correct.The addition of
github.com/wk8/go-ordered-map/v2is necessary for ordered map functionality.
25-25: Addition of_operatorsfield is correct.The
_operatorsfield is added to theParserstruct to store operators using an ordered map.
49-49: Initialization of_operatorsis correct.The
NewParserfunction initializes_operatorsusingvm.getOperators(), ensuring the use of an ordered map for operators.
259-261: Addition ofoperatorsstruct is correct.The
operatorsstruct encapsulates the ordered map for managing operators.
263-265: Addition ofnewOperatorsfunction is correct.The
newOperatorsfunction initializes anoperatorsinstance with an ordered map.
268-269: Addition ofdefinedmethod is correct.The
definedmethod checks if an operator is defined in the ordered map.
273-276: Addition ofdefinedInClassmethod is correct.The
definedInClassmethod checks if an operator is defined in a specific class.
283-290: Addition ofdefinemethod is correct.The
definemethod adds or updates an operator in the ordered map.
294-300: Addition ofremovemethod is correct.The
removemethod deletes an operator from the ordered map.Tools
GitHub Check: codecov/patch
[warning] 300-300: engine/parser.go#L300
Added line #L300 was not covered by tests
343-348: Addition ofgetOperatorsmethod is correct.The
getOperatorsmethod returns the_operatorsfield, initializing it if necessary.
425-426: Usage ofgetOperatorsmethod is correct.The
prefix,infix, andargmethods usegetOperatorsto access operators, ensuring the use of the ordered map.Also applies to: 440-441, 762-762
bdeneux
force-pushed
the
feat/sorted-map-procedures
branch
from
6245682
to
fbf751b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
codecov.ymlis excluded by!**/*.yml
Files selected for processing (1)
- .github/workflows/go.yml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/go.yml
bdeneux
force-pushed
the
feat/sorted-map-procedures
branch
from
fbf751b
to
3a315bf
Compare
📝 Description
Following the issue concerning the random display order of predicates listed by
current_predicate/1in the Axone Protocol chain (refer to Issue #695 for more details), this PR implements a significant modification to the underlying data structure that maintains the list of registered procedures. Specifically, it replaces the conventional map with an ordered map.The decision to adopt the second solution proposed in the issue (switching to an ordered map instead of sorting the procedures map before returning results) was driven by the foundational role of the procedures map in predicate operations. Ensuring consistent behavior across nodes necessitates a stable base for all predicates. This change guarantees that if any predicate relies on the map's order, it will operate on a consistent and predictable foundation, thereby enhancing the reliability and predictability of predicate executions across the network.
🔀 Ordered map libraries
I have explored several libraries that provide ordered map functionality in Go:
https://github.com/elliotchance/orderedmaphttps://github.com/wk8/go-ordered-maphttps://github.com/emirpasic/godsAfter careful consideration, I decided to use https://github.com/wk8/go-ordered-map due to its minimalistic approach and efficiency. The decision was further reinforced by the library's comprehensive comparison with alternatives, detailed at https://github.com/wk8/go-ordered-map?tab=readme-ov-file#alternatives. Initially, I had opted for
https://github.com/elliotchance/orderedmap, but encountered difficulties when attempting to compare two ordered maps in a testing scenario.Impact
Three maps has been impacted on the refactoring and replaced by an ordered map.
ProceduresmapClausesmapOperatorsmap🔗 Related links
current_predicaterandom display axoned#695