Skip to content

refactor(query_planner): Extract TableCtx and AST conversions to sepa…#51

Merged
genezhang merged 2 commits intomainfrom
refactor/plan-ctx-logical-expr-cleanup
Jan 28, 2026
Merged

refactor(query_planner): Extract TableCtx and AST conversions to sepa…#51
genezhang merged 2 commits intomainfrom
refactor/plan-ctx-logical-expr-cleanup

Conversation

@genezhang
Copy link
Copy Markdown
Owner

…rate modules

Code quality improvements to bring modules to Grade A level:

logical_expr/mod.rs (1404 → 947 lines, -33%):

  • Extract duplicated AST conversions to ast_conversion.rs (525 lines)
  • Module now contains only type definitions and core logic

plan_ctx/mod.rs (1391 → 1160 lines, -17%):

  • Extract TableCtx to table_ctx.rs (310 lines)
  • Refactor new_with_cte_reference to break circular dependency
  • Update graph_join_inference.rs to use new API

Files added:

  • logical_expr/ast_conversion.rs: From/TryFrom impls for AST conversion
  • logical_expr/combinators.rs: Expression combinators (already existed, now declared)
  • logical_expr/visitors.rs: Visitor pattern for expression traversal
  • plan_ctx/table_ctx.rs: TableCtx struct and implementation
  • plan_ctx/builder.rs: Builder pattern (already existed, now declared)

All 832 tests passing (single-threaded; pre-existing flaky test in parallel mode)

…rate modules

Code quality improvements to bring modules to Grade A level:

**logical_expr/mod.rs** (1404 → 947 lines, -33%):
- Extract duplicated AST conversions to ast_conversion.rs (525 lines)
- Module now contains only type definitions and core logic

**plan_ctx/mod.rs** (1391 → 1160 lines, -17%):
- Extract TableCtx to table_ctx.rs (310 lines)
- Refactor new_with_cte_reference to break circular dependency
- Update graph_join_inference.rs to use new API

Files added:
- logical_expr/ast_conversion.rs: From/TryFrom impls for AST conversion
- logical_expr/combinators.rs: Expression combinators (already existed, now declared)
- logical_expr/visitors.rs: Visitor pattern for expression traversal
- plan_ctx/table_ctx.rs: TableCtx struct and implementation
- plan_ctx/builder.rs: Builder pattern (already existed, now declared)

All 832 tests passing (single-threaded; pre-existing flaky test in parallel mode)
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request refactors the query planner modules by extracting code into separate, focused modules to improve code quality and maintainability. The primary goal is to achieve Grade A code quality by reducing module sizes and improving code organization.

Changes:

  • Extracted TableCtx from plan_ctx/mod.rs to plan_ctx/table_ctx.rs (310 lines)
  • Extracted AST conversion implementations from logical_expr/mod.rs to logical_expr/ast_conversion.rs (525 lines)
  • Added new helper modules: visitors.rs, combinators.rs, and builder.rs with comprehensive tests

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/query_planner/plan_ctx/table_ctx.rs New module containing TableCtx struct with well-organized methods and documentation
src/query_planner/plan_ctx/mod.rs Removed TableCtx implementation, added re-export for backward compatibility
src/query_planner/plan_ctx/builder.rs New builder pattern implementation for PlanCtx with fluent API and tests
src/query_planner/logical_expr/visitors.rs New visitor pattern implementation for expression traversal with helper visitors
src/query_planner/logical_expr/combinators.rs New predicate combinator functions for combining boolean expressions
src/query_planner/logical_expr/ast_conversion.rs Extracted AST conversion implementations with improved logging (println → log::trace)
src/query_planner/logical_expr/mod.rs Reduced from 1404 to 947 lines by extracting conversions and adding module declarations
src/query_planner/analyzer/graph_join_inference.rs Updated to use new new_with_cte_reference API that breaks circular dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@genezhang genezhang merged commit c010cb5 into main Jan 28, 2026
2 checks passed
@genezhang genezhang deleted the refactor/plan-ctx-logical-expr-cleanup branch January 28, 2026 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants