Implement SQL Planner for Logical Plans and Statement Validation - #38
Implement SQL Planner for Logical Plans and Statement Validation#38rahulc0dy wants to merge 13 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. WalkthroughThe PR adds a SQL planner that resolves catalog identifiers, expressions, conditions, and types, then produces logical plans for SELECT, DDL, INSERT, UPDATE, and DELETE statements with diagnostics and unit tests. ChangesSQL planner implementation
Sequence Diagram(s)sequenceDiagram
participant Client
participant Planner
participant Catalog
participant PlanContext
participant LogicalPlan
Client->>Planner: Submit AST statement
Planner->>PlanContext: Create planning context
PlanContext->>Catalog: Resolve databases, tables, and columns
Catalog-->>PlanContext: Return catalog metadata
PlanContext->>LogicalPlan: Construct resolved plan tree
LogicalPlan-->>Planner: Return plan and diagnostics
Planner-->>Client: Return planning result
Assessment against linked issues
Out-of-scope changes
Merge Risk: ⚪ Minimal · up to This PR adds SQL planning and statement validation without any identified merge-blocking issue; it is merge-ready after normal checks and review. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sql/planner/statements.go (1)
167-168: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun
goimportsbefore merging.The committed file fails the required lint check at Line 167. Apply
goimportsto remove the formatting discrepancy.goimports -w internal/sql/planner/statements.goSources: Linters/SAST tools, Pipeline failures
🧹 Nitpick comments (1)
internal/sql/planner/plan_statements_test.go (1)
267-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
ADD COLUMN ... UNIQUE/... PRIMARY KEYrejection.
planAlterSchema'sAlterAddcase has dedicated "not supported in v1" diagnostics forUniqueandPrimaryKeycolumns, but no test here exercises either path (only the NOT-NULL-without-default and duplicate-column paths are covered).func TestPlanAlterTable_AddUniqueColumn_Errors(t *testing.T) { pc := newPlanContext(testCatalog(), Session{ActiveDatabase: "shop"}, nil) stmt := &ast.AlterTableStmt{ Table: ident("users"), Action: &ast.AlterAction{Kind: ast.AlterAdd, Column: colDef("email", ast.TypeText, &ast.UniqueConstraint{})}, } _, err := pc.planAlterTable(stmt) if err == nil || pc.diag[len(pc.diag)-1].Code != CodeUnsupportedAlter { t.Errorf("expected CodeUnsupportedAlter, got err=%v diag=%+v", err, pc.diag) } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8dbf5033-632c-4ebb-8109-c14f73ba7f10
📒 Files selected for processing (19)
internal/sql/planner/.gitkeepinternal/sql/planner/aggregate.gointernal/sql/planner/conditions.gointernal/sql/planner/errors.gointernal/sql/planner/expressions.gointernal/sql/planner/operators.gointernal/sql/planner/plan_conditions.gointernal/sql/planner/plan_conditions_test.gointernal/sql/planner/plan_expressions.gointernal/sql/planner/plan_expressions_test.gointernal/sql/planner/plan_queries.gointernal/sql/planner/plan_queries_test.gointernal/sql/planner/plan_statements.gointernal/sql/planner/plan_statements_test.gointernal/sql/planner/planner.gointernal/sql/planner/resolve.gointernal/sql/planner/resolve_test.gointernal/sql/planner/statements.gointernal/sql/planner/typecheck.go
|
/coverage |
Coverage report (
|
| Package | Coverage | Statements |
|---|---|---|
internal/sql/planner |
81.4% | 717/881 |
| TOTAL (PR-affected) | 81.4% | 717/881 |
Treemap — by file
Box size = statements per file. Color = coverage heat, red (low) → green (high).
Treemap — by package (with %)
Box size = statements per package. Color = coverage heat, red (low) → green (high).
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0f230f9-5090-44b1-85dc-1f0848b98004
📒 Files selected for processing (11)
internal/sql/planner/aggregate.gointernal/sql/planner/errors.gointernal/sql/planner/plan_conditions.gointernal/sql/planner/plan_conditions_test.gointernal/sql/planner/plan_expressions.gointernal/sql/planner/plan_expressions_test.gointernal/sql/planner/plan_queries.gointernal/sql/planner/plan_queries_test.gointernal/sql/planner/plan_statements.gointernal/sql/planner/plan_statements_test.gointernal/sql/planner/typecheck.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/sql/planner/aggregate.go
- internal/sql/planner/plan_expressions.go
- internal/sql/planner/typecheck.go
- internal/sql/planner/plan_queries.go
- internal/sql/planner/plan_expressions_test.go
Souvik606
left a comment
Souvik606
left a comment
There was a problem hiding this comment.
Please check following reviews and do the needful.
| // ResolvedNullLiteral is a resolved NULL literal. It is compatible with | ||
| // every type and every operator; callers must check for this type (via | ||
| // isNullExpr) before consulting ResolvedType, whose return value here is a | ||
| // meaningless placeholder. | ||
| type ResolvedNullLiteral struct{} | ||
|
|
||
| func (*ResolvedNullLiteral) resolvedExprNode() {} | ||
| func (*ResolvedNullLiteral) ResolvedType() ast.DataTypeKind { return ast.TypeInt } |
There was a problem hiding this comment.
If any developer or future helper calls e.ResolvedType() directly without first checking isNullExpr(e), NULL will be silently treated as an INT. This can cause corrupt type coercion, invalid arithmetic promotion, or improper formatting in execution results.
Add a dedicated ast.TypeNull to DataTypeKind instead of returning a misleading TypeInt placeholder.
| type ResolvedColumnRef struct { | ||
| Column *ResolvedColumn | ||
| } |
There was a problem hiding this comment.
If any pass in the planner or executor mutates a ResolvedColumn in place (e.g. changing Index, Name, or Type), it mutates all references across the plan simultaneously without thread/memory safety.
Store ResolvedColumn by value or mark it immutable or think of some brilliant idea
| type ResolvedComparison struct { | ||
| ResolvedCondBase | ||
| Left ResolvedExpr | ||
| Op utils.TokenType |
There was a problem hiding this comment.
Tightly couples planner structs to raw lexer token constants.Planner should just have idea about its previous stage i.e parser stage.
Define an explicit planner operator enum.
| // completes and ResolvedExpr/ResolvedCond trees deliberately carry no | ||
| // equivalent — by the time those are built, any problem they'd have | ||
| // caused has already been turned into a diagnostic. | ||
| Span diagnostic.Span |
There was a problem hiding this comment.
Span is only used during planning error checks. Retaining Span on every ProjectItem inflates struct memory footprint during execution across large projection lists.
Strip Span after planning completes, or keep it in a temporary planning wrapper struct.
| Count int | ||
| Offset int |
There was a problem hiding this comment.
Count and Offset are typed as signed int.Allows negative values (e.g. Count = -5, Offset = -10) to be represented in the struct if validation fails or is bypassed.
Use int64 with non-negative validation or uint64
| Columns []*ResolvedColumn | ||
| Rows [][]ResolvedExpr |
There was a problem hiding this comment.
InsertPlan contains both Rows [][]ResolvedExpr (for VALUES) and Source *QueryPlan (for INSERT ... SELECT).
The struct permits invalid states where both Rows and Source are non-nil, or both are nil. The Go type system cannot enforce mutual exclusion.
Write a validator to enforce mutual exclusion like its written at AST level.
| type DropDatabasePlan struct { | ||
| PlanBase | ||
| Name string | ||
| Tables []*catalog.TableMeta |
There was a problem hiding this comment.
The executor only needs table names to delete keys. Holding full *catalog.TableMeta pointers needlessly pins large catalog schema trees in memory.
Store a slice of table names Tables []string instead of full metadata pointers.
| type Session struct { | ||
| ActiveDatabase string | ||
| } |
There was a problem hiding this comment.
Session struct contains only ActiveDatabase string.It cannot pass connection context (context.Context), query execution timeouts etc
Expand Session struct:
type Session struct {
ActiveDatabase string
Ctx context.Context
}
Need to check context cancellation stuffs in the concrete planner implementation codes too.Do it comfortably.
3 participants
Issue Reference
Summary by CodeRabbit