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

*: Remove leading comment when check IsQuery #210

Merged
merged 3 commits into from
Sep 21, 2015
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions parser/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -335,19 +335,21 @@ func (s *testParserSuite) TestParser0(c *C) {

// For select with where clause
{"SELECT * FROM t WHERE 1 = 1", true},

// For comment in query
{"/*comment*/ /*comment*/ select c /* this is a comment */ from t;", true},
Copy link
Member

Choose a reason for hiding this comment

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

It is better to add different types of comments.
like
/* xxx */ select * from t
select * /* xxx */ from t;
select * from t /* xxx */

}

for _, t := range table {
fmt.Printf("%s\n", t.src)
l := NewLexer(t.src)
ok := yyParse(l) == 0
c.Assert(ok, Equals, t.ok, Commentf("source %v", t.src))

switch ok {
case true:
c.Assert(len(l.errs), Equals, 0)
c.Assert(len(l.errs), Equals, 0, Commentf("src: %s", t.src))
Copy link
Member

Choose a reason for hiding this comment

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

Can use HasLen instead of old code.

case false:
c.Assert(len(l.errs), Not(Equals), 0)
c.Assert(len(l.errs), Not(Equals), 0, Commentf("src: %s", t.src))
Copy link
Member

Choose a reason for hiding this comment

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

Not(HasLen)?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the next pr.

}
}

Expand Down Expand Up @@ -391,4 +393,12 @@ func (s *testParserSuite) TestParser0(c *C) {
cv, ok := ss.Fields[0].Expr.(*expressions.FunctionCast)
c.Assert(ok, IsTrue)
c.Assert(cv.FunctionType, Equals, expressions.ConvertFunction)

// For query start with comment
src = "/* some comments */ /*comment*/ SELECT /*comment*/ CONVERT('111', /*comment*/ SIGNED) /*comment*/;"
l = NewLexer(src)
c.Assert(yyParse(l), Equals, 0)
st = l.Stmts()[0]
ss, ok = st.(*stmts.SelectStmt)
c.Assert(ok, IsTrue)
}
17 changes: 16 additions & 1 deletion tidb.go
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,24 @@ func NewStore(uri string) (kv.Storage, error) {

var queryStmtTable = []string{"explain", "select", "show", "execute", "describe", "desc"}

func trimSQL(sql string) string {
Copy link
Member

Choose a reason for hiding this comment

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

s/trim/stripComments ?

Copy link
Member

Choose a reason for hiding this comment

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

And comments may start with --

Copy link
Member Author

Choose a reason for hiding this comment

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

If comments start with -- or # the entire line is comment. Only /**/ can mix with sql statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

trim comment and space

Copy link
Member

Choose a reason for hiding this comment

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

We need to strip comments start with --
see #199

Copy link
Member

Choose a reason for hiding this comment

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

what I mean is to check the trimming result, not only IsQuery true or false.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ngaut We can handle the sql in #199 now. For sql text start with --, we just need to drop the entire sql text and do nothing else.

➜ interpreter git:(shenli/comment-in-query) ✗ cat xx
/* mysql-connector-java-5.1.35 ( Revision: 5fb9c5849535c13917c2cf9baaece6ef9693ef27 ) */SHOW VARIABLES WHERE Variable_name ='language' OR Variable_name = 'net_write_timeout' OR Variable_name = 'interactive_timeout' OR Variable_name = 'wait_timeout' OR Variable_name = 'character_set_client' OR Variable_name = 'character_set_connection' OR Variable_name = 'character_set' OR Variable_name = 'character_set_server' OR Variable_name = 'tx_isolation' OR Variable_name = 'transaction_isolation' OR Variable_name = 'character_set_results' OR Variable_name = 'timezone' OR Variable_name = 'time_zone' OR Variable_name = 'system_time_zone' OR Variable_name = 'lower_case_table_names' OR Variable_name = 'max_allowed_packet' OR Variable_name = 'net_buffer_length' OR Variable_name = 'sql_mode' OR Variable_name = 'query_cache_type' OR Variable_name = 'query_cache_size' OR Variable_name = 'license' OR Variable_name = 'init_connect';
➜ interpreter git:(shenli/comment-in-query) ✗ cat xx | ./interpreter
Welcome to the TiDB.
Version:
Git Commit Hash: None
UTC Build Time: None

+--------------------------+------------------------+
| Variable_name | Value |
+--------------------------+------------------------+
| character_set_results | latin1 |
| init_connect | |
| character_set_client | latin1 |
| tx_isolation | REPEATABLE-READ |
| interactive_timeout | 28800 |
| max_allowed_packet | 4194304 |
| lower_case_table_names | 2 |
| character_set_connection | latin1 |
| time_zone | SYSTEM |
| sql_mode | NO_ENGINE_SUBSTITUTION |
| query_cache_type | OFF |
| net_write_timeout | 60 |
| system_time_zone | CST |
| query_cache_size | 1048576 |
| net_buffer_length | 16384 |
| license | GPL |
| wait_timeout | 28800 |
| character_set_server | latin1 |
+--------------------------+------------------------+

Bye

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is my fault.
SQL like select * /* comment */ from t; is supported now.
Seems we only not support start with /* SQL, like /* comment */ select * from t;

Copy link
Member

Choose a reason for hiding this comment

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

Looks like mp didn't handle comments start -- correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

I test tidb-server and it is ok now.

Database changed
mysql> select /comment/ 1;
+------+
| 1 |
+------+
| 1 |
+------+
1 row in set (0.00 sec)

mysql> /comment/ select 1;
+------+
| 1 |
+------+
| 1 |
+------+
1 row in set (0.00 sec)

mysql>

// Trim space.
sql = strings.TrimSpace(sql)
// Trim leading /*comment*/
// There may be multiple comments
for strings.HasPrefix(sql, "/*") {
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 strings.HasPrefix(sql, "/*") is false, then comment will not be trimed?

Copy link
Member

Choose a reason for hiding this comment

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

Have same doubt too.

Copy link
Member Author

Choose a reason for hiding this comment

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

This function is used to fix bug when sql text start with "/comment/" IsQuery always returns false.
We do not need to care about other comment format.

i := strings.Index(sql, "*/")
if i != -1 && i < len(sql)+1 {
sql = sql[i+2:]
}
Copy link
Member

Choose a reason for hiding this comment

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

If SQL like /* select * from t;, here seems will be an endless loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

PTAL

sql = strings.TrimSpace(sql)
}
return sql
}

// IsQuery checks if a sql statement is a query statement.
func IsQuery(sql string) bool {
sqlText := strings.ToLower(strings.TrimSpace(sql))
sqlText := strings.ToLower(trimSQL(sql))
for _, key := range queryStmtTable {
if strings.HasPrefix(sqlText, key) {
return true
Expand Down
14 changes: 14 additions & 0 deletions tidb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,20 @@ func (s *testMainSuite) TestCheckArgs(c *C) {
"abc", []byte("abc"), time.Now(), time.Hour, time.Local)
}

func (s *testMainSuite) TestIsQuery(c *C) {
tbl := []struct {
sql string
ok bool
}{
{"/*comment*/ select 1;", true},
{"/*comment*/ /*comment*/ select 1;", true},
{"select /*comment*/ 1 /*comment*/;", true},
}
for _, t := range tbl {
c.Assert(IsQuery(t.sql), Equals, t.ok, Commentf(t.sql))
}
}

func sessionExec(c *C, se Session, sql string) ([]rset.Recordset, error) {
se.Execute("BEGIN;")
r, err := se.Execute(sql)
Expand Down