planner, executor: fix PREPARE FROM @var_name
#8437
Conversation
|
Hi contributor, thanks for your PR. This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically. |
AndrewDi
force-pushed
the
fix_prepare_issue_8074
branch
from
cfb5ba7 to
c0ae8c8
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AndrewDi)
executor/prepared_test.go, line 221 at r1 (raw file):
tk.MustExec("create table prepare1 (a decimal(1))") _, err = tk.Exec("prepare stmt FROM @sql1") c.Assert(err, NotNil)
please check the error message as well.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @zz-jason)
executor/prepared_test.go, line 221 at r1 (raw file):
Previously, zz-jason (Zhang Jian) wrote…
please check the error message as well.
Because @SQL is undefined，so “ERROR 1105 (HY000): line 1 column 4 near "" (total length 4)” message return。
AndrewDi
force-pushed
the
fix_prepare_issue_8074
branch
from
864fb23 to
c0ae8c8
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @zz-jason and @AndrewDi)
executor/prepared_test.go, line 221 at r1 (raw file):
Previously, AndrewDi (Andrew) wrote…
Because @SQL is undefined，so “ERROR 1105 (HY000): line 1 column 4 near "" (total length 4)” message return。
Uh.. I can't get your point. Except for c.Assert(err, NotNil), could you further test the error message that err contains, i.e. err.Error()?
There was a problem hiding this comment.
LGTM
Reviewed 2 of 2 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
zz-jason
added
contribution
AndrewDi
force-pushed
the
fix_prepare_issue_8074
branch
from
a5ade9d to
2e9d6c7
Compare
|
Test case address... |
XuHuaiyu
changed the title
PREPARE FROM @var_name
| 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) |
There was a problem hiding this comment.
Maybe we need to execute stmt to see whether the result is correct.
There was a problem hiding this comment.
fine, result check addressed.
planner/core/planbuilder.go
Outdated
| } else { | ||
| p.SQLText = x.SQLText | ||
| } | ||
| return p | ||
| return p, nil |
There was a problem hiding this comment.
Why adding one more return value error which is always nil?
There was a problem hiding this comment.
I think we may address some errors in this function first thought; but found no such errors should be address here.
There was a problem hiding this comment.
Then I prefer removing it.
There was a problem hiding this comment.
Address this comment makes an LGTM. @AndrewDi
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @zz-jason, @XuHuaiyu, and @eurekaka)
executor/prepared_test.go, line 224 at r3 (raw file):
Previously, AndrewDi (Andrew) wrote…
fine, result check addressed.
Done.
planner/core/planbuilder.go, line 412 at r3 (raw file):
Previously, eurekaka (Kenan Yao) wrote…
Then I prefer removing it.
I think both programming is fine。。。
AndrewDi
force-pushed
the
fix_prepare_issue_8074
branch
from
01ac8d5 to
456015f
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @eurekaka)
|
/run-all-tests |
There was a problem hiding this comment.
Reviewable status:
eurekaka
added
status/LGT2
|
Hi, @AndrewDi , could you cherry pick this commit to the release-2.1 and release-2.0 branch? |
What problem does this PR solve?
PlanBuilder: Fix prepare from session value issue #8074
What is changed and how it works?
When prepare from a user defined session name, replace session name with session value.
Improve mysql grammar compatibility.
Check List
Tests
This change is