Implement SQL Planner for Logical Plans and Statement Validation - #38
Implement SQL Planner for Logical Plans and Statement Validation#38rahulc0dy wants to merge 14 commits into
Conversation
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: 🟡 Moderate · up to The planner can currently accept invalid grouped queries involving self-joins, while related validation paths inconsistently handle NULL values and internal NULL types. These are bounded but concrete SQL correctness issues, so the PR is not merge-ready until they are fixed or explicitly accepted. Warning Your free Security trial is over. An organization admin can activate billing to continue. 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.
| 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 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.
There was a problem hiding this comment.
Actionable comments posted: 1
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/plan_statements.go (1)
406-414: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAllow NULL results from
INSERT ... SELECT.When a SELECT output has the NULL type,
typesCompatiblerejects it against a typed target column. The VALUES path usesexprsCompatible, which accepts NULL expressions. Apply the same NULL rule here. Add coverage forINSERT INTO ... SELECT NULL.
♻️ Duplicate comments (1)
internal/sql/planner/aggregate.go (1)
8-9: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve relation-instance identity in GROUP BY comparisons.
Line 9 compares only physical-column fields. In a self-join,
u1.idandu2.idhave the same compared values. Therefore,GROUP BY u1.idincorrectly permitsSELECT u2.id.Store a relation binding or immutable relation-instance key in the resolved column identity. Compare that key here. Add a self-join regression test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fb4b531-f7b6-4880-a0ef-68de851315ac
📒 Files selected for processing (17)
internal/sql/ast/clauses.gointernal/sql/planner/aggregate.gointernal/sql/planner/conditions.gointernal/sql/planner/errors.gointernal/sql/planner/expressions.gointernal/sql/planner/op.gointernal/sql/planner/op_convert.gointernal/sql/planner/operators.gointernal/sql/planner/plan_conditions.gointernal/sql/planner/plan_expressions.gointernal/sql/planner/plan_expressions_test.gointernal/sql/planner/plan_queries.gointernal/sql/planner/plan_statements.gointernal/sql/planner/plan_statements_test.gointernal/sql/planner/planner.gointernal/sql/planner/statements.gointernal/sql/planner/typecheck.go
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| TypeFloat // FLOAT | ||
| TypeDouble // DOUBLE | ||
| TypeDecimal // DECIMAL | ||
| TypeNull // NULL (sentinel; not a user-facing SQL type) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject TypeNull in DataType.Validate.
TypeNull is an internal sentinel. DataType.Validate accepts it because no validation branch rejects it. A programmatic DDL AST can then define a column with NULL as its declared type.
Reject TypeNull during AST validation. Use it only for resolved expressions.
Souvik606
left a comment
Souvik606
left a comment
There was a problem hiding this comment.
Please review these comments and fix as desirable and correct.
| // same physical column — used to check whether a SELECT-list or HAVING | ||
| // column reference matches one of the query's GROUP BY keys. | ||
| func columnRefsEqual(a, b *ResolvedColumn) bool { | ||
| return a == b |
There was a problem hiding this comment.
Compare by value identity instead
func columnRefsEqual(a, b *ResolvedColumn) bool {
return a.Database == b.Database && a.Table == b.Table && a.Name == b.Name
}
| if qualifier != "" { | ||
| return qualifier, nil | ||
| } |
There was a problem hiding this comment.
When qualifier != "", the function returns it without verifying that the database actually exists in the catalog.Add a check
| Name: col.Name, | ||
| Index: index, | ||
| Type: col.Type, | ||
| VarcharLen: col.VarcharLen, |
There was a problem hiding this comment.
VarcharLen *int is a pointer. This copies the pointer from catalog.ColumnMeta into ResolvedColumn. Both the catalog and the plan now share the same *int. If either side ever mutates the integer value through the pointer, it silently changes the other.So better deep-copy the pointer:
| case *ResolvedFunctionCall: | ||
| return nil |
There was a problem hiding this comment.
validateGroupedExpr stops recursion at ResolvedFunctionCall. This correctly allows SUM(salary) in a GROUP BY query. But it also silently allows nested aggregates like SUM(COUNT(*)), which is illegal in standard SQL.Add a nested-aggregate check.
| return &ResolvedBinaryExpr{ResolvedExprBase: newExprBase(resultType), Left: left, Op: be.Op, Right: right}, nil | ||
| } | ||
|
|
||
| func (pc *planContext) resolveUnaryExpr(scope *Scope, ue *ast.UnaryExpr) (ResolvedExpr, error) { |
There was a problem hiding this comment.
Negative float literals carry an extra unary operator node during execution, requiring extra CPU dispatch per row. Add float literal folding like integer
| func (pc *planContext) resolveSelectExpression(scope *Scope, se *ast.SelectExpression) (ResolvedExpr, error) { | ||
| if se.Expr != nil { | ||
| return pc.resolveExpr(scope, se.Expr) | ||
| } | ||
| cond, err := pc.resolveCond(scope, se.Cond) |
There was a problem hiding this comment.
If both se.Expr and se.Cond are nil (a malformed AST node), line 59 calls pc.resolveCond(scope, nil), which will hit the type switch in resolveCond with a nil condition and panic on cond.Span() .A malformed AST from a buggy parser causes a panic crash instead of a clean error.Add a nil guard:
| return nil, err | ||
| } | ||
|
|
||
| resultType, ok := aggregateResultType(name, arg.ResolvedType()) |
There was a problem hiding this comment.
If the function argument is NULL (e.g. SUM(NULL)), arg is a *ResolvedNullLiteral, and arg.ResolvedType() returns the dummy placeholder ast.TypeInt.
So aggregateResultType("SUM", ast.TypeInt) returns (TypeBigInt, true) — meaning SUM(NULL) is typed as BIGINT. This is technically harmless at runtime (the result is NULL regardless), but the reported output column type is wrong. The executor or client will see BIGINT as the column type when it should arguably be indeterminate or NULL.
For AVG(NULL), the placeholder TypeInt flows through aggregateResultType("AVG", TypeInt) → returns TypeDouble. So AVG(NULL) reports type DOUBLE. Neither is truly correct — the type should be unknown/null.
| return &ResolvedFloatLiteral{ResolvedExprBase: newExprBase(ast.TypeDouble), Value: v}, nil | ||
| } | ||
|
|
||
| func (pc *planContext) resolveBinaryExpr(scope *Scope, be *ast.BinaryExpr) (ResolvedExpr, error) { |
There was a problem hiding this comment.
The planner validates that operand types are numeric, but does not check for constant division by zero
The planner accepts this query. The division-by-zero error is only discovered at execution time when evaluating the expression per row.
Please consider whether this should be stopped at planning phase or be passed and catched at executor phase
| } else if !exprsCompatible(left, right) { | ||
| return nil, pc.errorf( | ||
| cp.Span(), CodeTypeMismatch, | ||
| "cannot compare %s and %s", exprTypeName(left), exprTypeName(right), | ||
| ) | ||
| } | ||
| return &ResolvedComparison{Left: left, Op: op, Right: right}, nil |
There was a problem hiding this comment.
In SQL, NULL = NULL always evaluates to NULL (unknown), never TRUE. Any WHERE NULL = NULL condition filters out all rows — it is almost certainly a user mistake.
Emit a diagnostic warning (not an error) when comparing with NULL using = or !=.
| // mismatches, so a query with several bad values reports all of them in one | ||
| // pass. It still returns the first error encountered, since a partially | ||
| // resolved ResolvedIn cannot be used by the caller. | ||
| func (pc *planContext) resolveIn(scope *Scope, ip *ast.InPredicate) (ResolvedCond, error) { |
There was a problem hiding this comment.
exprsCompatible(expr, NULL) returns true (NULL is compatible with anything), so NULL is silently added to the ResolvedIn.Values list.
In SQL, x IN (1, NULL, 3) never returns TRUE when x is the NULL value — it returns NULL. A NULL inside an IN list is almost always a user mistake and has no useful filtering effect (it can only change a FALSE to NULL, but NULL is still filtered out by WHERE).
3 participants
Issue Reference
Summary by CodeRabbit