engine: remove ng module

[carllerche]

No description provided.

[claude]

PR Review: Remove ng module

Summary

This PR removes the ng ("next generation") planner module by promoting it to the main planner implementation. The change consolidates ~400 lines of wrapper code, simplifying the architecture by moving ng submodules (info, lower, materialize) directly into the planner module.

Positive Aspects

1. Architecture Simplification ✅

• Removes unnecessary indirection layer (PlannerNg wrapper)
• Consolidates duplicate state management
• Cleaner module structure with direct imports
• Net deletion of 481 lines vs 329 additions shows genuine simplification

2. Code Quality ✅

• Follows established patterns from docs/ARCHITECTURE.md
• Maintains separation of concerns (lowering → materialization → execution)
• Preserves existing Visit/VisitMut patterns for AST transformations
• Import statements moved from glob imports to specific imports (good practice)

3. Incremental Migration Complete ✅

• Successfully completes the v2 planner migration
• Removes the temporary plan_v2_stmt wrapper, making it the primary implementation
• Eliminates old planner reference indirection

Concerns and Recommendations

1. Testing Gap ⚠️
The PR shows no changes to test files. Given this is a significant refactoring that:

• Removes an entire module
• Changes how the planner is initialized
• Consolidates execution paths

Recommendation: Confirm that the existing integration tests in tests/ adequately cover:

• All MaterializeKind variants (ExecStatement, NestedMerge, Project, etc.)
• Edge cases in statement decomposition
• Multi-database driver compatibility

2. Removed Helper Method ⚠️
The push_action method was removed:

// Removed:
fn push_action(&mut self, action: impl Into<plan::Action>) {
    let action = action.into();
    self.verify_action(&action);  // ← verification step lost
    self.actions.push(action);
}

Now actions are pushed directly without verification. This could hide bugs.

Recommendation:

• If verify_action was providing valuable debug assertions, consider adding it back
• If it was dead code, document why verification is no longer needed

3. Module Documentation 📝
The extensive documentation from ng.rs (lines 1-116) was removed, which described:

• High-level strategy
• Planning process phases
• Execution order guarantees
• Example execution plans

Recommendation: This documentation should be preserved, either:

• In planner.rs module-level docs
• In docs/ARCHITECTURE.md
• As comments near plan_v2_stmt

This is especially important for future maintainers understanding the materialization graph.

4. Error Handling 🔍
Multiple todo!() macros remain in production code:

// Line 172
let ty_fields = match &**ty_rows {
    stmt::Type::Record(ty_fields) => ty_fields.clone(),
    _ => todo!("ty={ty:#?}; node={node:#?}"),
};

// Lines 245, 333
_ => todo!()

Recommendation:

• Document which type combinations are unsupported
• Add issue references for incomplete features
• Consider returning proper errors instead of panicking

5. Deleted verify.rs and subquery.rs ❓
These files were removed entirely:

• verify.rs (5 lines) - appears to be dead code, OK to remove
• subquery.rs (44 lines) - unclear if functionality moved elsewhere

Question: Was subquery.rs functionality already integrated into the materialization logic, or is there a feature gap?

Performance Considerations

✅ No performance regressions expected:

• Removes allocation overhead from wrapper struct
• Maintains existing execution graph topology
• No changes to database interaction patterns

Security Considerations

✅ No security concerns identified:

• No changes to SQL generation or query execution
• No new unsafe code
• No changes to external interfaces

Minor Nits

1. Line 56: Uses debug_assert! with complex expression - could be clearer:

debug_assert!(
    {
        match &m.stmt {
            stmt::Statement::Query(query) => !query.single,
            _ => true,
        }
    },
    "as of now, no database can execute single queries"
);

2. Import organization: Consider grouping imports by category (std, external crates, internal modules)

Verdict

Approve with minor follow-ups ✅

This is a solid refactoring that achieves its goal of removing the temporary ng module. The main concerns are:

1. Preserve the planning documentation
2. Verify test coverage for the consolidated code
3. Clarify the removed verification logic

The architectural improvement outweighs the minor issues, which can be addressed in follow-up PRs.

---

Review performed using project guidelines from CLAUDE.md and ARCHITECTURE.md