From 1ddeee06dd23150bd04f175769ad299c9cb0105a Mon Sep 17 00:00:00 2001 From: Saif Alharthi Date: Fri, 27 Mar 2020 12:09:59 -0700 Subject: [PATCH 1/4] Fix Vtexplains StripComments Signed-off-by: Saif Alharthi Modify for better readability Signed-off-by: Saif Alharthi Make sure anything inside qoutations will not be removed Signed-off-by: Saif Alharthi Modity test for readablity Signed-off-by: Saif Alharthi --- go/vt/sqlparser/comments.go | 16 ++++++++++++++++ go/vt/sqlparser/comments_test.go | 15 +++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/go/vt/sqlparser/comments.go b/go/vt/sqlparser/comments.go index afffd326c8c..dc55aea7c26 100644 --- a/go/vt/sqlparser/comments.go +++ b/go/vt/sqlparser/comments.go @@ -178,6 +178,12 @@ func StripComments(sql string) string { if end <= 1 { break } + if start > end { + start, end = end, start + } + if containsQuotations(sql, start, end) { + break + } sql = sql[:start] + sql[end+2:] } @@ -186,6 +192,16 @@ func StripComments(sql string) string { return sql } +func containsQuotations(sql string, start, end int) bool { + indexBefore, indexAfter := start-1, end+2 + // Check Boundaries + if indexBefore < 0 || indexAfter >= len(sql) { + return false + } + // Check if the character the index before comments start and end substring is a single or double quotation + return (sql[indexBefore] == '\'' && sql[indexAfter] == '\'') || (sql[indexBefore] == '"' && sql[indexAfter] == '"') +} + // ExtractMysqlComment extracts the version and SQL from a comment-only query // such as /*!50708 sql here */ func ExtractMysqlComment(sql string) (version string, innerSQL string) { diff --git a/go/vt/sqlparser/comments_test.go b/go/vt/sqlparser/comments_test.go index bd9c26f9b8d..a9a40263dfe 100644 --- a/go/vt/sqlparser/comments_test.go +++ b/go/vt/sqlparser/comments_test.go @@ -281,6 +281,21 @@ a`, }, { input: `-- foo bar`, outSQL: "", + }, { + input: "select * from customer where name like '*//*'", + outSQL: "select * from customer where name like '*//*'", + }, { + input: "select * from customer where name like '*/%/*'", + outSQL: "select * from customer where name like '*/%/*'", + }, { + input: "select * from customer where name like '*/%/*'", + outSQL: "select * from customer where name like '*/%/*'", + }, { + input: "insert into customer values(1, '*//*')", + outSQL: "insert into customer values(1, '*//*')", + }, { + input: "insert into t5 (col) values('/**/')", + outSQL: "insert into t5 (col) values('/**/')", }} for _, testCase := range testCases { gotSQL := StripComments(testCase.input) From e907bdba981874c061254edb0d7e565c7dfcbd1b Mon Sep 17 00:00:00 2001 From: Saif Alharthi Date: Sun, 29 Mar 2020 21:15:23 -0700 Subject: [PATCH 2/4] Use Parser to handle comments if parseable Signed-off-by: Saif Alharthi --- go/vt/sqlparser/comments.go | 49 +++++++++++++++----------------- go/vt/sqlparser/comments_test.go | 7 +++-- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/go/vt/sqlparser/comments.go b/go/vt/sqlparser/comments.go index dc55aea7c26..744f1f150f0 100644 --- a/go/vt/sqlparser/comments.go +++ b/go/vt/sqlparser/comments.go @@ -169,39 +169,36 @@ func hasCommentPrefix(sql string) bool { func StripComments(sql string) string { sql = StripLeadingComments(sql) // handle -- or /* ... */ at the beginning - for { - start := strings.Index(sql, "/*") - if start == -1 { - break - } - end := strings.Index(sql, "*/") - if end <= 1 { - break - } - if start > end { - start, end = end, start - } - if containsQuotations(sql, start, end) { - break - } - sql = sql[:start] + sql[end+2:] + // Weird case that breaks the parser + if sql == "/*!*/" { + return "" } + stmt, err := Parse(sql) + // Errors would be syntax errors or empty statements at this point. + if err != nil { + for { + start := strings.Index(sql, "/*") + if start == -1 { + break + } + end := strings.Index(sql, "*/") + if end <= 1 { + break + } + if start > end { + start, end = end, start + } + sql = sql[:start] + sql[end+2:] + } + } else { + sql = String(stmt) + } sql = strings.TrimFunc(sql, unicode.IsSpace) return sql } -func containsQuotations(sql string, start, end int) bool { - indexBefore, indexAfter := start-1, end+2 - // Check Boundaries - if indexBefore < 0 || indexAfter >= len(sql) { - return false - } - // Check if the character the index before comments start and end substring is a single or double quotation - return (sql[indexBefore] == '\'' && sql[indexAfter] == '\'') || (sql[indexBefore] == '"' && sql[indexAfter] == '"') -} - // ExtractMysqlComment extracts the version and SQL from a comment-only query // such as /*!50708 sql here */ func ExtractMysqlComment(sql string) (version string, innerSQL string) { diff --git a/go/vt/sqlparser/comments_test.go b/go/vt/sqlparser/comments_test.go index a9a40263dfe..33ee965815c 100644 --- a/go/vt/sqlparser/comments_test.go +++ b/go/vt/sqlparser/comments_test.go @@ -292,10 +292,13 @@ a`, outSQL: "select * from customer where name like '*/%/*'", }, { input: "insert into customer values(1, '*//*')", - outSQL: "insert into customer values(1, '*//*')", + outSQL: "insert into customer values (1, '*//*')", }, { input: "insert into t5 (col) values('/**/')", - outSQL: "insert into t5 (col) values('/**/')", + outSQL: "insert into t5(col) values ('/**/')", + }, { + input: "select * from customer where name like 'abc*/la/*xyz'", + outSQL: "select * from customer where name like 'abc*/la/*xyz'", }} for _, testCase := range testCases { gotSQL := StripComments(testCase.input) From 9b2f2109a950d13052692587a7a3d143097ff102 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Wed, 1 Apr 2020 09:41:36 +0200 Subject: [PATCH 3/4] Try to use SplitMarginComments instead of having StripComments() separately Signed-off-by: Andres Taylor --- go/vt/sqlparser/comments.go | 41 +------- go/vt/sqlparser/comments_test.go | 131 +++++------------------- go/vt/vtexplain/vtexplain.go | 2 +- go/vt/vtexplain/vtexplain_flaky_test.go | 2 +- 4 files changed, 28 insertions(+), 148 deletions(-) diff --git a/go/vt/sqlparser/comments.go b/go/vt/sqlparser/comments.go index 744f1f150f0..e8cd4b87652 100644 --- a/go/vt/sqlparser/comments.go +++ b/go/vt/sqlparser/comments.go @@ -55,7 +55,7 @@ func leadingCommentEnd(text string) (end int) { // Found visible characters. Look for '/*' at the beginning // and '*/' somewhere after that. - if len(remainingText) < 4 || remainingText[:2] != "/*" { + if len(remainingText) < 4 || remainingText[:2] != "/*" || remainingText[2] == '!' { break } commentLength := 4 + strings.Index(remainingText[2:], "*/") @@ -93,8 +93,8 @@ func trailingCommentStart(text string) (start int) { // Find the beginning of the comment startCommentPos := strings.LastIndex(text[:reducedLen-2], "/*") - if startCommentPos < 0 { - // Badly formatted sql :/ + if startCommentPos < 0 || text[startCommentPos+2] == '!' { + // Badly formatted sql, or a special /*! comment break } @@ -164,41 +164,6 @@ func hasCommentPrefix(sql string) bool { return len(sql) > 1 && ((sql[0] == '/' && sql[1] == '*') || (sql[0] == '-' && sql[1] == '-')) } -// StripComments removes all comments from the string regardless -// of where they occur -func StripComments(sql string) string { - sql = StripLeadingComments(sql) // handle -- or /* ... */ at the beginning - - // Weird case that breaks the parser - if sql == "/*!*/" { - return "" - } - - stmt, err := Parse(sql) - // Errors would be syntax errors or empty statements at this point. - if err != nil { - for { - start := strings.Index(sql, "/*") - if start == -1 { - break - } - end := strings.Index(sql, "*/") - if end <= 1 { - break - } - if start > end { - start, end = end, start - } - sql = sql[:start] + sql[end+2:] - } - } else { - sql = String(stmt) - } - sql = strings.TrimFunc(sql, unicode.IsSpace) - - return sql -} - // ExtractMysqlComment extracts the version and SQL from a comment-only query // such as /*!50708 sql here */ func ExtractMysqlComment(sql string) (version string, innerSQL string) { diff --git a/go/vt/sqlparser/comments_test.go b/go/vt/sqlparser/comments_test.go index 33ee965815c..3d875faf1cb 100644 --- a/go/vt/sqlparser/comments_test.go +++ b/go/vt/sqlparser/comments_test.go @@ -119,20 +119,32 @@ func TestSplitComments(t *testing.T) { outSQL: "foo", outLeadingComments: "", outTrailingComments: "", + }, { + input: "select 1 from t where col = '*//*'", + outSQL: "select 1 from t where col = '*//*'", + outLeadingComments: "", + outTrailingComments: "", + }, { + input: "/*! select 1 */", + outSQL: "/*! select 1 */", + outLeadingComments: "", + outTrailingComments: "", }} for _, testCase := range testCases { - gotSQL, gotComments := SplitMarginComments(testCase.input) - gotLeadingComments, gotTrailingComments := gotComments.Leading, gotComments.Trailing + t.Run(testCase.input, func(t *testing.T) { + gotSQL, gotComments := SplitMarginComments(testCase.input) + gotLeadingComments, gotTrailingComments := gotComments.Leading, gotComments.Trailing - if gotSQL != testCase.outSQL { - t.Errorf("test input: '%s', got SQL\n%+v, want\n%+v", testCase.input, gotSQL, testCase.outSQL) - } - if gotLeadingComments != testCase.outLeadingComments { - t.Errorf("test input: '%s', got LeadingComments\n%+v, want\n%+v", testCase.input, gotLeadingComments, testCase.outLeadingComments) - } - if gotTrailingComments != testCase.outTrailingComments { - t.Errorf("test input: '%s', got TrailingComments\n%+v, want\n%+v", testCase.input, gotTrailingComments, testCase.outTrailingComments) - } + if gotSQL != testCase.outSQL { + t.Errorf("test input: '%s', got SQL\n%+v, want\n%+v", testCase.input, gotSQL, testCase.outSQL) + } + if gotLeadingComments != testCase.outLeadingComments { + t.Errorf("test input: '%s', got LeadingComments\n%+v, want\n%+v", testCase.input, gotLeadingComments, testCase.outLeadingComments) + } + if gotTrailingComments != testCase.outTrailingComments { + t.Errorf("test input: '%s', got TrailingComments\n%+v, want\n%+v", testCase.input, gotTrailingComments, testCase.outTrailingComments) + } + }) } } @@ -212,103 +224,6 @@ a`, } } -func TestRemoveComments(t *testing.T) { - var testCases = []struct { - input, outSQL string - }{{ - input: "/", - outSQL: "/", - }, { - input: "*/", - outSQL: "*/", - }, { - input: "/*/", - outSQL: "/*/", - }, { - input: "/*a", - outSQL: "/*a", - }, { - input: "/*a*", - outSQL: "/*a*", - }, { - input: "/*a**", - outSQL: "/*a**", - }, { - input: "/*b**a*/", - outSQL: "", - }, { - input: "/*a*/", - outSQL: "", - }, { - input: "/**/", - outSQL: "", - }, { - input: "/*!*/", - outSQL: "", - }, { - input: "/*!a*/", - outSQL: "", - }, { - input: "/*b*/ /*a*/", - outSQL: "", - }, { - input: `/*b*/ --foo -bar`, - outSQL: "bar", - }, { - input: "foo /* bar */", - outSQL: "foo", - }, { - input: "foo /* bar */ baz", - outSQL: "foo baz", - }, { - input: "/* foo */ bar", - outSQL: "bar", - }, { - input: "-- /* foo */ bar", - outSQL: "", - }, { - input: "foo -- bar */", - outSQL: "foo -- bar */", - }, { - input: `/* -foo */ bar`, - outSQL: "bar", - }, { - input: `-- foo bar -a`, - outSQL: "a", - }, { - input: `-- foo bar`, - outSQL: "", - }, { - input: "select * from customer where name like '*//*'", - outSQL: "select * from customer where name like '*//*'", - }, { - input: "select * from customer where name like '*/%/*'", - outSQL: "select * from customer where name like '*/%/*'", - }, { - input: "select * from customer where name like '*/%/*'", - outSQL: "select * from customer where name like '*/%/*'", - }, { - input: "insert into customer values(1, '*//*')", - outSQL: "insert into customer values (1, '*//*')", - }, { - input: "insert into t5 (col) values('/**/')", - outSQL: "insert into t5(col) values ('/**/')", - }, { - input: "select * from customer where name like 'abc*/la/*xyz'", - outSQL: "select * from customer where name like 'abc*/la/*xyz'", - }} - for _, testCase := range testCases { - gotSQL := StripComments(testCase.input) - - if gotSQL != testCase.outSQL { - t.Errorf("test input: '%s', got SQL\n%+v, want\n%+v", testCase.input, gotSQL, testCase.outSQL) - } - } -} - func TestExtractMysqlComment(t *testing.T) { var testCases = []struct { input, outSQL, outVersion string diff --git a/go/vt/vtexplain/vtexplain.go b/go/vt/vtexplain/vtexplain.go index 1887fb241a0..d1db8cea0e7 100644 --- a/go/vt/vtexplain/vtexplain.go +++ b/go/vt/vtexplain/vtexplain.go @@ -192,7 +192,7 @@ func parseSchema(sqlSchema string, opts *Options) ([]*sqlparser.DDL, error) { if sql == "" { break } - sql = sqlparser.StripComments(sql) + sql, _ = sqlparser.SplitMarginComments(sql) if sql == "" { continue } diff --git a/go/vt/vtexplain/vtexplain_flaky_test.go b/go/vt/vtexplain/vtexplain_flaky_test.go index 59a1efb2fcf..36b84ba9351 100644 --- a/go/vt/vtexplain/vtexplain_flaky_test.go +++ b/go/vt/vtexplain/vtexplain_flaky_test.go @@ -48,7 +48,7 @@ func initTest(mode string, opts *Options, t *testing.T) { opts.ExecutionMode = mode err = Init(string(vSchema), string(schema), opts) - require.NoError(t, err, "vtexplain Init error") + require.NoError(t, err, "vtexplain Init error\n%s", string(schema)) } func testExplain(testcase string, opts *Options, t *testing.T) { From 309c888f5fa1cefe4fef14608aadcd69a0c54a9e Mon Sep 17 00:00:00 2001 From: Saif Alharthi Date: Wed, 1 Apr 2020 12:48:04 -0700 Subject: [PATCH 4/4] Remove Partition comment from test to match current behaviour for Vitess Signed-off-by: Saif Alharthi --- go/vt/vtexplain/testdata/test-schema.sql | 6 ------ 1 file changed, 6 deletions(-) diff --git a/go/vt/vtexplain/testdata/test-schema.sql b/go/vt/vtexplain/testdata/test-schema.sql index e89714a474e..b901cceb25e 100644 --- a/go/vt/vtexplain/testdata/test-schema.sql +++ b/go/vt/vtexplain/testdata/test-schema.sql @@ -60,10 +60,4 @@ create table test_partitioned ( date_create int, primary key(id) ) Engine=InnoDB -/*!50100 PARTITION BY RANGE (date_create) -(PARTITION p2018_06_14 VALUES LESS THAN (1528959600) ENGINE = InnoDB, - PARTITION p2018_06_15 VALUES LESS THAN (1529046000) ENGINE = InnoDB, - PARTITION p2018_06_16 VALUES LESS THAN (1529132400) ENGINE = InnoDB, - PARTITION p2018_06_17 VALUES LESS THAN (1529218800) ENGINE = InnoDB) -*/ ;