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

parser: implement Restore for VariableExpr #87

Merged
merged 6 commits into from
Dec 26, 2018

Conversation

exialin
Copy link
Contributor

@exialin exialin commented Dec 19, 2018

Implement Restore and unit test for VariableExpr.

By the way, the literal string of operator EQ should be = instead of ==, which is fixed.

Issue: pingcap/tidb#8532

ast/expressions.go Outdated Show resolved Hide resolved
opcode/opcode.go Outdated Show resolved Hide resolved
@exialin exialin changed the title ast, opcode: implement Restore for VariableExpr and fix literal string of operator EQ parser, opcode: implement Restore for VariableExpr and fix literal string of operator EQ Dec 20, 2018
@exialin
Copy link
Contributor Author

exialin commented Dec 20, 2018

@kennytm Hi, I want to discuss two problems with you. After changing to WriteName, some tests will fail.

  1. The generated ASTs of select @`a` and select @a are different.

The AST of select @`a` :

*ast.SelectStmt	&{{{{select @`a`}}} {[]} 0xc00000f590 false <nil> <nil> 0xc00000f5c0 <nil> <nil> [] <nil> <nil> none [] false false}
  *ast.FieldList	&{{} [0xc000056660]}
    *ast.SelectField	&{{`a`} 8 <nil> 0xc0000e6960  false}
      *ast.VariableExpr	&{{{} {0 0 0 0   []} 64} a false false false <nil>}

The AST of select @a:

*ast.SelectStmt	&{{{{select @a}}} {[]} 0xc00000f590 false <nil> <nil> 0xc00000f5c0 <nil> <nil> [] <nil> <nil> none [] false false}
  *ast.FieldList	&{{} [0xc000056660]}
    *ast.SelectField	&{{@a} 7 <nil> 0xc00009caa0  false}
      *ast.VariableExpr	&{{{} {0 0 0 0   []} 64} a false false false <nil>}

So they are not deeply equal to each other. In the unit test function, I have to use @`a` but not @a as the source SQL text. But the restored SQL text will work.

  1. TiDB doesn't support queries like this while MySQL supports:
tidb> select @@global.`sql_mode`;
ERROR 1193 (HY000): Unknown system variable ''

The AST of select @@global.sql_mode:

*ast.SelectStmt	&{{{{select @@global.sql_mode}}} {[]} 0xc000083500 false <nil> <nil> 0xc000083530 <nil> <nil> [] <nil> <nil> none [] false false}
  *ast.FieldList	&{{} [0xc00007e600]}
    *ast.SelectField	&{{@@global.sql_mode} 7 <nil> 0xc000092aa0  false}
      *ast.VariableExpr	&{{{} {0 0 0 0   []} 64} sql_mode true true true <nil>}

The AST of select @@global.`sql_mode` :

ast.SelectStmt	&{{{{select @@global.`sql_mode`}}} {[]} 0xc000083500 false <nil> <nil> 0xc000083530 <nil> <nil> [] <nil> <nil> none [] false false}
  *ast.FieldList	&{{} [0xc00007e600]}
    *ast.SelectField	&{{} 7 <nil> 0xc000092a00 sql_mode false}
      *ast.VariableExpr	&{{{} {0 0 0 0   []} 64}  true true true <nil>}

As you can see the parser currently doesn't handle such case, so Variable.Name=''. Maybe this syntax should be added, and the test can pass only when it is supported.

@kennytm
Copy link
Contributor

kennytm commented Dec 20, 2018

@exialin Thanks for the throughout tests. I think both are bugs in the parser. I've filed #94 and #95.

So we have two paths forward:

  1. Use WriteKeyWord instead of WriteName and ignores variables with quoted names, or
  2. Wait until those two issues are fixed

I think we should do (1) (use WriteKeyWord) to avoid blocking this PR on these edge cases. WDYT @tiancaiamao @leoppro

@zier-one
Copy link
Contributor

i think we should wait the two issue fixed.

@zier-one zier-one self-assigned this Dec 21, 2018
@exialin exialin changed the title parser, opcode: implement Restore for VariableExpr and fix literal string of operator EQ parser: implement Restore for VariableExpr Dec 22, 2018
@zier-one
Copy link
Contributor

#112 id merged, please resolve conflicts and run test again @exialin

@exialin
Copy link
Contributor Author

exialin commented Dec 25, 2018

@leoppro PTAL.
Here's one more problem. The name of system variables are converted to lower cases:

parser/parser.y

Lines 5649 to 5651 in f4f4614

doubleAtIdentifier
{
v := strings.ToLower($1)

But user variables are not. So in the test cases, the names have to be case sensitive for user variables, while insensitive for system variables. Should they be consistent?

@zier-one
Copy link
Contributor

@leoppro PTAL.
Here's one more problem. The name of system variables are converted to lower cases:
parser/parser.y

Lines 5649 to 5651 in f4f4614

doubleAtIdentifier
{
v := strings.ToLower($1)
But user variables are not. So in the test cases, the names have to be case sensitive for user variables, while insensitive for system variables. Should they be consistent?

it doesn't matter, ignore it will be fine

@zier-one
Copy link
Contributor

LGTM, PTAL @kennytm

ast/expressions_test.go Outdated Show resolved Hide resolved
ast/expressions_test.go Outdated Show resolved Hide resolved
@zier-one zier-one added the status/LGT1 LGT1 label Dec 25, 2018
@zier-one
Copy link
Contributor

@kennytm PTAL

Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

ast/expressions_test.go Show resolved Hide resolved
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm kennytm added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Dec 26, 2018
@kennytm kennytm merged commit 60752e9 into pingcap:master Dec 26, 2018
@exialin exialin deleted the restore-variableexpr branch December 26, 2018 15:19
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
* ast, opcode: implement Restore for VariableExpr and fix literal string of operator `EQ`

* address comment but still has some problem

* fix test

* add test case
lyonzhi pushed a commit to lyonzhi/parser that referenced this pull request Apr 25, 2024
* ast, opcode: implement Restore for VariableExpr and fix literal string of operator `EQ`

* address comment but still has some problem

* fix test

* add test case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants