Fix: Vitess show character set where #5710

Ronihe · 2020-01-14T07:26:59Z

right now the show character set are hardcoded to be filtered with where statement.
since we don't really want to expand the results or forward the query to an underlying mysql because vitess only supports these 2 charsets.
Ideally there should be a where modular to do the function.

go/vt/vtgate/executor.go

Signed-off-by: Roni He <[email protected]>

deepthi · 2020-01-17T21:41:28Z

executor_test is failing:

##[error]    executor_test.go:833: show charset like '%foo':
        &{Fields:[name:"Charset" type:VARCHAR charset:33 flags:1  name:"Description" type:VARCHAR charset:33 flags:1  name:"Default collation" type:VARCHAR charset:33 flags:1  name:"Maxlen" type:INT32 ] RowsAffected:0 InsertID:0 Rows:[] Extras:<nil>}, want
        &{Fields:[name:"Charset" type:VARCHAR charset:33 flags:1  name:"Description" type:VARCHAR charset:33 flags:1  name:"Default collation" type:VARCHAR charset:33 flags:1  name:"Maxlen" type:INT32 ] RowsAffected:2 InsertID:0 Rows:[[VARCHAR("utf8") VARCHAR("UTF-8 Unicode") VARCHAR("utf8_general_ci") INT32(3)] [VARCHAR("utf8mb4") VARCHAR("UTF-8 Unicode") VARCHAR("utf8mb4_general_ci") INT32(4)]] Extras:<nil>}
##[error]    executor_test.go:833: show character set like '%foo':
        &{Fields:[name:"Charset" type:VARCHAR charset:33 flags:1  name:"Description" type:VARCHAR charset:33 flags:1  name:"Default collation" type:VARCHAR charset:33 flags:1  name:"Maxlen" type:INT32 ] RowsAffected:0 InsertID:0 Rows:[] Extras:<nil>}, want
        &{Fields:[name:"Charset" type:VARCHAR charset:33 flags:1  name:"Description" type:VARCHAR charset:33 flags:1  name:"Default collation" type:VARCHAR charset:33 flags:1  name:"Maxlen" type:INT32 ] RowsAffected:2 InsertID:0 Rows:[[VARCHAR("utf8") VARCHAR("UTF-8 Unicode") VARCHAR("utf8_general_ci") INT32(3)] [VARCHAR("utf8mb4") VARCHAR("UTF-8 Unicode") VARCHAR("utf8mb4_general_ci") INT32(4)]] Extras:<nil>}

I suggest changing the expected output for these cases and adding new cases to test with like 'utf8%' (if we want to support that).

Signed-off-by: Roni He <[email protected]>

deepthi

LGTM

deepthi · 2020-01-30T17:31:03Z

@Ronihe can you rebase or merge and resolve conflicts?

go/vt/sqlparser/parse_test.go

systay · 2020-01-31T07:40:18Z

go/vt/vtgate/executor.go

-		row1 = append(row1, sqltypes.NewInt32(4))
-		rows = append(rows, row0, row1)
+
+		charsets := []string{"utf8", "utf8mb4"}


Here you are using "magic" strings that are repeated in line 1580 of this file. I think it'd be better to use consts of strings, or even an enum.

systay · 2020-01-31T07:43:25Z

go/vt/vtgate/executor.go

+			rightString := string(sqlVal.Val)
+
+			switch cmpExp.Operator {
+			case "=":


This file is full of magic strings, even before your changes. But we can improve the situation. Here I would suggest that you use the same string that the parser uses, which is sqlparser.EqualStr. That way it's easy to find usages of this symbol.

@systay do you mean create a file in sqlparser just for all the magic string/ words?

Just replace = with sqlparser.EqualStr

systay · 2020-01-31T07:43:51Z

go/vt/vtgate/executor.go

+						filteredColName = colName
+					}
+				}
+			case "like":


Same as above, but with sqlparser.LikeStr

systay · 2020-01-31T07:47:34Z

go/vt/vtgate/executor_test.go

