Improved window function support#2913
Conversation
|
|
29de547 to
bd381c2
Compare
7f9d90b to
070f57e
Compare
|
SummaryThis run exercised SQL query correctness across aggregate and window-function behavior, covering standard analytics flows plus edge-case semantics like empty frames, wrappers, and startup timing boundaries. Core aggregate and ranking paths look stable, but behavior around named window references and inheritance is not reliable. Not safe to merge yet — this PR is tied to multiple high-severity correctness failures in window-query behavior that can silently return wrong analytical results. There is also a separate non-attributable medium issue to track, but the merge-blocking risk comes from the attributable high-severity failures. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Undefined named window reference is accepted as a valid query
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
| @@ -13630,7 +13630,7 @@ over_clause: | |||
| } | |||
| | OVER window_name | |||
There was a problem hiding this comment.
Named window reference via OVER w resolves correctly
What failed: Named window reference OVER w produces a full-partition SUM per row instead of a running SUM within each partition, even though inline window syntax behaves correctly.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: Queries that rely on named window ORDER BY semantics can return silently incorrect running totals, which can mislead users making decisions from query results.
- Steps to Reproduce:
- Create table t_named(id int, grp int, amt int) and insert (1,1,10),(2,1,20),(3,2,5).
- Run SELECT id, SUM(amt) OVER w AS s FROM t_named WINDOW w AS (PARTITION BY grp ORDER BY id) ORDER BY id;.
- Observe id=1 returns 30 instead of 10; compare with inline OVER (PARTITION BY grp ORDER BY id) which returns 10,30,5.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: postgres/parser/parser/sql.y now emits RefName for OVER window_name, so named windows flow through reference merge behavior. The PR also updates go-mysql-server in go.mod, and that dependency path applies a default unbounded frame too early in buildWindowDef, so after merge the inherited ORDER BY still uses a full-partition frame for SUM. The smallest practical fix is to make default frame selection happen only after resolving inherited ORDER BY and frame from the referenced window, or temporarily pin/revert the dependency bump until that fix is available.
- Why this is likely a bug: The same PARTITION BY and ORDER BY returns correct results with inline OVER syntax, while named-window SUM returns a silently wrong value for the first row; this isolates the defect to named-window reference resolution rather than test setup. Returning incorrect aggregate results for valid SQL is a production-code correctness bug.
Relevant code
postgres/parser/parser/sql.y:13631-13634
| OVER window_name\n {\n $$.val = &tree.WindowDef{RefName: tree.Name($2)}\n }go.mod:12
github.com/dolthub/go-mysql-server v0.20.1-0.20260713191803-1e74a51e8ddaEvidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — Named window reference via OVER w resolves correctly**
**What failed:** Named window reference OVER w produces a full-partition SUM per row instead of a running SUM within each partition, even though inline window syntax behaves correctly.
- **Impact:** Queries that rely on named window ORDER BY semantics can return silently incorrect running totals, which can mislead users making decisions from query results.
- **Steps to reproduce:**
1. Create table t_named(id int, grp int, amt int) and insert (1,1,10),(2,1,20),(3,2,5).
2. Run SELECT id, SUM(amt) OVER w AS s FROM t_named WINDOW w AS (PARTITION BY grp ORDER BY id) ORDER BY id;.
3. Observe id=1 returns 30 instead of 10; compare with inline OVER (PARTITION BY grp ORDER BY id) which returns 10,30,5.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** postgres/parser/parser/sql.y now emits RefName for OVER window_name, so named windows flow through reference merge behavior. The PR also updates go-mysql-server in go.mod, and that dependency path applies a default unbounded frame too early in buildWindowDef, so after merge the inherited ORDER BY still uses a full-partition frame for SUM. The smallest practical fix is to make default frame selection happen only after resolving inherited ORDER BY and frame from the referenced window, or temporarily pin/revert the dependency bump until that fix is available.
- **Why this is likely a bug:** The same PARTITION BY and ORDER BY returns correct results with inline OVER syntax, while named-window SUM returns a silently wrong value for the first row; this isolates the defect to named-window reference resolution rather than test setup. Returning incorrect aggregate results for valid SQL is a production-code correctness bug.
**Relevant code:**
`postgres/parser/parser/sql.y:13631-13634`
~~~yacc
| OVER window_name\n {\n $$.val = &tree.WindowDef{RefName: tree.Name($2)}\n }
~~~
`go.mod:12`
~~~go
github.com/dolthub/go-mysql-server v0.20.1-0.20260713191803-1e74a51e8dda
~~~| @@ -9,7 +9,7 @@ require ( | |||
| github.com/dolthub/dolt/go v0.40.5-0.20260710173237-7bdcf6ee8148 | |||
| github.com/dolthub/eventsapi_schema v0.0.0-20260310172945-37a9265ade69 | |||
| github.com/dolthub/flatbuffers/v23 v23.3.3-dh.2 | |||
There was a problem hiding this comment.
Named window inheritance drops ORDER BY semantics
What failed: Named-window reference resolution loses ORDER BY/frame behavior for aggregate windows, so inherited and override forms behave like full-partition windows instead of ordered running windows.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: Queries that use named window references can produce silently incorrect rankings and aggregates because ORDER BY is dropped during resolution, causing users to make decisions from wrong analytical results.
- Steps to Reproduce:
- Create
t_inherit(id, grp, amt)and insert rows(1,1,10),(2,1,20),(3,1,30),(4,2,5),(5,2,15). - Run
SELECT id, SUM(amt) OVER w2 AS s FROM t_inherit WINDOW w1 AS (PARTITION BY grp), w2 AS (w1 ORDER BY id) ORDER BY id;and observe full-partition sums (60,60,60,20,20). - Run
SELECT id, SUM(amt) OVER (PARTITION BY grp ORDER BY id) AS s FROM t_inherit ORDER BY id;and observe correct running sums (10,30,60,5,20).
- Create
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The PR updates the engine dependency in
go.mod(line 12) togithub.com/dolthub/go-mysql-server v0.20.1-0.20260713191803-1e74a51e8dda. Runtime evidence isolates the defect to named-window resolution: inlineOVER (PARTITION BY grp ORDER BY id)is correct, butOVER w2andOVER (w1 ORDER BY id)both collapse to partition totals. The parser/AST bridge in this repo maps named references throughRefName(postgres/parser/parser/sql.yandserver/ast/window.go), so resolution reaches the planner; the incorrect aggregate behavior is consistent with the changed dependency's named-window merge path keeping an unbounded frame for these reference forms. Smallest practical fix: patch or pin the dependency so named-window merge preserves ORDER BY/frame semantics for inherited/override references, or add a guard in the planning path to avoid promoting these references to an unbounded full-partition frame. - Why this is likely a bug: Equivalent inline and named window forms should be semantically consistent for SUM OVER with the same PARTITION/ORDER specification, but only named-reference forms return incorrect full-partition totals. This is deterministic on the same dataset and points to a planner merge defect rather than test setup noise.
Relevant code
go.mod:11-13
github.com/dolthub/flatbuffers/v23 v23.3.3-dh.2
github.com/dolthub/go-mysql-server v0.20.1-0.20260713191803-1e74a51e8dda
github.com/dolthub/pg_query_go/v6 v6.0.0-20251215122834-fb20be4254d1
postgres/parser/parser/sql.y:13631-13634
| OVER window_name
{
$$.val = &tree.WindowDef{RefName: tree.Name($2)}
}server/ast/window.go:104-107
return &vitess.WindowDef{
Name: vitess.NewColIdent(string(node.Name)),
NameRef: vitess.NewColIdent(string(node.RefName)),
PartitionBy: partitionBy,Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — Named window inheritance drops ORDER BY semantics**
**What failed:** Named-window reference resolution loses ORDER BY/frame behavior for aggregate windows, so inherited and override forms behave like full-partition windows instead of ordered running windows.
- **Impact:** Queries that use named window references can produce silently incorrect rankings and aggregates because ORDER BY is dropped during resolution, causing users to make decisions from wrong analytical results.
- **Steps to reproduce:**
1. Create `t_inherit(id, grp, amt)` and insert rows `(1,1,10),(2,1,20),(3,1,30),(4,2,5),(5,2,15)`.
2. Run `SELECT id, SUM(amt) OVER w2 AS s FROM t_inherit WINDOW w1 AS (PARTITION BY grp), w2 AS (w1 ORDER BY id) ORDER BY id;` and observe full-partition sums (`60,60,60,20,20`).
3. Run `SELECT id, SUM(amt) OVER (PARTITION BY grp ORDER BY id) AS s FROM t_inherit ORDER BY id;` and observe correct running sums (`10,30,60,5,20`).
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The PR updates the engine dependency in `go.mod` (line 12) to `github.com/dolthub/go-mysql-server v0.20.1-0.20260713191803-1e74a51e8dda`. Runtime evidence isolates the defect to named-window resolution: inline `OVER (PARTITION BY grp ORDER BY id)` is correct, but `OVER w2` and `OVER (w1 ORDER BY id)` both collapse to partition totals. The parser/AST bridge in this repo maps named references through `RefName` (`postgres/parser/parser/sql.y` and `server/ast/window.go`), so resolution reaches the planner; the incorrect aggregate behavior is consistent with the changed dependency's named-window merge path keeping an unbounded frame for these reference forms. Smallest practical fix: patch or pin the dependency so named-window merge preserves ORDER BY/frame semantics for inherited/override references, or add a guard in the planning path to avoid promoting these references to an unbounded full-partition frame.
- **Why this is likely a bug:** Equivalent inline and named window forms should be semantically consistent for SUM OVER with the same PARTITION/ORDER specification, but only named-reference forms return incorrect full-partition totals. This is deterministic on the same dataset and points to a planner merge defect rather than test setup noise.
**Relevant code:**
`go.mod:11-13`
~~~gomod
github.com/dolthub/flatbuffers/v23 v23.3.3-dh.2
github.com/dolthub/go-mysql-server v0.20.1-0.20260713191803-1e74a51e8dda
github.com/dolthub/pg_query_go/v6 v6.0.0-20251215122834-fb20be4254d1
~~~
`postgres/parser/parser/sql.y:13631-13634`
~~~yacc
| OVER window_name
{
$$.val = &tree.WindowDef{RefName: tree.Name($2)}
}
~~~
`server/ast/window.go:104-107`
~~~go
return &vitess.WindowDef{
Name: vitess.NewColIdent(string(node.Name)),
NameRef: vitess.NewColIdent(string(node.RefName)),
PartitionBy: partitionBy,
~~~
Implements several window functions in the doltgres layer, instead of relying on the implementations in go-mysql-server. This allows us to cleanly match PostgreSQL's behavior and exact return types. Also enables more engine tests for window functions.
Depends on: dolthub/go-mysql-server#3614