Skip to content

Commit 1056d7e

Browse files
committed
parser,scanner: fix bugs with parsing and scanning sql comments
Fixes a bug in the parser where retained comments are mistakenly added to multiple statements. This when parsing a multi-statement sql containing strings with the "retainComments" bool set to true. In this case, comments in a statement are propogated to subsequent statements in the same parse. Fixes a bug where `\n` characters are included in scanned singe- line comments. Now, these wont be included in comments and will instead be stripped out via the skipWhitespace function. Epic: None Release note: None
1 parent c8e3617 commit 1056d7e

File tree

4 files changed

+61
-3
lines changed

4 files changed

+61
-3
lines changed

pkg/sql/parser/parse.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,10 @@ func (p *Parser) parse(
201201
return statements.Statement[tree.Statement]{}, err
202202
}
203203

204+
// Once a statement has been parsed, reset the comments to ensure
205+
// that the next statement does not pick up comments from the previous
206+
// statement.
207+
defer p.scanner.ResetComments()
204208
return statements.Statement[tree.Statement]{
205209
AST: p.lexer.stmt,
206210
SQL: sql,

pkg/sql/parser/parse_internal_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ package parser
88
import (
99
"reflect"
1010
"testing"
11+
12+
"github.com/stretchr/testify/require"
1113
)
1214

1315
func TestScanOneStmt(t *testing.T) {
@@ -135,3 +137,44 @@ func TestScanOneStmt(t *testing.T) {
135137
}
136138
}
137139
}
140+
141+
func TestParseWithDept_retainComment(t *testing.T) {
142+
testData := []struct {
143+
in string
144+
exp [][]string
145+
}{
146+
{in: ``, exp: nil},
147+
{in: `SELECT 1;`, exp: [][]string{nil}},
148+
{in: `
149+
-- comment
150+
SELECT 1`, exp: [][]string{{`-- comment`}}},
151+
{in: `/* comment */ SELECT 1`, exp: [][]string{{`/* comment */`}}},
152+
{in: `SELECT 1 /* comment */`, exp: [][]string{{`/* comment */`}}},
153+
{in: `SELECT 1;SELECT 2`, exp: [][]string{nil, nil}},
154+
{in: `SELECT 1 /* block comment1 */ ;SELECT 2 /* block comment2 */`, exp: [][]string{{`/* block comment1 */`}, {`/* block comment2 */`}}},
155+
{in: `SELECT 1 /* block comment */ ;SELECT 2 -- single-line-comment`, exp: [][]string{{`/* block comment */`}, {`-- single-line-comment`}}},
156+
{in: `SELECT 1 /* block comment */ /* block comment */`, exp: [][]string{{`/* block comment */`, `/* block comment */`}}},
157+
{in: `
158+
/* This is a select 1 */
159+
SELECT 1 -- in-line comment
160+
;
161+
-- This is a select 2
162+
SELECT 2; -- comment ignore, not part of statement
163+
/** Comment ignored, not part of statement */
164+
`, exp: [][]string{{`/* This is a select 1 */`, `-- in-line comment`}, {`-- This is a select 2`}}},
165+
}
166+
var p Parser
167+
for _, d := range testData {
168+
t.Run(d.in, func(t *testing.T) {
169+
stmts, err := p.parseWithDepth(1, d.in, defaultNakedIntType, retainComments)
170+
require.NoError(t, err)
171+
172+
var res [][]string
173+
for i := range stmts {
174+
res = append(res, stmts[i].Comments)
175+
}
176+
177+
require.Equal(t, d.exp, res)
178+
})
179+
}
180+
}

pkg/sql/parser/scanner_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,14 +135,16 @@ func TestScanComment(t *testing.T) {
135135
remainder string
136136
}{
137137
{`/* hello */world`, "", "world"},
138+
{`/* hello */
139+
world`, "", "\nworld"},
138140
{`/* hello */*`, "", "*"},
139141
{`/* /* deeply /* nested */ comment */ */`, "", ""},
140142
{`/* /* */* */`, "", ""},
141143
{`/* /* /*/ */ */ */`, "", ""},
142144
{`/* multi line
143145
comment */`, "", ""},
144146
{`-- hello world
145-
foo`, "", "foo"},
147+
foo`, "", "\nfoo"},
146148
{`/*`, "unterminated comment", ""},
147149
{`/*/`, "unterminated comment", ""},
148150
{`/* /* */`, "unterminated comment", ""},

pkg/sql/scanner/scan.go

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,10 @@ func (s *Scanner) RetainComments() {
100100
s.retainComments = true
101101
}
102102

103+
func (s *Scanner) ResetComments() {
104+
s.Comments = nil
105+
}
106+
103107
// Cleanup is used to avoid holding on to memory unnecessarily (for the cases
104108
// where we reuse a Scanner).
105109
func (s *Scanner) Cleanup() {
@@ -596,8 +600,13 @@ func (s *Scanner) ScanComment(lval ScanSymType) (present, ok bool) {
596600
return false, true
597601
}
598602
for {
599-
switch s.next() {
600-
case eof, '\n':
603+
next := s.next()
604+
switch next {
605+
case eof:
606+
return true, true
607+
case '\n':
608+
// Don't include the new-line character in in-line comments.
609+
s.pos--
601610
return true, true
602611
}
603612
}

0 commit comments

Comments
 (0)