Fix expression String methods - #3803
Open
zachmu wants to merge 85 commits into
Open
Conversation
Member
Author
|
Companion DoltgreSQL PR: dolthub/doltgresql#3277 |
|
SummaryCoverage spans SQL expression generation and execution, prepared-value binding, aggregates and window calculations, subqueries, JSON and spatial functions, typed literals, and safe handling of adversarial values. The run exercises both normal workflows and edge cases, with most behaviors working but a valid grouped window calculation failing during execution. Merge with caution — the PR has a medium-severity, attributable execution failure affecting valid grouped queries that combine aggregation with window calculations, creating a correctness and availability risk for those users. A separate medium-severity binding concern is not attributable to this PR and is a flag for later rather than a merge driver. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Mixed query values are lost
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
Member
Author
|
@itoqa The attributable grouped-window failure is fixed in 457fd50 with an engine regression test. Please rerun against the current head. |
Member
Author
|
@itoqa The current head is 006422b; its only follow-up after the grouped-window fix scopes the MySQL-specific regression test to the MySQL dialect. CI is green. Please rerun against this head. |
|
Diff SummaryCoverage exercises grouped and ungrouped reporting calculations, running totals, ordering, filtering, null handling, repeated-query behavior, and result cleanup. It includes both normal report flows and edge-case or adversarial checks around query planning and resource lifecycle behavior. Merge with caution — a PR-attributable medium-severity failure causes valid grouped reports that combine a running total with row numbering to return an error, despite other grouped-window behaviors passing. This is a functional limitation in the changed reporting path and is more than a minor caveat. Tests run by Ito
Tests that are no longer relevantBelow are tests that previously ran and are no longer relevant:
Tip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
| @@ -95,6 +95,10 @@ func (b *Builder) buildSelect(inScope *scope, s *ast.Select) (outScope *scope) { | |||
| if b.needsAggregation(fromScope, s) { | |||
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Grouped queries reject multiple window expressions
What failed: The database returned an error for the query containing both the running total and row number. The running total query alone returned 15, 22, 3, and 7, and the row-number query alone returned 1 and 2 within each group, so the expected combined result should also have succeeded.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Applications that use grouped reports with both a running total and row numbers receive a database error instead of results. They must remove one calculation or split the query to complete the report.
- Steps to Reproduce:
- Create a table with grp, ord, and val columns and insert rows that form two groups with two ordinals in each group.
- Clear the session SQL mode and run a grouped query with SUM(SUM(val)) OVER (...) and ROW_NUMBER() OVER (...) in the same SELECT list.
- Check the result. The combined query returns ERROR 1140 instead of one row for each grouped (grp, ord) pair.
- Run each window expression in a separate grouped query. Each separate query returns the expected four rows, isolating the failure to the combination.
- Stub / mock content: The test used a local SQL fixture and cleared the session SQL mode as required for this engine regression. No mocks, route interception, or application bypasses were used.
- Code Analysis: The PR adds a grouped-window branch in sql/planbuilder/select.go:95-101. After buildAggregation creates the grouped scope at line 97, lines 98-101 copy fromScope.windowFuncs to the output scope and call buildWindow, which is the direct production path required by REV-10. The PR also changes sql/planbuilder/aggregates.go:205-245 to track window-function column IDs while collecting aggregation dependencies, and sql/planbuilder/aggregates.go:612-685 builds one plan.Window node from every entry in fromScope.windowFuncs and then adds projection aliases. The independent SUM(SUM(val)) and ROW_NUMBER() queries prove that each expression is supported alone; the deterministic ERROR 1140 only when both are present points to incorrect dependency or scope handling while the changed aggregation and multi-window lists are combined. The smallest practical fix is to correct the changed dependency transfer/projection construction so every window expression remains a window dependency and all required grouped columns are available to the single post-aggregation Window node, then retain the regression query with both expressions.
- Why this is likely a bug: This is a real engine failure, not a browser or setup failure. Port 3306 speaks the MySQL protocol, so the browser error was correctly bypassed with a local MySQL client; the client connected, created the fixture, and produced valid control results. Clearing SQL mode removed the initial only_full_group_by setup condition, but the combined query continued to return ERROR 1140 while each expression worked independently. The failing input exactly targets the PR's new aggregation-then-window path and its multiple-window collection. A normal grouped report should be able to add a ranking column without making an otherwise valid running aggregate fail, so the changed planner path needs a targeted correction rather than a test-only workaround.
Relevant code
sql/planbuilder/select.go:95-101
if b.needsAggregation(fromScope, s) {
groupingCols := b.buildGroupingCols(fromScope, projScope, s.GroupBy, s.SelectExprs)
outScope = b.buildAggregation(fromScope, projScope, groupingCols)
if len(fromScope.windowFuncs) > 0 {
outScope.windowFuncs = fromScope.windowFuncs
outScope = b.buildWindow(outScope, projScope)
}
}sql/planbuilder/aggregates.go:205-245
windowIds := make(map[sql.ColumnId]struct{}, len(fromScope.windowFuncs))
for _, col := range fromScope.windowFuncs {
windowIds[sql.ColumnId(col.id)] = struct{}{}
}
...
case *expression.GetField:
if _, ok := windowIds[e.Id()]; ok {
return false
}sql/planbuilder/aggregates.go:612-685
func (b *Builder) buildWindow(fromScope, projScope *scope) *scope {
if len(fromScope.windowFuncs) == 0 {
return fromScope
}
...
for _, col := range fromScope.windowFuncs {
...
selectExprs = append(selectExprs, e)
}
window := plan.NewWindow(selectExprs, fromScope.node)
fromScope.node = window
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Grouped queries reject multiple window expressions**
**What failed:** The database returned an error for the query containing both the running total and row number. The running total query alone returned 15, 22, 3, and 7, and the row-number query alone returned 1 and 2 within each group, so the expected combined result should also have succeeded.
- **Impact:** Applications that use grouped reports with both a running total and row numbers receive a database error instead of results. They must remove one calculation or split the query to complete the report.
- **Steps to reproduce:**
1. Create a table with grp, ord, and val columns and insert rows that form two groups with two ordinals in each group.
2. Clear the session SQL mode and run a grouped query with SUM(SUM(val)) OVER (...) and ROW_NUMBER() OVER (...) in the same SELECT list.
3. Check the result. The combined query returns ERROR 1140 instead of one row for each grouped (grp, ord) pair.
4. Run each window expression in a separate grouped query. Each separate query returns the expected four rows, isolating the failure to the combination.
- **Stub / mock content:** The test used a local SQL fixture and cleared the session SQL mode as required for this engine regression. No mocks, route interception, or application bypasses were used.
- **Code analysis:** The PR adds a grouped-window branch in sql/planbuilder/select.go:95-101. After buildAggregation creates the grouped scope at line 97, lines 98-101 copy fromScope.windowFuncs to the output scope and call buildWindow, which is the direct production path required by REV-10. The PR also changes sql/planbuilder/aggregates.go:205-245 to track window-function column IDs while collecting aggregation dependencies, and sql/planbuilder/aggregates.go:612-685 builds one plan.Window node from every entry in fromScope.windowFuncs and then adds projection aliases. The independent SUM(SUM(val)) and ROW_NUMBER() queries prove that each expression is supported alone; the deterministic ERROR 1140 only when both are present points to incorrect dependency or scope handling while the changed aggregation and multi-window lists are combined. The smallest practical fix is to correct the changed dependency transfer/projection construction so every window expression remains a window dependency and all required grouped columns are available to the single post-aggregation Window node, then retain the regression query with both expressions.
- **Why this is likely a bug:** This is a real engine failure, not a browser or setup failure. Port 3306 speaks the MySQL protocol, so the browser error was correctly bypassed with a local MySQL client; the client connected, created the fixture, and produced valid control results. Clearing SQL mode removed the initial only_full_group_by setup condition, but the combined query continued to return ERROR 1140 while each expression worked independently. The failing input exactly targets the PR's new aggregation-then-window path and its multiple-window collection. A normal grouped report should be able to add a ranking column without making an otherwise valid running aggregate fail, so the changed planner path needs a targeted correction rather than a test-only workaround.
**Relevant code:**
`sql/planbuilder/select.go:95-101`
~~~go
if b.needsAggregation(fromScope, s) {
groupingCols := b.buildGroupingCols(fromScope, projScope, s.GroupBy, s.SelectExprs)
outScope = b.buildAggregation(fromScope, projScope, groupingCols)
if len(fromScope.windowFuncs) > 0 {
outScope.windowFuncs = fromScope.windowFuncs
outScope = b.buildWindow(outScope, projScope)
}
}
~~~
`sql/planbuilder/aggregates.go:205-245`
~~~go
windowIds := make(map[sql.ColumnId]struct{}, len(fromScope.windowFuncs))
for _, col := range fromScope.windowFuncs {
windowIds[sql.ColumnId(col.id)] = struct{}{}
}
...
case *expression.GetField:
if _, ok := windowIds[e.Id()]; ok {
return false
}
~~~
`sql/planbuilder/aggregates.go:612-685`
~~~go
func (b *Builder) buildWindow(fromScope, projScope *scope) *scope {
if len(fromScope.windowFuncs) == 0 {
return fromScope
}
...
for _, col := range fromScope.windowFuncs {
...
selectExprs = append(selectExprs, e)
}
window := plan.NewWindow(selectExprs, fromScope.node)
fromScope.node = window
}
~~~
Member
Author
There was a problem hiding this comment.
@itoqa Fixed in 110dc09. Explicit GROUP BY state is now preserved during projection analysis, allowing multiple grouped window expressions without relaxing the existing implicit-grouping validation. The engine regression covers the reported query and reversed SELECT-list order; go test ./... passes. Please rerun against the current head.
|
Diff SummaryCoverage spans normal grouped and ungrouped running calculations, ordering and projection variations, null and empty inputs, and rejection of invalid aggregate/window combinations. These tests exercise core query behavior along with edge cases and planner-boundary conditions, with most covered behavior remaining healthy. Merge with caution — this PR introduces a medium-severity regression where filtered grouped results can produce incorrect running totals, causing users to see wrong query results. The issue is limited to queries combining grouped calculations, filtering, and running totals, but it is directly attributable to the change and remains a merge risk. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
| // projections from (4). | ||
| // 6) Finish with final target projections. | ||
| fromScope := b.buildFrom(inScope, s.From) | ||
| fromScope.explicitGrouping = len(s.GroupBy) > 0 |
There was a problem hiding this comment.
🔁 Regression: previously passing at 006422b
Filtered groups change running totals incorrectly
What failed: The running total still includes a grouped row that HAVING removed. Group 2 is returned with 7 instead of the expected 4.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Queries that combine grouped results, HAVING, and a running total can show incorrect totals. Users can avoid the error by filtering the grouped results before calculating the running total.
- Steps to Reproduce:
- Create grouped rows with two groups, and make the HAVING condition remove the first ordered row from group 2.
- Run a grouped query that calculates a running window total and applies HAVING to the grouped result.
- Compare it with a reference query that materializes the grouped rows and applies HAVING before the window calculation.
- Observe that the direct query reports group 2 running_total as 7, while the filtered reference reports 4.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: Builder.buildSelect in sql/planbuilder/select.go records the query's GROUP BY state at line 70, builds grouped aggregation at lines 96-98, and then constructs the Window node at lines 99-102. It does not build the Having node until line 114, after the window stage has already consumed the grouped rows. The test comparison confirms the consequence: the direct query returns group 2's running_total as 7, while the materialized reference, where the HAVING-filtered grouped rows are the window input, returns 4. The PR's sql/planbuilder/aggregates.go change at lines 303-306 relaxes the aggregate/window rejection for explicit GROUP BY queries, and select.go line 70 supplies that new state, making this previously rejected combination executable. The smallest practical fix is to place the HAVING filter on the grouped relation before constructing the window stage, while preserving the existing final projection and ORDER BY handling; add a regression assertion for a HAVING-filtered grouped window result.
- Why this is likely a bug: This is a deterministic semantic mismatch, not a browser or setup failure: the same fixture produces 7 from the direct query and 4 from the HAVING-filtered materialized reference. SQL users expect a window over a grouped query to use the rows that remain after HAVING for this test's documented stage ordering. The planner source independently confirms that the Window node is built before the Having node, which explains the stale contribution and gives a focused fix location. The browser could not render localhost:3306 because it is a MySQL protocol endpoint, but the local SQL client comparison and source path establish the defect without relying on production access.
Relevant code
sql/planbuilder/select.go:70
fromScope.explicitGrouping = len(s.GroupBy) > 0sql/planbuilder/select.go:96-116
outScope = b.buildAggregation(fromScope, projScope, groupingCols)
if len(fromScope.windowFuncs) > 0 {
outScope.windowFuncs = fromScope.windowFuncs
outScope = b.buildWindow(outScope, projScope)
}
...
b.buildHaving(fromScope, projScope, outScope, s.Having)
b.buildOrderBy(outScope, orderByScope)sql/planbuilder/aggregates.go:303-306
if len(inScope.windowFuncs) > 0 && !inScope.explicitGrouping {
err := sql.ErrNonAggregatedColumnWithoutGroupBy.New()
b.handleErr(err)
}sql/plan/having.go:32-34
func NewHaving(cond sql.Expression, child sql.Node) *Having {
return &Having{UnaryNode{Child: child}, cond}
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Filtered groups change running totals incorrectly**
**What failed:** The running total still includes a grouped row that HAVING removed. Group 2 is returned with 7 instead of the expected 4.
- **Impact:** Queries that combine grouped results, HAVING, and a running total can show incorrect totals. Users can avoid the error by filtering the grouped results before calculating the running total.
- **Steps to reproduce:**
1. Create grouped rows with two groups, and make the HAVING condition remove the first ordered row from group 2.
2. Run a grouped query that calculates a running window total and applies HAVING to the grouped result.
3. Compare it with a reference query that materializes the grouped rows and applies HAVING before the window calculation.
4. Observe that the direct query reports group 2 running_total as 7, while the filtered reference reports 4.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** Builder.buildSelect in sql/planbuilder/select.go records the query's GROUP BY state at line 70, builds grouped aggregation at lines 96-98, and then constructs the Window node at lines 99-102. It does not build the Having node until line 114, after the window stage has already consumed the grouped rows. The test comparison confirms the consequence: the direct query returns group 2's running_total as 7, while the materialized reference, where the HAVING-filtered grouped rows are the window input, returns 4. The PR's sql/planbuilder/aggregates.go change at lines 303-306 relaxes the aggregate/window rejection for explicit GROUP BY queries, and select.go line 70 supplies that new state, making this previously rejected combination executable. The smallest practical fix is to place the HAVING filter on the grouped relation before constructing the window stage, while preserving the existing final projection and ORDER BY handling; add a regression assertion for a HAVING-filtered grouped window result.
- **Why this is likely a bug:** This is a deterministic semantic mismatch, not a browser or setup failure: the same fixture produces 7 from the direct query and 4 from the HAVING-filtered materialized reference. SQL users expect a window over a grouped query to use the rows that remain after HAVING for this test's documented stage ordering. The planner source independently confirms that the Window node is built before the Having node, which explains the stale contribution and gives a focused fix location. The browser could not render localhost:3306 because it is a MySQL protocol endpoint, but the local SQL client comparison and source path establish the defect without relying on production access.
**Relevant code:**
`sql/planbuilder/select.go:70`
~~~go
fromScope.explicitGrouping = len(s.GroupBy) > 0
~~~
`sql/planbuilder/select.go:96-116`
~~~go
outScope = b.buildAggregation(fromScope, projScope, groupingCols)
if len(fromScope.windowFuncs) > 0 {
outScope.windowFuncs = fromScope.windowFuncs
outScope = b.buildWindow(outScope, projScope)
}
...
b.buildHaving(fromScope, projScope, outScope, s.Having)
b.buildOrderBy(outScope, orderByScope)
~~~
`sql/planbuilder/aggregates.go:303-306`
~~~go
if len(inScope.windowFuncs) > 0 && !inScope.explicitGrouping {
err := sql.ErrNonAggregatedColumnWithoutGroupBy.New()
b.handleErr(err)
}
~~~
`sql/plan/having.go:32-34`
~~~go
func NewHaving(cond sql.Expression, child sql.Node) *Having {
return &Having{UnaryNode{Child: child}, cond}
}
~~~
Member
Author
There was a problem hiding this comment.
@itoqa Fixed in 6e61c15. Grouped-query HAVING now filters the aggregation result before the Window node is built, and the regression verifies that a removed group no longer contributes to the running total. The full GMS suite passes; please rerun against the current head.
|
Diff SummaryCoverage exercised core SQL behavior for grouped and ungrouped running totals, filtering grouped results, partition boundaries, null and empty-result handling, and validation of invalid aggregate/window combinations. It also covered expression round-tripping and keeping readable query text separate from internal plan details, including nested expressions and edge-case predicate behavior. Safe to merge — the exercised behavior passed without any PR-attributable regressions, new failures, or previously failing tests that remain unresolved. The untested cases were previously passing and are a coverage caveat rather than a merge blocker. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
Member
Author
|
@itoqa Unary aggregate Describe implementations, projected-field debug descriptions, and stronger String round-trip assertions are now in 3ff64aa. Please rerun against the current head. |
…on-string # Conflicts: # enginetest/queries/window_functions_queries.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
expr.String() is load-bearing in several contexts, and was implemented incorrectly for many expressions. This PR audits all expression types and fixes those problems.
This PR also implements Describe() for many types that were missing it, which is responsible for the changes in query plan test expectations.
Fixes SQL generation for expression String methods with companion plan updates in dolthub/doltgresql#3277.