Skip to content

Commit b6a16ba

Browse files
claudeclaude
committed
Skip empty WindowDefinition in EXPLAIN AST output
ClickHouse's EXPLAIN AST doesn't output a WindowDefinition node when the window spec is empty (OVER ()). This fix updates the explain output to match ClickHouse's behavior by checking if the window spec has any content (PartitionBy, OrderBy, or Frame offset) before outputting WindowDefinition. Also updates metadata.json files for tests that now pass.
1 parent 16bd8a1 commit b6a16ba

178 files changed

Lines changed: 173 additions & 875 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.

‎internal/explain/functions.go‎

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,11 @@ func explainFunctionCallWithAlias(sb *strings.Builder, n *ast.FunctionCall, alia
1616
if len(n.Parameters) > 0 {
1717
children++ // parameters ExpressionList
1818
}
19-
// Only count WindowDefinition as a child for inline window specs (not named references)
20-
// When it's just a reference like "OVER w", it's shown in the SELECT's WINDOW clause instead
21-
// Inline specs include OVER () - empty window, and OVER (ORDER BY x) - window with spec
22-
// Named refs have n.Over.Name set and no inline definition
23-
hasInlineWindowSpec := n.Over != nil && n.Over.Name == ""
24-
if hasInlineWindowSpec {
19+
// Only count WindowDefinition as a child for inline window specs that have content
20+
// Empty OVER () doesn't produce a WindowDefinition in ClickHouse EXPLAIN AST
21+
// Named refs like "OVER w" are shown in the SELECT's WINDOW clause instead
22+
hasNonEmptyWindowSpec := n.Over != nil && n.Over.Name == "" && windowSpecHasContent(n.Over)
23+
if hasNonEmptyWindowSpec {
2524
children++ // WindowDefinition for OVER clause
2625
}
2726
// Normalize function name
@@ -72,12 +71,30 @@ func explainFunctionCallWithAlias(sb *strings.Builder, n *ast.FunctionCall, alia
7271
}
7372
// Window definition (for window functions with inline OVER clause)
7473
// WindowDefinition is a sibling to ExpressionList, so use the same indent
75-
// Only output for inline specs, not named references like "OVER w"
76-
if hasInlineWindowSpec {
74+
// Only output for non-empty inline specs, not named references like "OVER w"
75+
if hasNonEmptyWindowSpec {
7776
explainWindowSpec(sb, n.Over, indent+" ", depth+1)
7877
}
7978
}
8079

80+
// windowSpecHasContent returns true if the window spec has any content
81+
// (PartitionBy, OrderBy, or Frame with offset). Empty OVER () returns false.
82+
func windowSpecHasContent(w *ast.WindowSpec) bool {
83+
if w == nil {
84+
return false
85+
}
86+
if len(w.PartitionBy) > 0 {
87+
return true
88+
}
89+
if len(w.OrderBy) > 0 {
90+
return true
91+
}
92+
if w.Frame != nil && w.Frame.StartBound != nil && w.Frame.StartBound.Offset != nil {
93+
return true
94+
}
95+
return false
96+
}
97+
8198
func explainLambda(sb *strings.Builder, n *ast.Lambda, indent string, depth int) {
8299
// Lambda is represented as Function lambda with tuple of params and body
83100
fmt.Fprintf(sb, "%sFunction lambda (children %d)\n", indent, 1)

‎parser/testdata/00052_group_by_in/metadata.json‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1 @@
1-
{
2-
"explain_todo": {
3-
"stmt1": true
4-
}
5-
}
1+
{}

‎parser/testdata/00162_shard_global_join/metadata.json‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1 @@
1-
{
2-
"explain_todo": {
3-
"stmt1": true
4-
}
5-
}
1+
{}

‎parser/testdata/00309_formats/metadata.json‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1 @@
1-
{
2-
"explain_todo": {
3-
"stmt13": true,
4-
"stmt14": true
5-
}
6-
}
1+
{}

‎parser/testdata/00609_prewhere_and_default/metadata.json‎

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1 @@
1-
{
2-
"explain_todo": {
3-
"stmt10": true,
4-
"stmt11": true,
5-
"stmt12": true,
6-
"stmt18": true,
7-
"stmt19": true,
8-
"stmt20": true,
9-
"stmt21": true,
10-
"stmt22": true,
11-
"stmt23": true,
12-
"stmt24": true,
13-
"stmt6": true,
14-
"stmt7": true,
15-
"stmt8": true,
16-
"stmt9": true
17-
}
18-
}
1+
{}

‎parser/testdata/00765_sql_compatibility_aliases/metadata.json‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,7 @@
33
"stmt10": true,
44
"stmt18": true,
55
"stmt2": true,
6-
"stmt21": true,
7-
"stmt22": true,
86
"stmt23": true,
9-
"stmt24": true,
107
"stmt25": true,
118
"stmt26": true,
129
"stmt27": true,

‎parser/testdata/00996_limit_with_ties/metadata.json‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1 @@
1-
{
2-
"explain_todo": {
3-
"stmt31": true,
4-
"stmt32": true
5-
}
6-
}
1+
{}

‎parser/testdata/01051_scalar_optimization/metadata.json‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1 @@
1-
{
2-
"explain_todo": {
3-
"stmt1": true
4-
}
5-
}
1+
{}

‎parser/testdata/01055_compact_parts_1/metadata.json‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
{
22
"explain_todo": {
3-
"stmt1": true,
43
"stmt6": true
54
}
65
}

‎parser/testdata/01070_template_empty_file/metadata.json‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1 @@
1-
{
2-
"explain_todo": {
3-
"stmt1": true,
4-
"stmt2": true
5-
}
6-
}
1+
{}

0 commit comments

Comments
 (0)