+	}
+	for _, query := range []string{"show charset like 'utf8'", "show character set like 'utf8'", "show charset where charset = 'utf8'", "show character set where charset = 'utf8'"} {
+		qr, err := executor.Execute(context.Background(), "TestExecute", session, query, nil)
+		if err != nil {


There's a few of them in the new code, and it's probably because most of this file is doing it.
For new code, I think it's way better to write require.NoError(t, err).

@systay

I'll let @systay approve once his comments have been addressed

Signed-off-by: roni <[email protected]>

systay reviewed Jan 16, 2020

View reviewed changes

go/vt/vtgate/executor.go Outdated Show resolved Hide resolved

systay reviewed Jan 16, 2020

View reviewed changes

go/vt/vtgate/executor.go Outdated Show resolved Hide resolved

systay reviewed Jan 16, 2020

View reviewed changes

go/vt/vtgate/executor.go Outdated Show resolved Hide resolved

Ronihe added 5 commits January 17, 2020 11:45

seperate show charset vs show charset in parser

8ea28f2

Signed-off-by: Roni He <[email protected]>

changed the executor accordingly

75c8abe

Signed-off-by: Roni He <[email protected]>

use where_expression_opt in parser, change ast and exector accordingly

94b7ba1

Signed-off-by: Roni He <[email protected]>

change the parser for show charset to filter with where or like

a54be7e

Signed-off-by: Roni He <[email protected]>

exec func done, need to add more test

e34ade7

Signed-off-by: Roni He <[email protected]>

Ronihe force-pushed the roni-fix-show-bug branch from 7e1dab6 to e34ade7 Compare January 17, 2020 20:43

Ronihe marked this pull request as ready for review January 17, 2020 20:47

Ronihe requested a review from sougou as a code owner January 17, 2020 20:47

Ronihe requested review from systay and deepthi January 17, 2020 20:47

add err in func generateCharsetRows, added tests

30ae767

Signed-off-by: Roni He <[email protected]>

Ronihe added 2 commits January 17, 2020 13:49

fix err bug in the func generateCharsetRow

3c68962

Signed-off-by: Roni He <[email protected]>

correct error type to vterror

047af95

Signed-off-by: Roni He <[email protected]>

deepthi previously approved these changes Jan 30, 2020

View reviewed changes

systay reviewed Jan 31, 2020

View reviewed changes

go/vt/sqlparser/parse_test.go Show resolved Hide resolved

systay reviewed Jan 31, 2020

View reviewed changes

go/vt/vtgate/executor.go Outdated

filteredColName = colName

}

}

case "like":

Copy link

Collaborator

systay Jan 31, 2020

Choose a reason for hiding this comment

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

Same as above, but with sqlparser.LikeStr

systay reviewed Jan 31, 2020

View reviewed changes

Ronihe added 4 commits February 3, 2020 16:22

changed the check err stmt to require.NoError0

bd211c0

Signed-off-by: roni <[email protected]>

git merge master

43f6f16

Signed-off-by: roni <[email protected]>

updated

b10dd0f

Signed-off-by: roni <[email protected]>

added test for show charset

adf89ea

Signed-off-by: roni <[email protected]>

systay approved these changes Feb 6, 2020

View reviewed changes

systay merged commit a2dd7a0 into vitessio:master Feb 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Vitess show character set where #5710

Fix: Vitess show character set where #5710

Ronihe commented Jan 14, 2020 •

edited by deepthi

Loading

deepthi commented Jan 17, 2020 •

edited

Loading

deepthi left a comment

deepthi commented Jan 30, 2020

systay Jan 31, 2020

systay Jan 31, 2020

Ronihe Feb 4, 2020

saifalharthi Feb 4, 2020

systay Jan 31, 2020

systay Jan 31, 2020 •

edited

Loading

Fix: Vitess show character set where #5710

Fix: Vitess show character set where #5710

Conversation

Ronihe commented Jan 14, 2020 • edited by deepthi Loading

deepthi commented Jan 17, 2020 • edited Loading

deepthi left a comment

Choose a reason for hiding this comment

deepthi commented Jan 30, 2020

systay Jan 31, 2020

Choose a reason for hiding this comment

systay Jan 31, 2020

Choose a reason for hiding this comment

Ronihe Feb 4, 2020

Choose a reason for hiding this comment

saifalharthi Feb 4, 2020

Choose a reason for hiding this comment

systay Jan 31, 2020

Choose a reason for hiding this comment

systay Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

Ronihe commented Jan 14, 2020 •

edited by deepthi

Loading

deepthi commented Jan 17, 2020 •

edited

Loading

systay Jan 31, 2020 •

edited

Loading