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

expression: Fix the wrong behavior of const.String() #17495

Merged
merged 8 commits into from
Jun 4, 2020

Conversation

Reminiscent
Copy link
Contributor

@Reminiscent Reminiscent commented May 29, 2020

What problem does this PR solve?

Issue Number: close #17287

Problem Summary:

  1. After we execute the statement, it will decode the plan to string. And the behavior of const.String() is wrong.
  2. When the const.DeferredExpr != nil, we need to use the passed function parameter chunk.row for Eval, instead of using an empty chunk.Row.

What is changed and how it works?

Use the const.DeferredExpr.String() rather than to eval the const.
Use the function parameter chunk.row for Eval instead of an empty chunk.Row().

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Release note

  • fix panic when we use prepare statement

@Reminiscent Reminiscent requested a review from a team as a code owner May 29, 2020 01:57
@ghost ghost requested review from SunRunAway and removed request for a team May 29, 2020 01:57
@pingcap pingcap deleted a comment from sre-bot May 29, 2020

tk.MustExec("use test;")
tk.MustExec("drop table if exists t;")
tk.MustExec("set @@tidb_enable_vectorized_expression = false;")
Copy link
Contributor Author

@Reminiscent Reminiscent May 29, 2020

Choose a reason for hiding this comment

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

Set this to test the behavior of const.Eval(). If we don't change the implementation of the const.Eval(). The following SQL statement will panic.

@Reminiscent Reminiscent self-assigned this May 29, 2020
@Reminiscent
Copy link
Contributor Author

@SunRunAway PTAL

@Reminiscent Reminiscent requested review from qw4990 and XuHuaiyu June 1, 2020 02:09
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Jun 1, 2020

Codecov Report

Merging #17495 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #17495   +/-   ##
===========================================
  Coverage   79.4585%   79.4585%           
===========================================
  Files           524        524           
  Lines        141212     141212           
===========================================
  Hits         112205     112205           
  Misses        19948      19948           
  Partials       9059       9059           

Copy link
Contributor

@danmay319 danmay319 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Jun 4, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jun 4, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 4, 2020

@Reminiscent merge failed.

@Reminiscent
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 4, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 4, 2020

@Reminiscent merge failed.

@Reminiscent
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 4, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 4, 2020

@Reminiscent merge failed.

@Reminiscent
Copy link
Contributor Author

/merge

1 similar comment
@Reminiscent
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 4, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 4, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 4, 2020

@Reminiscent merge failed.

@Reminiscent
Copy link
Contributor Author

/run-integration-br-test

@SunRunAway SunRunAway merged commit a501f25 into pingcap:master Jun 4, 2020
@SunRunAway
Copy link
Contributor

Release note
fix the wrong behavior of const.String()

Please fix the release note as user perspective.

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Jun 4, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jun 4, 2020

cherry pick to release-4.0 in PR #17673

SunRunAway pushed a commit that referenced this pull request Jun 5, 2020
@Reminiscent Reminiscent deleted the fix17287 branch August 5, 2021 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic when use prepare statement
6 participants