Skip to content

Commit dafa4e7

Browse files
committed
Store LIMIT BY expressions in AST and update documentation
- Add LimitBy field to SelectQuery AST to store LIMIT BY expressions - Update parser to store LIMIT BY expressions instead of discarding them - Add documentation in CLAUDE.md about explain.txt being ground truth - Update AST golden files to include limit_by field - Add comments in explain output clarifying LIMIT BY is not in EXPLAIN AST Note: LIMIT BY expressions are stored in the AST for semantic information but are not output in EXPLAIN AST (matching ClickHouse behavior).
1 parent 2b1e86c commit dafa4e7

File tree

11 files changed

+66
-9
lines changed

11 files changed

+66
-9
lines changed

ast/ast.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ type SelectQuery struct {
6161
Window []*WindowDefinition `json:"window,omitempty"`
6262
OrderBy []*OrderByElement `json:"order_by,omitempty"`
6363
Limit Expression `json:"limit,omitempty"`
64+
LimitBy []Expression `json:"limit_by,omitempty"`
6465
Offset Expression `json:"offset,omitempty"`
6566
Settings []*SettingExpr `json:"settings,omitempty"`
6667
IntoOutfile *IntoOutfileClause `json:"into_outfile,omitempty"`

internal/explain/select.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,8 @@ func explainSelectQuery(sb *strings.Builder, n *ast.SelectQuery, indent string,
106106
if n.Limit != nil {
107107
Node(sb, n.Limit, depth+1)
108108
}
109+
// Note: LIMIT BY expressions are stored in the AST but not output in EXPLAIN AST
110+
// (ClickHouse doesn't output them in EXPLAIN AST)
109111
// SETTINGS - output here if there's no FORMAT, otherwise it's at SelectWithUnionQuery level
110112
if len(n.Settings) > 0 && n.Format == nil {
111113
fmt.Fprintf(sb, "%s Set\n", indent)
@@ -214,6 +216,7 @@ func countSelectQueryChildren(n *ast.SelectQuery) int {
214216
if n.Offset != nil {
215217
count++
216218
}
219+
// Note: LimitBy is stored in AST but not counted in EXPLAIN AST children
217220
// SETTINGS is counted here only if there's no FORMAT
218221
// If FORMAT is present, SETTINGS is at SelectWithUnionQuery level
219222
if len(n.Settings) > 0 && n.Format == nil {

parser/CLAUDE.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,16 @@
11
# Parser Development Notes
22

3+
## Test Data Files
4+
5+
The `testdata/` directory contains test cases from ClickHouse. Each test has:
6+
7+
- `query.sql` - The SQL query to parse
8+
- `explain.txt` - **DO NOT MODIFY** - This is the ground truth output from ClickHouse's `EXPLAIN AST` command. Our parser must produce output that matches this exactly.
9+
- `metadata.json` - Test metadata (todo, skip, etc.)
10+
- `ast.json` - Optional golden file for AST regression testing
11+
12+
To fix a failing test, you must fix the **parser** to produce output matching `explain.txt`, never modify `explain.txt` itself.
13+
314
## Running Tests
415

516
Always run parser tests with a 5 second timeout:

parser/parser.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -375,9 +375,9 @@ func (p *Parser) parseSelect() *ast.SelectQuery {
375375
// LIMIT BY clause (ClickHouse specific: LIMIT n BY expr1, expr2, ...)
376376
if p.currentIs(token.BY) {
377377
p.nextToken()
378-
// Parse LIMIT BY expressions - skip them for now
378+
// Parse LIMIT BY expressions
379379
for !p.isEndOfExpression() {
380-
p.parseExpression(LOWEST)
380+
sel.LimitBy = append(sel.LimitBy, p.parseExpression(LOWEST))
381381
if p.currentIs(token.COMMA) {
382382
p.nextToken()
383383
} else {

parser/testdata/00176_distinct_limit_by_limit_bug_43377/ast.json

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,14 @@
9494
"limit": {
9595
"type": "Integer",
9696
"value": 10
97-
}
97+
},
98+
"limit_by": [
99+
{
100+
"parts": [
101+
"Title"
102+
]
103+
}
104+
]
98105
}
99106
]
100107
}

parser/testdata/00583_limit_by_expressions/ast.json

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,13 @@
2222
"limit": {
2323
"type": "Integer",
2424
"value": 1
25-
}
25+
},
26+
"limit_by": [
27+
{
28+
"type": "Integer",
29+
"value": 1
30+
}
31+
]
2632
}
2733
]
2834
}

parser/testdata/00590_limit_by_column_removal/ast.json

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,14 @@
4343
"limit": {
4444
"type": "Integer",
4545
"value": 1
46-
}
46+
},
47+
"limit_by": [
48+
{
49+
"parts": [
50+
"y"
51+
]
52+
}
53+
]
4754
}
4855
]
4956
}

parser/testdata/01471_limit_by_format/ast.json

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,10 @@
2121
"limit": {
2222
"type": "Integer",
2323
"value": 1
24-
}
24+
},
25+
"limit_by": [
26+
{}
27+
]
2528
}
2629
]
2730
}

parser/testdata/02281_limit_by_distributed/ast.json

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,14 @@
9898
"limit": {
9999
"type": "Integer",
100100
"value": 1
101-
}
101+
},
102+
"limit_by": [
103+
{
104+
"parts": [
105+
"k"
106+
]
107+
}
108+
]
102109
}
103110
]
104111
}

parser/testdata/03290_limit_by_segv/ast.json

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,12 @@
2727
"limit": {
2828
"type": "Integer",
2929
"value": 0
30-
}
30+
},
31+
"limit_by": [
32+
{
33+
"pattern": "1"
34+
}
35+
]
3136
}
3237
]
3338
}

0 commit comments

Comments
 (0)