Skip to content

Commit 6ddfb36

Browse files
committed
Fix OR/AND chain flattening in EXPLAIN by tracking explicit parenthesization
Add a Parenthesized field to BinaryExpr in the AST to track when an expression was wrapped in explicit parentheses. This allows the EXPLAIN output to correctly distinguish between: - "a OR b OR c" → or(a, b, c) [flattened, no explicit parens] - "(a OR b) OR c" → or(or(a,b), c) [nested, explicit parens preserved] Changes: - ast/ast.go: Add Parenthesized field to BinaryExpr - parser/expression.go: Set Parenthesized=true when parsing grouped expressions - internal/explain/expressions.go: Add collectLogicalOperands() that respects the Parenthesized flag when flattening OR/AND chains This fixes the 02711_trim_aliases test and many other tests that were previously in explain_todo due to incorrect OR/AND chain handling.
1 parent ac43df1 commit 6ddfb36

432 files changed

Lines changed: 408 additions & 1883 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

ast/ast.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1037,10 +1037,11 @@ const (
10371037

10381038
// BinaryExpr represents a binary expression.
10391039
type BinaryExpr struct {
1040-
Position token.Position `json:"-"`
1041-
Left Expression `json:"left"`
1042-
Op string `json:"op"`
1043-
Right Expression `json:"right"`
1040+
Position token.Position `json:"-"`
1041+
Left Expression `json:"left"`
1042+
Op string `json:"op"`
1043+
Right Expression `json:"right"`
1044+
Parenthesized bool `json:"parenthesized,omitempty"` // True if wrapped in explicit parentheses
10441045
}
10451046

10461047
func (b *BinaryExpr) Pos() token.Position { return b.Position }

internal/explain/expressions.go

Lines changed: 51 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -264,10 +264,17 @@ func explainBinaryExpr(sb *strings.Builder, n *ast.BinaryExpr, indent string, de
264264
return
265265
}
266266

267-
// Note: OR and AND chains are NOT flattened because we cannot distinguish
268-
// implicit left-associativity from explicit parenthesization in the AST.
269-
// For example: "a OR b OR c" and "(a OR b) OR c" produce identical parse trees
270-
// but ClickHouse EXPLAIN outputs different structures for them.
267+
// For OR and AND operators, flatten left-associative chains
268+
// but preserve explicit parenthesization like "(a OR b) OR c"
269+
if n.Op == "OR" || n.Op == "AND" {
270+
operands := collectLogicalOperands(n)
271+
fmt.Fprintf(sb, "%sFunction %s (children %d)\n", indent, fnName, 1)
272+
fmt.Fprintf(sb, "%s ExpressionList (children %d)\n", indent, len(operands))
273+
for _, op := range operands {
274+
Node(sb, op, depth+2)
275+
}
276+
return
277+
}
271278

272279
fmt.Fprintf(sb, "%sFunction %s (children %d)\n", indent, fnName, 1)
273280
fmt.Fprintf(sb, "%s ExpressionList (children %d)\n", indent, 2)
@@ -296,6 +303,26 @@ func collectConcatOperands(n *ast.BinaryExpr) []ast.Expression {
296303
return operands
297304
}
298305

306+
// collectLogicalOperands flattens chained OR/AND operations into a list of operands,
307+
// but respects explicit parenthesization. For example:
308+
// - "a OR b OR c" → [a, b, c] (flattened)
309+
// - "(a OR b) OR c" → [(a OR b), c] (preserved due to explicit parens)
310+
func collectLogicalOperands(n *ast.BinaryExpr) []ast.Expression {
311+
var operands []ast.Expression
312+
313+
// Recursively collect from left side if it's the same operator AND not parenthesized
314+
if left, ok := n.Left.(*ast.BinaryExpr); ok && left.Op == n.Op && !left.Parenthesized {
315+
operands = append(operands, collectLogicalOperands(left)...)
316+
} else {
317+
operands = append(operands, n.Left)
318+
}
319+
320+
// Don't flatten right side - explicit parentheses would be on the left in left-associative parsing
321+
operands = append(operands, n.Right)
322+
323+
return operands
324+
}
325+
299326
func explainUnaryExpr(sb *strings.Builder, n *ast.UnaryExpr, indent string, depth int) {
300327
// Handle negate of literal numbers - output as negative literal instead of function
301328
if n.Op == "-" {
@@ -415,6 +442,14 @@ func explainAliasedExpr(sb *strings.Builder, n *ast.AliasedExpr, depth int) {
415442
for _, op := range operands {
416443
Node(sb, op, depth+2)
417444
}
445+
} else if e.Op == "OR" || e.Op == "AND" {
446+
// For OR and AND operators, flatten but respect explicit parenthesization
447+
operands := collectLogicalOperands(e)
448+
fmt.Fprintf(sb, "%sFunction %s (alias %s) (children %d)\n", indent, fnName, escapeAlias(n.Alias), 1)
449+
fmt.Fprintf(sb, "%s ExpressionList (children %d)\n", indent, len(operands))
450+
for _, op := range operands {
451+
Node(sb, op, depth+2)
452+
}
418453
} else {
419454
fmt.Fprintf(sb, "%sFunction %s (alias %s) (children %d)\n", indent, fnName, escapeAlias(n.Alias), 1)
420455
fmt.Fprintf(sb, "%s ExpressionList (children %d)\n", indent, 2)
@@ -587,6 +622,18 @@ func explainWithElement(sb *strings.Builder, n *ast.WithElement, indent string,
587622
for _, op := range operands {
588623
Node(sb, op, depth+2)
589624
}
625+
} else if e.Op == "OR" || e.Op == "AND" {
626+
// For OR and AND operators, flatten but respect explicit parenthesization
627+
operands := collectLogicalOperands(e)
628+
if n.Name != "" {
629+
fmt.Fprintf(sb, "%sFunction %s (alias %s) (children %d)\n", indent, fnName, n.Name, 1)
630+
} else {
631+
fmt.Fprintf(sb, "%sFunction %s (children %d)\n", indent, fnName, 1)
632+
}
633+
fmt.Fprintf(sb, "%s ExpressionList (children %d)\n", indent, len(operands))
634+
for _, op := range operands {
635+
Node(sb, op, depth+2)
636+
}
590637
} else {
591638
if n.Name != "" {
592639
fmt.Fprintf(sb, "%sFunction %s (alias %s) (children %d)\n", indent, fnName, n.Name, 1)

parser/expression.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -910,6 +910,13 @@ func (p *Parser) parseGroupedOrTuple() ast.Expression {
910910
}
911911

912912
p.expect(token.RPAREN)
913+
914+
// Mark binary expressions as parenthesized so we can preserve explicit
915+
// grouping in EXPLAIN output (e.g., "(a OR b) OR c" vs "a OR b OR c")
916+
if binExpr, ok := first.(*ast.BinaryExpr); ok {
917+
binExpr.Parenthesized = true
918+
}
919+
913920
return first
914921
}
915922

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
{"explain_todo":{"stmt11":true,"stmt12":true,"stmt5":true,"stmt6":true,"stmt7":true}}
1+
{}
Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1 @@
1-
{
2-
"explain_todo": {
3-
"stmt2": true
4-
}
5-
}
1+
{}
Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1 @@
1-
{
2-
"explain_todo": {
3-
"stmt6": true,
4-
"stmt7": true,
5-
"stmt8": true
6-
}
7-
}
1+
{}
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
{"explain_todo":{"stmt4":true}}
1+
{}
Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1 @@
1-
{
2-
"explain_todo": {
3-
"stmt28": true,
4-
"stmt42": true,
5-
"stmt43": true,
6-
"stmt44": true,
7-
"stmt45": true,
8-
"stmt46": true,
9-
"stmt47": true,
10-
"stmt48": true
11-
}
12-
}
1+
{}
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
{"explain_todo":{"stmt3":true,"stmt4":true}}
1+
{}
Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1 @@
1-
{
2-
"explain_todo": {
3-
"stmt20": true,
4-
"stmt21": true
5-
}
6-
}
1+
{}

0 commit comments

Comments
 (0)