Context
We use this parser at SigNoz to validate user-authored ClickHouse SQL before it reaches our telemetry store. This issue surfaced while running our production query corpus through the parser: the statements are valid ClickHouse but produce no AST, so they cannot be validated.
We would like to contribute the fix for this upstream rather than work around it locally, and we intend to do the same for the other gaps we find. Reporting first so the approach can be agreed before we open a PR.
Summary
-1 / +1 following a closing bracket is lexed as a single signed-literal token, so the parser sees two adjacent expressions with no operator between them and rejects SQL that ClickHouse accepts.
Reproduces on v0.5.2 and on master (e4c6e11).
Reproduction
_, err := parser.NewParser("SELECT (1)-1").ParseStmts()
line 1:11 <EOF> or ';' was expected, but got: "-1"
SELECT (1)-1
^^
All of the following are rejected:
SELECT (1)-1 -- <EOF> or ';' was expected, but got: "-1"
SELECT (1)+1 -- <EOF> or ';' was expected, but got: "+1"
SELECT (1)-1.5 -- <EOF> or ';' was expected, but got: "-1.5"
SELECT arr[1]-1 -- <EOF> or ';' was expected, but got: "-1"
SELECT (f()-1) -- expected ')', but got '<int>'
SELECT (now()-1)*2 -- expected ')', but got '<int>'
SELECT (toUnixTimestamp(now())-3600)*1000000000 -- expected ')', but got '<int>'
ClickHouse accepts these
clickhouse local -q "SELECT (1)-1"
Proposed fix
Lexer.hasPrecedenceToken disambiguates unary from binary +/-, but its set of tokens that can terminate an expression omits the closing brackets:
func (l *Lexer) hasPrecedenceToken(last *Token) bool {
return last != nil && (last.Kind == TokenKindIdent ||
last.Kind == TokenKindKeyword ||
last.Kind == TokenKindInt ||
last.Kind == TokenKindFloat ||
last.Kind == TokenKindString)
}
This explains the split above: a-1 and 1-1 parse because an identifier and a number are both in the set, while (1)-1 and arr[1]-1 do not. A ) or ] also terminates an expression, so a following - is a binary operator in exactly the same way.
- Adding TokenKindRParen and TokenKindRBracket to that set resolves every case.
- Adding more fixtures under parser/testdata/query/ covering the bracket, function-call and array-index forms.
Context
We use this parser at SigNoz to validate user-authored ClickHouse SQL before it reaches our telemetry store. This issue surfaced while running our production query corpus through the parser: the statements are valid ClickHouse but produce no AST, so they cannot be validated.
We would like to contribute the fix for this upstream rather than work around it locally, and we intend to do the same for the other gaps we find. Reporting first so the approach can be agreed before we open a PR.
Summary
-1 / +1 following a closing bracket is lexed as a single signed-literal token, so the parser sees two adjacent expressions with no operator between them and rejects SQL that ClickHouse accepts.
Reproduces on v0.5.2 and on master (e4c6e11).
Reproduction
line 1:11 <EOF> or ';' was expected, but got: "-1" SELECT (1)-1 ^^All of the following are rejected:
ClickHouse accepts these
Proposed fix
Lexer.hasPrecedenceToken disambiguates unary from binary +/-, but its set of tokens that can terminate an expression omits the closing brackets:
This explains the split above: a-1 and 1-1 parse because an identifier and a number are both in the set, while (1)-1 and arr[1]-1 do not. A ) or ] also terminates an expression, so a following - is a binary operator in exactly the same way.