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

Conversation

Projects
None yet
5 participants
@AndrewDi
Contributor

AndrewDi commented Nov 24, 2018

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

  • Unit test

This change is Reviewable

@sre-bot

This comment has been minimized.

sre-bot commented Nov 24, 2018

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 AndrewDi force-pushed the AndrewDi:fix_prepare_issue_8074 branch from cfb5ba7 to c0ae8c8 Nov 25, 2018

@zz-jason

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.

@AndrewDi

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 AndrewDi force-pushed the AndrewDi:fix_prepare_issue_8074 branch from 864fb23 to c0ae8c8 Nov 25, 2018

@zz-jason

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()?

@zz-jason

LGTM

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

addressed

@AndrewDi AndrewDi force-pushed the AndrewDi:fix_prepare_issue_8074 branch from a5ade9d to 2e9d6c7 Nov 25, 2018

@AndrewDi

This comment has been minimized.

Contributor

AndrewDi commented Nov 25, 2018

Test case address...

@XuHuaiyu XuHuaiyu changed the title from PlanBuilder: fix prepare from session value to planner, executor: fix `PREPARE FROM @var_name` Nov 26, 2018

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)

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Nov 26, 2018

Contributor

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

This comment has been minimized.

@AndrewDi

AndrewDi Nov 26, 2018

Contributor

fine, result check addressed.

} else {
p.SQLText = x.SQLText
}
return p
return p, nil

This comment has been minimized.

@eurekaka

eurekaka Nov 26, 2018

Contributor

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

This comment has been minimized.

@AndrewDi

AndrewDi Nov 26, 2018

Contributor

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

This comment has been minimized.

@eurekaka

eurekaka Nov 26, 2018

Contributor

Then I prefer removing it.

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Nov 26, 2018

Contributor

Address this comment makes an LGTM. @AndrewDi

@AndrewDi

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 AndrewDi force-pushed the AndrewDi:fix_prepare_issue_8074 branch from 01ac8d5 to 456015f Nov 27, 2018

@XuHuaiyu

Reviewed 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @eurekaka)

@XuHuaiyu

This comment has been minimized.

Contributor

XuHuaiyu commented Nov 27, 2018

/run-all-tests
PTAL @eurekaka

@eurekaka

LGTM

@eurekaka

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@eurekaka eurekaka added status/LGT2 and removed status/LGT1 labels Nov 27, 2018

@zz-jason zz-jason merged commit 0ac0e15 into pingcap:master Nov 27, 2018

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
code-review/reviewable 2 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@zz-jason

This comment has been minimized.

Member

zz-jason commented Nov 27, 2018

Hi, @AndrewDi , could you cherry pick this commit to the release-2.1 and release-2.0 branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment