Fix RANGE window frame panics and bugs - #3030
Conversation
033eeb7 to
208b92f
Compare
|
208b92f to
1e2be03
Compare
|
SummaryCoverage spans SQL windowed totals across numeric, date, timestamp, month-end, large-value, asymmetric-boundary, and combined-frame scenarios, including both normal results and edge-case boundary behavior. Most supported range and calendar calculations behave correctly, but combined numeric windows can silently produce incorrect totals. Merge with caution — this PR introduces a medium-severity correctness issue where multiple numeric window calculations in one statement return wrong results without an error. Separate pre-existing numeric-range crashes and unsupported prepared statements are important caveats but are not attributable to this PR. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Numeric RANGE windows crash during execution
Evidence Package🟡 Prepared queries reject interval windows
Evidence Package🟡 Numeric RANGE windows crash during execution
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
|
Diff SummaryCoverage spans SQL window and aggregate behavior across numeric, ranking, partitioning, ordering, row-based, and time-based ranges, including boundary, tie, repetition, calendar, leap-year, and dialect-validation cases. The application behavior is broadly healthy, with both ordinary usage and difficult edge conditions behaving as expected except for a narrow subsecond timestamp boundary issue. Safe to merge — the only failure is a high-severity correctness issue in microsecond timestamp windows, but it is explicitly unrelated to this PR and affects unchanged or dependency-level behavior. No PR-attributable regressions or previously flagged failures were found; the timestamp issue is a flag for later investigation rather than a merge blocker. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Microsecond window boundaries include wrong rows
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
|
Hydrocharged
left a comment
There was a problem hiding this comment.
LGTM! Not a fan of how we have to compare mismatched numbers, but it's just a limitation of GMS rather than an issue with the PR.
|
Diff SummaryCoverage spans core database behavior including windowed calculations, date and numeric boundary handling, custom types, extensions, persistence, concurrent updates, branch isolation, rollback safety, and invalid-input handling. It primarily exercises end-to-end SQL flows and business logic across happy paths, edge cases, error recovery, and concurrency scenarios, with some build and packaging checks. Safe to merge — no failures are attributable to this PR, and the change-related coverage shows no regressions. The unrelated findings are medium-severity pre-existing limitations in backup support and type merging, suitable for follow-up rather than merge blockers. Tests run by ItoAdditional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Database backup fails during export
Evidence Package🟡 Merge drops domain validation rules
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
…oundary offset is numeric or an INTERVAL over a DATE/TIMESTAMP column
…the same SELECT statement
… value outside its valid domain, and stop swallowing non-EOF framer errors that let it turn into an out-of-bounds index.
674f965 to
2bc8ad7
Compare
… value outside its valid domain, and stop swallowing non-EOF framer errors that let it turn into an out-of-bounds index.
Commit: SummaryCoverage spans windowed calculations over dates, timestamps, and numeric values, including ordering, partitions, frame boundaries, peer rows, precision, and recovery after invalid input. It exercises both normal business logic and adversarial type or syntax edge cases, with the tested behavior broadly healthy aside from one unsafe error path. Merge with caution — the PR still permits a medium-severity edge case to panic the server when a range comparison crosses incompatible value types, rather than returning a controlled error. The impact appears limited to the failing query without evidence of data corruption or broader query damage, but the issue is directly attributable to this change and remains relevant to its purpose. Tests run by ItoTip Reply with @itoqa to send us feedback on this test run. |
| return int(i.(int32)), nil | ||
| } | ||
|
|
||
| // A value crossing in from GMS's own generic machinery without going through DoltgresType.Convert |
There was a problem hiding this comment.
Numeric range errors can crash queries
What failed: A range query that crosses from a text order value to a numeric boundary crashes instead of returning a controlled error.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Users running a range query that compares text with a numeric value receive a server error instead of a clear type error. That query fails, but there is no evidence that other queries or stored data are affected.
- Steps to Reproduce:
- Create a table with a text column used as the window ORDER BY value and numeric rows or a numeric RANGE boundary.
- Run a window query whose frame compares the text order value with a numeric boundary, including the nonnumeric-to-numeric direction.
- Observe that the query returns SQLSTATE XX000 from an interface type assertion instead of a controlled typed error, and that the following health-check query does not run.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: server/types/type.go:306-312 adds a reflect.Type mismatch check and calls compareMismatchedNumeric only when numericAsFloat64(v1) succeeds. For a reverse mismatch such as v1 being string and v2 being float64, numericAsFloat64(v1) returns false, so execution falls through to the switch at line 314. The string case at lines 378-386 then performs v2.(string), which panics for a float64 value. The typed mismatch error in compareMismatchedNumeric at lines 213-217 and the later default error at lines 482-484 are not reached. The minimal correction is to route any mismatch where either operand is numeric through the helper, allowing its existing type check to return an error when the other operand is nonnumeric.
- Why this is likely a bug: The focused NUMERIC-4 run reported an SQLSTATE XX000 interface-conversion panic while evaluating DoltgresType.Compare, and the source has a deterministic path that produces exactly that panic for string v1 and float64 v2. The test could not complete its later controlled-mismatch and health checks because the query failed first, but this is not dependent on the missing local ICU header: the unsafe assertion is visible in production code. The PR is specifically intended to prevent RANGE numeric panics, and its new asymmetric guard does not cover this reverse crossing. Extending that guard to handle numeric v2 is a targeted fix; broad changes to window framing are not required.
Relevant code
server/types/type.go:306-312
if reflect.TypeOf(v1) != reflect.TypeOf(v2) {
if _, ok := numericAsFloat64(v1); ok {
return compareMismatchedNumeric(v1, v2)
}
}server/types/type.go:378-386
case string:
bb := v2.(string)
if ab == bb {
return 0, nil
} else if ab < bb {
return -1, nil
} else {
return 1, nil
}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 — Numeric range errors can crash queries**
**What failed:** A range query that crosses from a text order value to a numeric boundary crashes instead of returning a controlled error.
- **Impact:** Users running a range query that compares text with a numeric value receive a server error instead of a clear type error. That query fails, but there is no evidence that other queries or stored data are affected.
- **Steps to reproduce:**
1. Create a table with a text column used as the window ORDER BY value and numeric rows or a numeric RANGE boundary.
2. Run a window query whose frame compares the text order value with a numeric boundary, including the nonnumeric-to-numeric direction.
3. Observe that the query returns SQLSTATE XX000 from an interface type assertion instead of a controlled typed error, and that the following health-check query does not run.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** server/types/type.go:306-312 adds a reflect.Type mismatch check and calls compareMismatchedNumeric only when numericAsFloat64(v1) succeeds. For a reverse mismatch such as v1 being string and v2 being float64, numericAsFloat64(v1) returns false, so execution falls through to the switch at line 314. The string case at lines 378-386 then performs v2.(string), which panics for a float64 value. The typed mismatch error in compareMismatchedNumeric at lines 213-217 and the later default error at lines 482-484 are not reached. The minimal correction is to route any mismatch where either operand is numeric through the helper, allowing its existing type check to return an error when the other operand is nonnumeric.
- **Why this is likely a bug:** The focused NUMERIC-4 run reported an SQLSTATE XX000 interface-conversion panic while evaluating DoltgresType.Compare, and the source has a deterministic path that produces exactly that panic for string v1 and float64 v2. The test could not complete its later controlled-mismatch and health checks because the query failed first, but this is not dependent on the missing local ICU header: the unsafe assertion is visible in production code. The PR is specifically intended to prevent RANGE numeric panics, and its new asymmetric guard does not cover this reverse crossing. Extending that guard to handle numeric v2 is a targeted fix; broad changes to window framing are not required.
**Relevant code:**
`server/types/type.go:306-312`
~~~go
if reflect.TypeOf(v1) != reflect.TypeOf(v2) {
if _, ok := numericAsFloat64(v1); ok {
return compareMismatchedNumeric(v1, v2)
}
}
~~~
`server/types/type.go:378-386`
~~~go
case string:
bb := v2.(string)
if ab == bb {
return 0, nil
} else if ab < bb {
return -1, nil
} else {
return 1, nil
}
~~~


Fixes panics and silent mishandling when the frame boundary offset is numeric or an
INTERVALover aDATE/TIMESTAMPcolumn.Depends on: dolthub/go-mysql-server#3665