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.

Sign up for GitHub

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

Jump to bottom

expression: make "SHOW TABLE STATUS" be case insensitive #7991

Closed
wants to merge 21 commits into from
Closed

expression: make "SHOW TABLE STATUS" be case insensitive #7991

wants to merge 21 commits into from

Conversation

iamzhoug37
Copy link
Contributor

@iamzhoug37 iamzhoug37 commented Oct 22, 2018 •
edited by zz-jason
Loading

What problem does this PR solve?

fix #7518

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to update the tidb-ansible repository
  • Need to be included in the release note

@winoros winoros added the contribution This PR is from a community contributor. label Oct 22, 2018
@winoros
Copy link
Member

winoros commented Oct 22, 2018

In my opinion, we'd better do this out of the LIKE builtin function?
PTAL @lysu @jackysp

@iamzhoug37 iamzhoug37 mentioned this pull request Oct 22, 2018
Closed
@iamzhoug37
Copy link
Contributor Author

OK，let me rewrite this change

@sre-bot
Copy link
Contributor

sre-bot commented Oct 22, 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.

@zz-jason
Copy link
Member

@iamzhoug37 please fill the PR description in detail and check the Check List carefully.

@CLAassistant
Copy link

CLAassistant commented Oct 23, 2018 •
edited
Loading

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ iamzhoug37
✅ zz-jason
❌ liyuzhou

liyuzhou seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@zz-jason zz-jason changed the title fix show table status case insensitivity #7518 expression: make "SHOW TABLE STATUS" be case insensitive Oct 23, 2018
zz-jason
zz-jason reviewed Oct 23, 2018
View reviewed changes
expression/builtin_like.go Show resolved Hide resolved
expression/builtin_like.go Outdated
@@ -80,6 +83,13 @@ func (b *builtinLikeSig) evalInt(row chunk.Row) (int64, bool, error) {
return 0, isNull, errors.Trace(err)
}
escape := byte(val)

lowerCaseTableName,_ := strconv.Atoi(variable.SysVars["lower_case_table_names"].Value)
if lowerCaseTableName != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

how about directly: variable.SysVars["lower_case_table_names"].Value != "0"

@zz-jason
Copy link
Member

@iamzhoug37

  1. Please add some test to cover the bug this PR fixes.
  2. Please fix CI.

@iamzhoug37
Copy link
Contributor Author

Ok，I will fix this. This is my first open source code develop experience, don't know some rules. Please forgive me..

@iamzhoug37
Copy link
Contributor Author

@winoros If don't implement this logic in LIKE builtin function, we need to do this in ShowExec fetchShowTableStatus and expressionRewriter Leave function. Should we implement this in two place or do you have other better suggestion?

zz-jason
zz-jason reviewed Oct 25, 2018
View reviewed changes
expression/evaluator_test.go Outdated
@@ -14,6 +14,7 @@
package expression

import (
"github.com/pingcap/tidb/sessionctx/variable"
Copy link
Member

Choose a reason for hiding this comment

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

please reorg the packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok，I'm doing "make dev" in my local

zz-jason
zz-jason reviewed Oct 25, 2018
View reviewed changes
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 the status/LGT1 Indicates that a PR has LGTM 1. label Oct 25, 2018
@iamzhoug37 iamzhoug37 closed this Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zz-jason zz-jason zz-jason left review comments

Assignees
No one assigned
Labels
component/expression contribution This PR is from a community contributor. status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SHOW TABLE STATUS case sensitivity
5 participants
@iamzhoug37 @winoros @sre-bot @zz-jason @CLAassistant