diff --git a/parser/keyword.go b/parser/keyword.go index be1c4f1..2488baa 100644 --- a/parser/keyword.go +++ b/parser/keyword.go @@ -259,6 +259,70 @@ const ( KeywordSecurity = "SECURITY" ) +// reservedKeywords are the structural keywords — statement starters, clause +// starters and expression operators — that cannot be used as bare identifiers: +// letting them match identifier positions makes a missing name silently swallow +// the next clause (e.g. `SELECT a FROM WHERE b = 1` parsing FROM as an alias). +// Every other keyword is non-reserved and is accepted anywhere an identifier +// is expected (see Parser.matchTokenKind). Positions where even a reserved +// keyword is provably used as a name — after AS, after a dot in a qualified +// name, or a lookahead-disambiguated select item — use parseAnyKeyword. +// +// Keywords that double as ClickHouse function or engine names (IF, LEFT, +// RIGHT, ANY, MIN, MAX, TRIM, SET, JOIN...) must stay non-reserved. +var reservedKeywords = NewSet( + KeywordAlter, + KeywordAnd, + KeywordAs, + KeywordBetween, + KeywordBy, + KeywordCase, + KeywordCreate, + KeywordCross, + KeywordDescribe, + KeywordDistinct, + KeywordDrop, + KeywordElse, + KeywordEnd, + KeywordExcept, + KeywordExplain, + KeywordFormat, + KeywordFrom, + KeywordGrant, + KeywordGroup, + KeywordHaving, + KeywordIlike, + KeywordIn, + KeywordInner, + KeywordInsert, + KeywordInterval, + KeywordInto, + KeywordIs, + KeywordKill, + KeywordLike, + KeywordLimit, + KeywordNot, + KeywordOffset, + KeywordOn, + KeywordOptimize, + KeywordOr, + KeywordOrder, + KeywordPrewhere, + KeywordRename, + KeywordSelect, + KeywordSettings, + KeywordShow, + KeywordThen, + KeywordTruncate, + KeywordUnion, + KeywordUse, + KeywordUsing, + KeywordWhen, + KeywordWhere, + KeywordWindow, + KeywordWith, +) + var keywords = NewSet( KeywordAdd, KeywordAdmin, diff --git a/parser/keyword_test.go b/parser/keyword_test.go new file mode 100644 index 0000000..bfe68df --- /dev/null +++ b/parser/keyword_test.go @@ -0,0 +1,123 @@ +package parser + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestReservedKeywordsAreKeywords(t *testing.T) { + for _, kw := range reservedKeywords.Members() { + require.True(t, keywords.Contains(kw), + "reserved keyword %q is missing from the keywords set", kw) + } +} + +// TestReservedKeywordRejectedAsIdentifier asserts that a reserved keyword no +// longer silently fills an identifier slot: a missing name before a clause +// keyword must fail at the keyword instead of swallowing it (the bug class +// behind #268/#269: e.g. `SELECT a FROM WHERE b = 1` used to parse FROM as a +// bare alias of `a`). +func TestReservedKeywordRejectedAsIdentifier(t *testing.T) { + cases := []string{ + "SELECT a FROM WHERE b = 1", // FROM must not become an alias of `a` + "SELECT a, b FROM GROUP BY a", // FROM must not become an alias of `b` + "INSERT INTO SELECT 1", // SELECT must not become a table name + "SELECT a AS FROM t", // explicit alias FROM consumes the clause keyword + "SELECT a FROM t JOIN ON a = b", // ON must not become a table name + "CREATE TABLE t (from String) ENGINE=Memory", // reserved keyword as column name needs quoting + } + for _, sql := range cases { + t.Run(sql, func(t *testing.T) { + _, err := NewParser(sql).ParseStmts() + require.Error(t, err) + }) + } +} + +// TestNonReservedKeywordAsIdentifier asserts that non-reserved keywords keep +// working as identifiers anywhere an identifier is expected. +func TestNonReservedKeywordAsIdentifier(t *testing.T) { + cases := []string{ + "SELECT key FROM t", + "SELECT date, first, last, timestamp FROM t", + "CREATE TABLE t (key String, date Date) ENGINE=Memory", + "SELECT t.key FROM t", + "SELECT if(a, 1, 2), any(b), left(c, 1) FROM t", + "SELECT * FROM t WHERE key = 1", + } + for _, sql := range cases { + t.Run(sql, func(t *testing.T) { + _, err := NewParser(sql).ParseStmts() + require.NoError(t, err) + }) + } +} + +// TestReservedKeywordInDisambiguatedPositions asserts that reserved keywords +// are still accepted as names where context proves they cannot start a clause: +// after AS, after a dot in a qualified name, lookahead-disambiguated select +// items, query parameters, window names, and GRANT options. +func TestReservedKeywordInDisambiguatedPositions(t *testing.T) { + cases := []string{ + "SELECT 1 AS from", + "SELECT 1 AS interval, 2 AS from, 3 AS limit", + "SELECT * FROM t AS from", + "SELECT a FROM db.from", + "SELECT t.from FROM t", + "SELECT a, limit FROM t", + "SELECT case;", + "SELECT limit", + "SELECT a FROM t WHERE ts < {end:UInt32}", + "SELECT sum(x) OVER (order) FROM t WINDOW order AS (PARTITION BY team)", + "SELECT sum(x) OVER order FROM t WINDOW order AS (PARTITION BY team)", + "GRANT SELECT(x) ON db.table TO john WITH GRANT OPTION", + "GRANT SELECT ON db.from TO john", + "CREATE TABLE t (j JSON(max_dynamic_paths=1, SKIP a.from)) ENGINE=Memory", + } + for _, sql := range cases { + t.Run(sql, func(t *testing.T) { + _, err := NewParser(sql).ParseStmts() + require.NoError(t, err) + }) + } +} + +// TestReservedOperatorKeywordsAreCallable covers the regressions from the +// review of #275: operator keywords double as ordinary ClickHouse function +// names and must stay callable when followed by '('. +func TestReservedOperatorKeywordsAreCallable(t *testing.T) { + inputs := []string{ + "SELECT and(a, b) FROM t", + "SELECT or(a, b) FROM t", + "SELECT in(1, [1])", + "SELECT like(s, '%a%') FROM t", + "SELECT ilike(s, '%a%') FROM t", + } + for _, sql := range inputs { + t.Run(sql, func(t *testing.T) { + stmts, err := NewParser(sql).ParseStmts() + require.NoError(t, err) + require.Len(t, stmts, 1) + }) + } +} + +// TestReservedKeywordAliasesAfterAs covers the review regressions of #275: +// AS proves the next token is an alias name, so even reserved keywords are +// accepted in expression lists, ORDER BY, and non-parenthesized CTEs. +func TestReservedKeywordAliasesAfterAs(t *testing.T) { + inputs := []string{ + "SELECT (1 AS from)", + "SELECT sum(x AS from) FROM t", + "SELECT a FROM t ORDER BY x AS from", + "WITH 1 AS from SELECT from", + } + for _, sql := range inputs { + t.Run(sql, func(t *testing.T) { + stmts, err := NewParser(sql).ParseStmts() + require.NoError(t, err) + require.Len(t, stmts, 1) + }) + } +} diff --git a/parser/parse_system.go b/parser/parse_system.go index c7d4352..98cb65f 100644 --- a/parser/parse_system.go +++ b/parser/parse_system.go @@ -1278,7 +1278,9 @@ func (p *Parser) parseGrantOption(_ Pos) (string, error) { if err := p.expectKeyword(KeywordWith); err != nil { return "", err } - ident, err := p.parseIdent() + // Between WITH and OPTION the token can only be the option name, which may + // be a reserved keyword (e.g. `WITH GRANT OPTION`). + ident, err := p.parseAnyKeyword() if err != nil { return "", err } @@ -1299,7 +1301,12 @@ func (p *Parser) parseGrantSource(_ Pos) (*TableIdentifier, error) { Table: ident, }, nil } - dotIdent, err := p.parseIdentOrStar() + var dotIdent *Ident + if p.matchTokenKind("*") { + dotIdent, err = p.parseIdentOrStar() + } else { + dotIdent, err = p.parseAnyKeyword() + } if err != nil { return nil, err } diff --git a/parser/parser_column.go b/parser/parser_column.go index bc61dbd..badd589 100644 --- a/parser/parser_column.go +++ b/parser/parser_column.go @@ -152,8 +152,10 @@ func (p *Parser) parseInfix(expr Expr, precedence int) (Expr, error) { // access column with dot notation var rightExpr Expr var err error - if p.matchTokenKind(TokenKindIdent) { - rightExpr, err = p.parseIdent() + if p.matchTokenKind(TokenKindIdent, TokenKindKeyword) { + // After a dot the token can only be a member name, so even + // reserved keywords are accepted (e.g. `t.from`). + rightExpr, err = p.parseAnyKeyword() } else { rightExpr, err = p.parseDecimal(p.Pos()) } @@ -472,7 +474,7 @@ func (p *Parser) parseColumnExpr(pos Pos) (Expr, error) { //nolint:funlen // terminator/alias check). if p.keywordIsSelectItemIdentifier() || (p.matchTokenKind(TokenKindKeyword) && p.peekIsEndOfStatement()) { - return p.parseIdent() + return p.parseAnyKeyword() } switch { case p.matchKeyword(KeywordInterval): @@ -496,6 +498,12 @@ func (p *Parser) parseColumnExpr(pos Pos) (Expr, error) { //nolint:funlen return p.parseColumnExtractExpr(pos) case p.matchTokenKind(TokenKindIdent): return p.parseIdentOrFunction(pos) + case p.matchTokenKind(TokenKindKeyword) && p.peekTokenKind(TokenKindLParen): + // Reserved operator keywords stay callable as ordinary functions when + // followed by '(': and(a, b), or(a, b), in(x, set), like(s, pat), ... + // Keywords with dedicated syntax (CAST, CASE, EXTRACT, INTERVAL, ...) + // are handled by their own cases above. + return p.parseIdentOrFunction(pos) case p.matchTokenKind(TokenKindString): // string literal return p.parseString(pos) case p.matchTokenKind(TokenKindInt), @@ -676,8 +684,10 @@ func (p *Parser) parseInterval(requireKeyword bool) (*IntervalExpr, error) { } func (p *Parser) parseFunctionExpr(_ Pos) (*FunctionExpr, error) { - // parse function name - name, err := p.parseIdent() + // parse function name; callers gate entry (select-item modifiers match + // EXCEPT/APPLY/REPLACE first, INSERT INTO FUNCTION follows the FUNCTION + // keyword), so even reserved keywords are valid names here. + name, err := p.parseAnyKeyword() if err != nil { return nil, err } @@ -794,7 +804,9 @@ func (p *Parser) parseQueryParam(pos Pos) (*QueryParam, error) { return nil, err } - ident, err := p.parseIdent() + // Inside `{name:Type}` the token can only be the parameter name, so even + // reserved keywords are accepted (e.g. `{end:UInt32}`). + ident, err := p.parseAnyKeyword() if err != nil { return nil, err } @@ -844,7 +856,8 @@ func (p *Parser) parseColumnsExpr(pos Pos) (*ColumnExpr, error) { var alias *Ident if p.tryConsumeKeywords(KeywordAs) { - alias, err = p.parseIdent() + // after AS the token can only be an alias name, reserved keyword or not + alias, err = p.parseAnyKeyword() if err != nil { return nil, err } @@ -877,21 +890,21 @@ func (p *Parser) parseSelectItem() (*SelectItem, error) { var alias *Ident switch { case p.tryConsumeKeywords(KeywordAs): - // `SELECT 1 AS ` works for any keyword because - // matchTokenKind(TokenKindIdent) coerces TokenKindKeyword to ident - // (see parser_common.go matchTokenKind), so parseIdent accepts a - // keyword token here without needing a special-case. - alias, err = p.parseIdent() + // `SELECT 1 AS ` works for any keyword, reserved or not: + // after AS the token can only be an alias name. + alias, err = p.parseAnyKeyword() if err != nil { return nil, err } - case p.currentTokenKind() == TokenKindKeyword && !p.isSelectItemTerminatorKeyword(): + case p.matchTokenKind(TokenKindIdent) && !p.isSelectItemTerminatorKeyword(): + // A bare alias can be a normal identifier or non-reserved keyword; a + // reserved keyword here starts the next clause (e.g. `SELECT a FROM ...`). alias, err = p.parseIdent() if err != nil { return nil, err } default: - alias = p.tryParseIdent() + alias = nil } return &SelectItem{ @@ -1111,7 +1124,7 @@ func (p *Parser) parseJSONPath() (*JSONPath, error) { idents = append(idents, ident) for !p.lexer.isEOF() && p.tryConsumeTokenKind(TokenKindDot) != nil { - ident, err := p.parseIdent() + ident, err := p.parseAnyKeyword() if err != nil { return nil, err } diff --git a/parser/parser_common.go b/parser/parser_common.go index 18b927b..917fa14 100644 --- a/parser/parser_common.go +++ b/parser/parser_common.go @@ -64,9 +64,30 @@ func (p *Parser) Pos() Pos { return last.Pos } -func (p *Parser) matchTokenKind(kind TokenKind) bool { - return p.currentTokenKind() == kind || - (kind == TokenKindIdent && p.currentTokenKind() == TokenKindKeyword) +// matchTokenKind reports whether the current token matches any of the given +// kinds. A non-reserved keyword also matches TokenKindIdent: most ClickHouse +// keywords (DATE, KEY, FIRST, ...) are valid identifiers anywhere an +// identifier is expected. Reserved keywords (see reservedKeywords) do not +// match TokenKindIdent, so a missing identifier before e.g. FROM or WHERE +// fails fast instead of silently swallowing the clause keyword as a name; use +// matchTokenKind(TokenKindIdent, TokenKindKeyword) where any keyword - +// reserved or not - is acceptable. +func (p *Parser) matchTokenKind(kinds ...TokenKind) bool { + cur := p.currentTokenKind() + for _, kind := range kinds { + if cur == kind { + return true + } + } + if cur != TokenKindKeyword { + return false + } + for _, kind := range kinds { + if kind == TokenKindIdent { + return !reservedKeywords.Contains(strings.ToUpper(p.current().String)) + } + } + return false } // expectTokenKind consumes the current token if it is the given kind. @@ -141,6 +162,29 @@ func (p *Parser) tryParseIdent() *Ident { } } +// parseAnyKeyword parses the current token as an identifier, accepting +// any keyword token — reserved or not — as the name. Use it only in positions +// where context has already proven the token is a name and not the start of a +// clause or expression: after AS, after a dot in a qualified name, or a select +// item the lookahead disambiguated. +func (p *Parser) parseAnyKeyword() (*Ident, error) { + last := p.current() + if !p.matchTokenKind(TokenKindIdent, TokenKindKeyword) { + return nil, &ParseError{ + Pos: p.Pos(), + Got: last, + Expected: []TokenKind{TokenKindIdent}, + } + } + _ = p.lexer.consumeToken() + return &Ident{ + NamePos: last.Pos, + NameEnd: last.End, + Name: last.String, + QuoteType: last.QuoteType, + }, nil +} + func (p *Parser) parseIdent() (*Ident, error) { curToken := p.current() if err := p.expectTokenKind(TokenKindIdent); err != nil { @@ -194,13 +238,20 @@ func (p *Parser) tryParseDotIdent(_ Pos) (*Ident, error) { if p.tryConsumeTokenKind(TokenKindDot) == nil { return nil, nil // nolint } - return p.parseIdent() + // After a dot the token can only be a member name, so even reserved + // keywords are accepted (e.g. `db.from`, `t.limit`). + return p.parseAnyKeyword() } func (p *Parser) tryParseDotIdentOrString(_ Pos) (*Ident, error) { if p.tryConsumeTokenKind(TokenKindDot) == nil { return nil, nil // nolint } + // After a dot the token can only be a member name, so even reserved + // keywords are accepted (e.g. `db.from`). + if p.matchTokenKind(TokenKindKeyword) { + return p.parseAnyKeyword() + } return p.parseIdentOrString() } diff --git a/parser/parser_query.go b/parser/parser_query.go index f4d55ac..2ed6e4b 100644 --- a/parser/parser_query.go +++ b/parser/parser_query.go @@ -399,7 +399,9 @@ func (p *Parser) parseTableExpr(pos Pos) (*TableExpr, error) { tableEnd := expr.End() if p.tryConsumeKeywords(KeywordAs) { - alias, err := p.parseIdent() + // After AS the token can only be an alias name, so even reserved + // keywords are accepted (e.g. `FROM t AS from`). + alias, err := p.parseAnyKeyword() if err != nil { return nil, err } @@ -826,8 +828,10 @@ func (p *Parser) parseWindowCondition(pos Pos) (*WindowExpr, error) { } var windowName *Ident if p.canParseWindowNameInParens() { + // canParseWindowNameInParens already disambiguated keyword tokens + // (e.g. `OVER (order)` vs `OVER (ORDER BY ...)`). var err error - windowName, err = p.parseIdent() + windowName, err = p.parseAnyKeyword() if err != nil { return nil, err } @@ -859,7 +863,7 @@ func (p *Parser) parseWindowCondition(pos Pos) (*WindowExpr, error) { } func (p *Parser) canParseWindowNameInParens() bool { - if !p.matchTokenKind(TokenKindIdent) { + if !p.matchTokenKind(TokenKindIdent, TokenKindKeyword) { return false } if !p.matchTokenKind(TokenKindKeyword) { @@ -893,7 +897,9 @@ func (p *Parser) parseWindowClause(pos Pos) (*WindowClause, error) { windows := make([]*WindowDefinition, 0, 1) for { - windowName, err := p.parseIdent() + // After WINDOW (or a comma) the token can only be a window name, so + // even reserved keywords are accepted (e.g. `WINDOW order AS (...)`). + windowName, err := p.parseAnyKeyword() if err != nil { return nil, err } @@ -1194,7 +1200,8 @@ func (p *Parser) parseCTEStmt(pos Pos) (*CTEStmt, error) { Alias: selectQuery, }, nil } - name, err := p.parseIdent() + // after AS the token can only be an alias name, reserved keyword or not + name, err := p.parseAnyKeyword() if err != nil { return nil, err } diff --git a/parser/parser_table.go b/parser/parser_table.go index 0380b75..cf69034 100644 --- a/parser/parser_table.go +++ b/parser/parser_table.go @@ -400,7 +400,15 @@ func (p *Parser) parseCreateTable(pos Pos, orReplace bool) (*CreateTable, error) } func (p *Parser) parseIdentOrFunction(_ Pos) (Expr, error) { - ident, err := p.parseIdent() + var ident *Ident + var err error + if p.matchTokenKind(TokenKindKeyword) && p.peekTokenKind(TokenKindLParen) { + // reserved operator keywords stay callable as ordinary functions: + // and(a, b), or(a, b), in(x, set), like(s, pat), ... + ident, err = p.parseAnyKeyword() + } else { + ident, err = p.parseIdent() + } if err != nil { return nil, err } @@ -428,8 +436,11 @@ func (p *Parser) parseIdentOrFunction(_ Pos) (Expr, error) { if p.tryConsumeKeywords(KeywordOver) { var overExpr Expr switch { - case p.matchTokenKind(TokenKindIdent): - overExpr, err = p.parseIdent() + case p.matchTokenKind(TokenKindIdent, TokenKindKeyword): + // After OVER a bare token can only be a window name, so even + // reserved keywords are accepted (e.g. `OVER order`), + // mirroring the WINDOW clause definition side. + overExpr, err = p.parseAnyKeyword() case p.matchTokenKind(TokenKindLParen): overExpr, err = p.parseWindowCondition(p.Pos()) if err != nil { @@ -451,10 +462,12 @@ func (p *Parser) parseIdentOrFunction(_ Pos) (Expr, error) { return funcExpr, nil case p.tryConsumeTokenKind(TokenKindDot) != nil: switch { - case p.matchTokenKind(TokenKindIdent): + case p.matchTokenKind(TokenKindIdent, TokenKindKeyword): fields := []*Ident{ident} for { - child, err := p.parseIdent() + // After a dot the token can only be a member name, so even + // reserved keywords are accepted (e.g. `t.from`). + child, err := p.parseAnyKeyword() if err != nil { return nil, err } @@ -972,7 +985,8 @@ func (p *Parser) parseOrderExpr(pos Pos) (*OrderExpr, error) { } // consume the `AS` keyword _ = p.lexer.consumeToken() - alias, err = p.parseIdent() + // after AS the token can only be an alias name, reserved keyword or not + alias, err = p.parseAnyKeyword() if err != nil { return nil, err }