chore(tesseract): Move SQL rendering parts from sql_evaluator to plan

[waralexrom]

Check List

• Tests have been run in packages where changes have been made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet
• Docs have been added / updated if required

[claude]

Claude finished @waralexrom's task in 2m 57s —— View job

---

Code Review: PR #10836

• Gather context and understand the changes
• Review the diff for correctness
• Check for import path consistency
• Verify module structure changes
• Post comprehensive review

---

Summary

This PR completes a three-step refactoring of the Tesseract native SQL planner:

1. Commit 612ca5a — Moves SQL rendering parts (sql_nodes, sql_visitor, symbols/to_sql, cube_ref_evaluator, references_builder) from planner/sql_evaluator/ into plan/
2. Commit fb3bab2 — Renames the plan/ module to physical_plan/
3. Commit bf13761 — Scatters the remaining sql_evaluator contents (collectors, compiler, symbols, sql_call, visitor) into planner/ directly and removes the sql_evaluator module entirely

The result is a cleaner separation of concerns: planner/ owns logical/compilation concerns (symbols, collectors, compiler, sql_call), while physical_plan/ owns SQL rendering (sql_nodes, visitor, filters, select/from/join structures).

198 files changed, but the net delta is only +2 lines — this is almost entirely renames and import path updates.

---

Review Findings

Overall: This is a clean, well-structured refactoring. No bugs, no security issues, no behavioral changes.

Correctness ✅

• All crate::plan:: references have been updated to crate::physical_plan::
• All planner::sql_evaluator:: references have been updated — either to physical_plan:: (for rendering code) or directly to planner:: (for compilation code)
• No stale references remain: I verified with grep that zero files reference the old crate::plan::, sql_evaluator, or super::sql_evaluator paths
• The old plan/ directory and planner/sql_evaluator/ directory are fully removed

Module Structure ✅

• physical_plan/mod.rs has a clean set of pub use re-exports covering all the types consumers need
• planner/mod.rs now directly exports collectors, compiler, symbols, sql_call, and visitor instead of routing through sql_evaluator
• VisitorContext was correctly split: FiltersContext stays in planner/visitor_context.rs, while the SQL-rendering VisitorContext and its evaluate_with_context/evaluate_sql_call_with_context functions move to physical_plan/visitor_context.rs

Commit Hygiene ✅

• Each commit is logically self-contained and follows the chore(tesseract): conventional commit format
• The progression (move rendering code → rename → scatter remaining) makes the changes easy to review commit-by-commit

Pre-existing Items (Not Introduced by This PR)

These are pre-existing in the base branch and were carried forward without change:

• Typo validete_member_for_leaf_query — should be validate_member_for_leaf_query (references_builder.rs:79)
• Typo has_source_for_leaf_memeber — should be has_source_for_leaf_member (references_builder.rs:162)
• Typo SecutityContextProps — should be SecurityContextProps (member_sql.rs:178)
• Commented-out error returns in references_builder.rs (lines 38-40, 68-72, 90-94) — these appear to be intentionally disabled for now

These are outside the scope of this PR but worth noting for a follow-up.

Performance & Security ✅

• No behavioral changes — purely structural
• No new allocations, no API changes, no new unsafe code

Testing

• Since this is a pure rename/restructure with no logic changes, existing tests provide adequate coverage. No new tests needed.

---

Verdict

LGTM — This is a well-executed structural refactoring that improves module organization without changing behavior. The commit breakdown is logical and the old paths are cleanly eliminated.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.83%. Comparing base (10f249e) to head (0e7b40f).

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #10836   +/-   ##
=======================================
  Coverage   78.82%   78.83%           
=======================================
  Files         470      470           
  Lines       92292    92292           
  Branches     3436     3436           
=======================================
+ Hits        72750    72755    +5     
+ Misses      19039    19034    -5     
  Partials      503      503           

Flag	Coverage Δ
cube-backend	58.16% <ø> (ø)
cubesql	83.47% <ø> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @waralexrom's task in 1m 47s —— View job

---

Code Review: PR #10836

• Gather context and understand the changes
• Review the full diff for correctness and hidden behavioral changes
• Verify no stale import paths remain
• Check module structure and re-exports
• Post comprehensive review

---

Summary

This PR completes a three-step structural refactoring of the Tesseract native SQL planner (cubesqlplanner):

1. 13e30fe — Moves SQL rendering code (sql_nodes, sql_visitor, symbols/to_sql, cube_ref_evaluator, references_builder) from planner/sql_evaluator/ into plan/
2. c037e6e — Renames plan/ to physical_plan/
3. 0e7b40f — Scatters the remaining sql_evaluator contents (collectors, compiler, symbols, sql_call, visitor) directly into planner/ and removes the sql_evaluator module

198 files changed, but the net delta is +2 lines — this is purely structural with no behavioral changes.

---

Review Findings

Correctness ✅

• Zero stale references: Verified with grep — no files reference the old crate::plan::, sql_evaluator, or super::sql_evaluator paths
• Old directories removed: Both plan/ and planner/sql_evaluator/ are fully gone
• Import paths updated consistently across all consumers (logical_plan/, physical_plan_builder/, planner/, tests/, test_fixtures/)

Module Structure ✅

• physical_plan/mod.rs — Clean pub use re-exports for all rendering types (Select, From, Join, Expr, VisitorContext, ReferencesBuilder, SqlEvaluatorVisitor, etc.)
• planner/mod.rs — Now directly exports collectors, compiler, symbols, sql_call, and visitor instead of routing through sql_evaluator
• VisitorContext split done correctly: FiltersContext stays in planner/visitor_context.rs (compilation concern), while VisitorContext + evaluate_with_context / evaluate_sql_call_with_context moved to physical_plan/visitor_context.rs (rendering concern)

Commit Hygiene ✅

• Each commit is logically self-contained and follows chore(tesseract): conventional commit format
• The progression (move rendering → rename → scatter) makes review straightforward

Performance & Security ✅

• No behavioral changes, no new allocations, no API changes, no unsafe code

Pre-existing Items (Not Introduced by This PR)

Carried forward without change — noted for a possible follow-up:

• Typo validete_member_for_leaf_query → validate_member_for_leaf_query (references_builder.rs:79)
• Typo has_source_for_leaf_memeber → has_source_for_leaf_member (references_builder.rs:162, 170)
• Typo SecutityContextProps → SecurityContextProps (member_sql.rs:178)
• Commented-out error returns in references_builder.rs (lines 38-40, 68-72, 90-94) — appear intentionally disabled

Testing

Pure rename/restructure — existing tests provide adequate coverage. Codecov confirms zero coverage change.

---

Verdict

LGTM — This is a well-executed structural refactoring that cleanly separates compilation concerns (planner/) from SQL rendering concerns (physical_plan/). No bugs, no security issues, no hidden behavioral changes.