Skip to content

planner: fix plus prefix for select constant#9707

Merged
qw4990 merged 6 commits intopingcap:masterfrom
wjhuang2016:fix_plus_prefix
Mar 21, 2019
Merged

planner: fix plus prefix for select constant#9707
qw4990 merged 6 commits intopingcap:masterfrom
wjhuang2016:fix_plus_prefix

Conversation

@wjhuang2016
Copy link
Copy Markdown
Member

What problem does this PR solve?

For #9683

What is changed and how it works?

Return TRUE or FALSE for select true; or select false
Trim the plus prefix for most of types except bit literal or hex literal,
besides, trim the ( or ) and it not long needs innerExpr.Text()

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 13, 2019

Codecov Report

Merging #9707 into master will decrease coverage by 0.01%.
The diff coverage is 58.3333%.

@@               Coverage Diff                @@
##             master      #9707        +/-   ##
================================================
- Coverage   67.1882%   67.1782%   -0.0101%     
================================================
  Files           381        381                
  Lines         79947      79956         +9     
================================================
- Hits          53715      53713         -2     
- Misses        21448      21455         +7     
- Partials       4784       4788         +4

@eurekaka
Copy link
Copy Markdown
Contributor

@erjiaqing PTAL

Comment thread planner/core/logical_plan_builder.go Outdated
// See #9683
// TRUE or FALSE can be a int64
if mysql.HasIsBooleanFlag(valueExpr.Type.Flag) {
i, _ := valueExpr.GetValue().(int64)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if i := valueExpr.GetValue().(int64); i == 0{
return xxx
}else{
return xxx
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We need to check the BooleanFlag, would you mean
if mysql.HasIsBooleanFlag(valueExpr.Type.Flag) {
if i := valueExpr.GetValue().(int64); i == 0{
return xxx
}else{
return xxx
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

linting
https://revive.run/r#indent-error-flow if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
planner/core/logical_plan_builder.go:622:11

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, my fault, we should not use else{

Comment thread planner/core/logical_plan_builder.go Outdated
Comment thread planner/core/logical_plan_builder.go
@XuHuaiyu XuHuaiyu added type/compatibility contribution This PR is from a community contributor. sig/planner SIG: Planner labels Mar 14, 2019
@XuHuaiyu XuHuaiyu requested a review from qw4990 March 14, 2019 02:51
Comment thread cmd/explaintest/t/select.test Outdated
Copy link
Copy Markdown
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@lzmhhh123 lzmhhh123 added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 14, 2019
XuHuaiyu
XuHuaiyu previously approved these changes Mar 14, 2019
Copy link
Copy Markdown
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@XuHuaiyu
Copy link
Copy Markdown
Contributor

/run-all-tests

@wjhuang2016
Copy link
Copy Markdown
Member Author

It seems that the test case in common-test need to be update.

2019/03/14 14:25:06.272 main.go:718: [fatal] run test [operator] err: sql:select --1, ++1, -+1, +1, -+!1, !-+-1;: failed to run query

"select --1, ++1, -+1, +1, -+!1, !-+-1;"

around line 13,

we need(99):

select --1, ++1, -+1, +1, -+!1, !-+-1;

--1 ++1 -+1 +1 -+!1 !-+-1

1 1 -1 18446744073709551614 0

but got(99):

select --1, ++1, -+1, +1, -+!1, !-+-1;

--1 1 -+1 +1 -+!1 !-+-1

1 1 -1 18446744073709551614 0 1

in test case, select ++1; return column name ++1

@XuHuaiyu
Copy link
Copy Markdown
Contributor

/run-common-test tidb-test=pr/757
/run-integration-common-test tidb-test=pr/757

@wjhuang2016
Copy link
Copy Markdown
Member Author

@XuHuaiyu Please run the test again.

@XuHuaiyu
Copy link
Copy Markdown
Contributor

Check

tidb> select +
    ->
    ->   1;
+-------+
|

  1 |
+-------+
|     1 |
+-------+
1 row in set (0.00 sec)

@XuHuaiyu
Copy link
Copy Markdown
Contributor

/run-all-tests

@XuHuaiyu
Copy link
Copy Markdown
Contributor

/run-common-test tidb-test=pr/757
/run-integration-common-test tidb-test=pr/757

Comment thread planner/core/logical_plan_builder.go Outdated
fieldName := field.Text()
fieldName = strings.TrimLeft(fieldName, " +(")
fieldName = strings.TrimRight(fieldName, " )")
fieldName = strings.TrimLeft(fieldName, "\n +(")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MySQL has three white space character, [ \n\t] (in file mysql-server:storage/innobase/pars/pars0lex.l)

so, do not forget \t, this should be the last whitespace you need concern.

2019-03-17 21-18-10屏幕截图
2019-03-17 21-18-26屏幕截图

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had considered \t, but I found that I can't type \t by Mysql-client. And you're right, we need to deal with \t.

@erjiaqing erjiaqing self-requested a review March 20, 2019 01:18
@qw4990 qw4990 added the status/LGT2 Indicates that a PR has LGTM 2. label Mar 21, 2019
@qw4990 qw4990 removed the status/LGT1 Indicates that a PR has LGTM 1. label Mar 21, 2019
@qw4990 qw4990 merged commit e829920 into pingcap:master Mar 21, 2019
@wjhuang2016 wjhuang2016 deleted the fix_plus_prefix branch November 17, 2022 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution This PR is from a community contributor. sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2. type/compatibility

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants