Fix expression precedence for lambda, ternary and NOT operators#279
Conversation
The precedence table disagreed with ClickHouse in several places and two operators were unreachable. All of these misparses round-tripped to the same SQL text, so the golden tests never caught them: - Lambda `->` bound tightest instead of loosest: `x -> x + 1` parsed as `(x -> x) + 1`. It now binds loosest and is right-associative. - Ternary `?:` bound above OR/AND: `a OR b ? 1 : 2` parsed as `a OR (b ? 1 : 2)` instead of `(a OR b) ? 1 : 2`. - Prefix NOT grabbed only a primary: `NOT a = b` parsed as `(NOT a) = b` instead of `NOT (a = b)`, and `NOT NOT a` misparsed the second NOT as a column named NOT. - `x NOT BETWEEN 1 AND 2` was a hard error: the BETWEEN branch inside parseInfix's NOT case sat after the default arm and was dead code. BetweenClause gains a Not field. - `NOT IN` entered through NOT's low precedence while `IN` had its own, so `a = b IN (1)` and `a = b NOT IN (1)` grouped differently. - getNextPrecedence listed TokenKindDot/TokenKindDash twice. - INTERSECT was not a keyword at all: `SELECT 1 INTERSECT SELECT 2` misparsed as `SELECT 1 AS INTERSECT` then errored. It is now wired like EXCEPT (SelectQuery.Intersect), including walk/format support; walk also gains the previously missing Except child. Golden JSON diffs are purely the two new fields (BetweenClause.Not, SelectQuery.Intersect); no format output changed. New structural tests in precedence_test.go assert the tree shapes directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2586409eef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| selectStmt.Except = exceptExpr | ||
| case p.tryConsumeKeywords(KeywordIntersect): | ||
| intersectExpr, err := p.parseSelectQuery(p.Pos()) |
There was a problem hiding this comment.
When an INTERSECT is followed later by a lower-priority UNION or EXCEPT, this recursive RHS parse lets that lower-priority operator bind inside the right operand. ClickHouse documents INTERSECT as higher priority than UNION/EXCEPT (https://clickhouse.com/docs/sql-reference/statements/select/intersect), so SELECT a FROM t1 INTERSECT SELECT b FROM t2 UNION ALL SELECT c FROM t3 should be represented as (t1 INTERSECT t2) UNION ALL t3; currently the top node is Intersect and the RHS carries UnionAll, changing the AST semantics for consumers.
Useful? React with 👍 / 👎.
| case p.tryConsumeKeywords(KeywordIntersect): | ||
| intersectExpr, err := p.parseSelectQuery(p.Pos()) |
There was a problem hiding this comment.
ClickHouse supports INTERSECT DISTINCT (https://clickhouse.com/docs/sql-reference/statements/select/intersect), but this branch consumes only the bare INTERSECT keyword and immediately expects the next token to start a SELECT. Valid queries such as SELECT number FROM numbers(10) INTERSECT DISTINCT SELECT number FROM numbers(5) therefore fail at DISTINCT instead of parsing and formatting.
Useful? React with 👍 / 👎.
1 participant
Summary
The precedence table in
parser_column.godisagreed with ClickHouse in several places, and two constructs were unreachable. Crucially, all of these misparses round-trip to identical SQL text, so the round-trip-based golden tests never caught them — only consumers reading the AST saw wrong trees.arrayMap(x -> x + 1, arr)(x -> x) + 1x + 1x -> y -> x + y(x -> y) -> (x + y)x -> (y -> x + y)(right-assoc)NOT a = b(NOT a) = bNOT (a = b)NOT NOT aNOT (column \"NOT\") AS aNOT (NOT a)a OR b ? 1 : 2a OR (b ? 1 : 2)(a OR b) ? 1 : 2x NOT BETWEEN 1 AND 2BetweenClause.Nota = b NOT IN (1)(a = b) NOT IN (1)a = (b NOT IN (1)), same asINSELECT 1 INTERSECT SELECT 21 AS INTERSECT, then errorChanges
->binds loosest, then?:, then OR/AND. Removed the duplicateTokenKindDot/TokenKindDasharms ingetNextPrecedence.->gets its own right-associativeparseInfixcase (body parsed atprecedence-1).NOTparses its operand atPrecedenceNotviaparseSubExprinstead of stopping at a primary expression.NOTbinds with the precedence of the operator it negates (peek-based), andNOT BETWEENis reachable;BetweenClausegains aNotfield (parser/formatter wired).GLOBAL(only valid beforeIN) binds likeINitself.INTERSECTkeyword +SelectQuery.Intersect, mirroringEXCEPTin parser, formatter, Accept, and Walk. While wiring it I foundWalknever visitedSelectQuery.Excepteither — added both.AST / grammar impact
BetweenClause.Not,SelectQuery.Intersect(the golden JSON churn is exactly these two fields; no format output changed for any existing fixture).precedence_test.goassert tree shapes directly, since round-trip goldens cannot catch precedence regressions.🤖 Generated with Claude Code