Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

planner, executor: fix PREPARE FROM @var_name #8437

Merged
merged 4 commits into from
Nov 27, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
PlanBuilder: fix prepare from session value
  • Loading branch information
AndrewDi committed Nov 27, 2018
commit 0b93777c052a36be683ef1cf16247e4278d41156
9 changes: 9 additions & 0 deletions executor/prepared_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,15 @@ func (s *testSuite) TestPrepared(c *C) {
c.Assert(err, IsNil)
c.Assert(len(fields), Equals, 0)

// issue 8074
tk.MustExec("drop table if exists prepare1;")
tk.MustExec("create table prepare1 (a decimal(1))")
_, err = tk.Exec("prepare stmt FROM @sql1")
c.Assert(err.Error(), Equals, "line 1 column 4 near \"\" (total length 4)")
tk.MustExec("SET @sql = 'update prepare1 set a=5 where a=?';")
_, err = tk.Exec("prepare stmt FROM @sql")
c.Assert(err, IsNil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we need to execute stmt to see whether the result is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fine, result check addressed.


// Coverage.
exec := &executor.ExecuteExec{}
exec.Next(ctx, nil)
Expand Down
13 changes: 8 additions & 5 deletions planner/core/planbuilder.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ func (b *PlanBuilder) Build(node ast.Node) (Plan, error) {
case *ast.LoadStatsStmt:
return b.buildLoadStats(x), nil
case *ast.PrepareStmt:
return b.buildPrepare(x), nil
return b.buildPrepare(x)
case *ast.SelectStmt:
return b.buildSelect(x)
case *ast.UnionStmt:
Expand Down Expand Up @@ -396,17 +396,20 @@ func (b *PlanBuilder) buildSelectLock(src LogicalPlan, lock ast.SelectLockType)
return selectLock
}

func (b *PlanBuilder) buildPrepare(x *ast.PrepareStmt) Plan {
func (b *PlanBuilder) buildPrepare(x *ast.PrepareStmt) (Plan, error) {
p := &Prepare{
Name: x.Name,
}
if x.SQLVar != nil {
// TODO: Prepared statement from variable expression do not work as expected.
// p.SQLText, _ = x.SQLVar.GetValue().(string)
if v, ok := b.ctx.GetSessionVars().Users[x.SQLVar.Name]; ok {
p.SQLText = v
} else {
p.SQLText = "NULL"
}
} else {
p.SQLText = x.SQLText
}
return p
return p, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Why adding one more return value error which is always nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we may address some errors in this function first thought; but found no such errors should be address here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I prefer removing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Address this comment makes an LGTM. @AndrewDi

}

func (b *PlanBuilder) buildCheckIndex(dbName model.CIStr, as *ast.AdminStmt) (Plan, error) {
Expand Down