engine: remove 2 from types and fns

[carllerche]

No description provided.

[claude]

PR Review: Remove '2' suffix from types and functions

Summary

This is a cleanup PR that removes the '2' suffix from various types and function names in the engine module, making the codebase cleaner and more maintainable. The changes are mostly mechanical renames with no functional changes.

Positive Aspects ✅

1. Consistency: The PR successfully removes the '2' suffix from most Action variants and their associated types (e.g., ExecStatement2 → ExecStatement, Output2 → Output)

2. File Renames: Properly renamed both module files and their internal implementations:

   • kv2.rs → kv.rs
   • exec_statement2.rs → exec_statement.rs

3. Comprehensive Coverage: Updated all the following types and their usages:

   • ExecStatement2 → ExecStatement
   • FindPkByIndex2 → FindPkByIndex
   • GetByKey2 → GetByKey
   • QueryPk2 → QueryPk
   • ReadModifyWrite2 → ReadModifyWrite
   • SetVar2 → SetVar
   • Output2 → Output

4. Clean Refactor: All call sites and pattern matches were updated consistently

Issues Found 🔴

Incomplete Cleanup: Several functions still retain the '2' suffix that should be renamed for consistency:

1. try_build_key_filter2 in crates/toasty/src/engine/kv.rs:37

   • Declaration at line 37
   • Usage at crates/toasty/src/engine/planner/materialize.rs:560
   • Should be renamed to try_build_key_filter

2. plan_index_path2 in crates/toasty/src/engine/index.rs:16

   • Declaration at line 16
   • Usage at crates/toasty/src/engine/planner/materialize.rs:532
   • Should be renamed to plan_index_path

3. collect_input2 in crates/toasty/src/engine/exec.rs:73

   • Declaration at line 73
   • Usages at:
     • crates/toasty/src/engine/exec/find_pk_by_index.rs:19
     • crates/toasty/src/engine/exec/query_pk.rs:12
   • Should be renamed to collect_input

4. compute_materialization_execution_order2 in crates/toasty/src/engine/planner/materialize.rs:1098

   • Declaration at line 1098
   • Usages at lines 1070 and 1115 (recursive call)
   • Should be renamed to compute_materialization_execution_order

Recommendations

1. Complete the cleanup: Rename the remaining functions to remove the '2' suffix for full consistency
2. Search pattern: Use \b\w+2\s*\( to find any remaining '2'-suffixed functions
3. Update call sites: Make sure to update all call sites when renaming the functions

Code Quality Assessment

• Correctness: ✅ The changes are mechanical renames with proper updates to all references
• Testing: ⚠️ Should verify tests pass after addressing the incomplete renames
• Performance: ✅ No performance impact (zero-cost refactor)
• Security: ✅ No security concerns
• Best Practices: ⚠️ Should complete the cleanup to maintain consistency

Conclusion

This is good cleanup work, but it's incomplete. The PR should either:

1. Be updated to include the remaining '2'-suffixed functions, OR
2. Have a clear comment explaining why those functions retain the suffix (if intentional)

Given the PR title "remove 2 from types and fns", I recommend option 1 - completing the cleanup for full consistency across the codebase.

[claude]

Code Review

This is a clean refactoring PR that removes the "2" suffix from engine types and functions. The changes are well-executed and improve code clarity.

Positive Observations

1. Comprehensive and Consistent

• All references updated systematically across 27 files
• File renames (kv2.rs → kv.rs, exec_statement2.rs → exec_statement.rs) match the type renames
• No orphaned "2" suffixes left behind

2. Code Quality Improvements

• filter_expr_unwrap() helper method added (crates/toasty-core/src/stmt/filter.rs:119-123) - good abstraction that simplifies code in plan_index_path()
• The simplified plan_index_path() in crates/toasty/src/engine/index.rs:16-26 is cleaner after removing the intermediate plan_index_path() overload

3. Zero Behavioral Changes

• Pure rename refactoring with no logic changes
• Low risk of introducing bugs

Minor Observations

1. New Helper Method
The filter_expr_unwrap() method uses .expect() which will panic if there's no filter expression. This is fine for internal engine code where the precondition is guaranteed, but consider documenting when it's safe to call (e.g., "Panics" section in doc comment).

2. Naming Rationale
The "2" suffixes suggest this was a migration from v1 to v2 implementations. Now that v1 is gone, removing the suffixes makes perfect sense. This improves readability significantly.

Testing Considerations

• No test changes needed - this is a pure refactoring
• Existing tests will validate behavior is preserved
• CI should catch any compilation issues

Verdict

Approved ✅

This is exactly the kind of technical debt cleanup that keeps a codebase healthy. The changes are mechanical, comprehensive, and make the code more maintainable. Great work!

---

Review generated with assistance from Claude Code