optimize cast logic#2892
Open
jycor wants to merge 5 commits into
Open
Conversation
Contributor
|
|
SummaryThis run exercised SQL behavior around custom type conversion setup and lookup, including normal coercion paths plus long-identifier edge cases and persistence round-trips, and also checked function overload resolution behavior. Overall health is mixed: overload selection remains stable, but the cast-related paths show regressions where created conversions do not reliably remain usable. Not safe to merge yet — this PR appears to introduce multiple medium-severity regressions in the same changed area, breaking core custom-cast behavior after creation and under long-name conditions. Because these are functional correctness issues in user-facing conversion workflows rather than isolated pre-existing noise, merge risk is high until the encoding/lookup consistency is fixed. Tests run by Ito
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}
| } | ||
| return Cast(NewId(Section_Cast, string(sourceType), string(targetType))) | ||
|
|
||
| return Cast( |
There was a problem hiding this comment.
User-defined cast cannot be resolved after successful CREATE CAST
What failed: The cast registration write path acknowledges success, but subsequent cast retrieval paths do not resolve that same cast identity for execution/catalog access.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Users cannot reliably use user-defined casts after creation because the cast is not discoverable by lookup or coercion paths. Workflows that depend on custom type coercion fail until the ID parity bug is fixed.
- Steps to Reproduce:
- Create source and target user-defined types and a conversion function.
- Run CREATE CAST (source AS target) WITH FUNCTION ... AS ASSIGNMENT and confirm it returns success.
- Attempt coercion with
SELECT ROW(42)::enc1_x::enc1_y;and observecast does not exist. - Query pg_catalog.pg_cast for the created cast and observe no matching discoverable cast entry for the new pair.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis:
NewCastnow bypassesNewIdand manually emits first-format bytes withuint8(len(...))incore/id/id_wrappers.golines 94-104. The cast system uses this encoded key for persistence and lookup (server/node/create_cast.goline 86 andcore/casts/collection.golines 422-427), while catalog/runtime consumers decode source/target via segment extraction (server/tables/pgcatalog/pg_cast.golines 117-119 andcore/id/id_wrappers.golines 258-264). The changed encoding path is therefore a direct practical cause of the observed write/read mismatch for cast discoverability. - Why this is likely a bug: The same source/target pair succeeds during CREATE CAST but is immediately unresolved during coercion, which indicates an internal cast ID consistency defect rather than expected SQL behavior. The failure aligns with the PR's changed cast-ID encoding path and impacts core cast retrieval semantics.
Relevant code
core/id/id_wrappers.go:94-104
return Cast(
string(
[]byte{
uint8(Section_Cast),
2,
uint8(len(sourceType)),
uint8(len(targetType)),
}) +
string(sourceType) +
string(targetType),
)server/node/create_cast.go:85-87
newCast := casts.Cast{
ID: id.NewCast(c.Source.ID, c.Target.ID),
CastType: c.Type,
}server/tables/pgcatalog/pg_cast.go:117-119
return sql.Row{
cast.ID.AsId(),
cast.ID.SourceType().AsId(),
cast.ID.TargetType().AsId(),
...
}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 — User-defined cast cannot be resolved after successful CREATE CAST**
**What failed:** The cast registration write path acknowledges success, but subsequent cast retrieval paths do not resolve that same cast identity for execution/catalog access.
- **Impact:** Users cannot reliably use user-defined casts after creation because the cast is not discoverable by lookup or coercion paths. Workflows that depend on custom type coercion fail until the ID parity bug is fixed.
- **Steps to reproduce:**
1. Create source and target user-defined types and a conversion function.
2. Run CREATE CAST (source AS target) WITH FUNCTION ... AS ASSIGNMENT and confirm it returns success.
3. Attempt coercion with `SELECT ROW(42)::enc1_x::enc1_y;` and observe `cast does not exist`.
4. Query pg_catalog.pg_cast for the created cast and observe no matching discoverable cast entry for the new pair.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** `NewCast` now bypasses `NewId` and manually emits first-format bytes with `uint8(len(...))` in `core/id/id_wrappers.go` lines 94-104. The cast system uses this encoded key for persistence and lookup (`server/node/create_cast.go` line 86 and `core/casts/collection.go` lines 422-427), while catalog/runtime consumers decode source/target via segment extraction (`server/tables/pgcatalog/pg_cast.go` lines 117-119 and `core/id/id_wrappers.go` lines 258-264). The changed encoding path is therefore a direct practical cause of the observed write/read mismatch for cast discoverability.
- **Why this is likely a bug:** The same source/target pair succeeds during CREATE CAST but is immediately unresolved during coercion, which indicates an internal cast ID consistency defect rather than expected SQL behavior. The failure aligns with the PR's changed cast-ID encoding path and impacts core cast retrieval semantics.
**Relevant code:**
`core/id/id_wrappers.go:94-104`
~~~go
return Cast(
string(
[]byte{
uint8(Section_Cast),
2,
uint8(len(sourceType)),
uint8(len(targetType)),
}) +
string(sourceType) +
string(targetType),
)
~~~
`server/node/create_cast.go:85-87`
~~~go
newCast := casts.Cast{
ID: id.NewCast(c.Source.ID, c.Target.ID),
CastType: c.Type,
}
~~~
`server/tables/pgcatalog/pg_cast.go:117-119`
~~~go
return sql.Row{
cast.ID.AsId(),
cast.ID.SourceType().AsId(),
cast.ID.TargetType().AsId(),
...
}
~~~
| } | ||
| return Cast(NewId(Section_Cast, string(sourceType), string(targetType))) | ||
|
|
||
| return Cast( |
There was a problem hiding this comment.
Cast ID encoding breaks long type identifier handling
What failed: Cast IDs are encoded with fixed one-byte length fields in NewCast, so long-segment IDs can overflow and break lookup/catalog identity consistency.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Users cannot rely on custom cast definitions because CREATE CAST appears to succeed but casts are missing from the catalog and runtime conversions fail.
- Steps to Reproduce:
- Create source and target types with long names so their internal type IDs can exceed one-byte segment limits.
- Create a cast between those types with
CREATE CAST ... WITH FUNCTION .... - Query
pg_catalog.pg_castand execute a coercion using that cast. - Observe cast identity/lookup mismatch (missing catalog entry or cast-not-found behavior) when ID decoding does not round-trip.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: In
core/id/id_wrappers.go,NewCastnow manually builds first-format ID bytes and stores segment lengths asuint8. Incore/id/id.go,NewIdexplicitly switches to a second format when any segment length is greater than 255. The PR change bypasses that safeguard specifically for cast IDs, creating a direct defect path for long source/target type IDs. - Why this is likely a bug: The shared ID constructor already defines the required fallback for segments above 255 bytes, but
NewCastno longer uses it and can emit truncated length metadata. The smallest practical fix is to restoreNewCasttoNewId(Section_Cast, string(sourceType), string(targetType))or mirror the same second-format fallback before writing one-byte lengths.
Relevant code
core/id/id_wrappers.go:94-104
return Cast(
string(
[]byte{
uint8(Section_Cast),
2,
uint8(len(sourceType)),
uint8(len(targetType)),
}) +
string(sourceType) +
string(targetType),
)core/id/id.go:109-113
segmentLength := len(segment)
if segmentLength > 255 {
return newIdSecondFormat(section, data)
}
buf.WriteByte(uint8(segmentLength))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 — Cast ID encoding breaks long type identifier handling**
**What failed:** Cast IDs are encoded with fixed one-byte length fields in `NewCast`, so long-segment IDs can overflow and break lookup/catalog identity consistency.
- **Impact:** Users cannot rely on custom cast definitions because CREATE CAST appears to succeed but casts are missing from the catalog and runtime conversions fail.
- **Steps to reproduce:**
1. Create source and target types with long names so their internal type IDs can exceed one-byte segment limits.
2. Create a cast between those types with `CREATE CAST ... WITH FUNCTION ...`.
3. Query `pg_catalog.pg_cast` and execute a coercion using that cast.
4. Observe cast identity/lookup mismatch (missing catalog entry or cast-not-found behavior) when ID decoding does not round-trip.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** In `core/id/id_wrappers.go`, `NewCast` now manually builds first-format ID bytes and stores segment lengths as `uint8`. In `core/id/id.go`, `NewId` explicitly switches to a second format when any segment length is greater than 255. The PR change bypasses that safeguard specifically for cast IDs, creating a direct defect path for long source/target type IDs.
- **Why this is likely a bug:** The shared ID constructor already defines the required fallback for segments above 255 bytes, but `NewCast` no longer uses it and can emit truncated length metadata. The smallest practical fix is to restore `NewCast` to `NewId(Section_Cast, string(sourceType), string(targetType))` or mirror the same second-format fallback before writing one-byte lengths.
**Relevant code:**
`core/id/id_wrappers.go:94-104`
~~~go
return Cast(
string(
[]byte{
uint8(Section_Cast),
2,
uint8(len(sourceType)),
uint8(len(targetType)),
}) +
string(sourceType) +
string(targetType),
)
~~~
`core/id/id.go:109-113`
~~~go
segmentLength := len(segment)
if segmentLength > 255 {
return newIdSecondFormat(section, data)
}
buf.WriteByte(uint8(segmentLength))
~~~
| } | ||
| return Cast(NewId(Section_Cast, string(sourceType), string(targetType))) | ||
|
|
||
| return Cast( |
There was a problem hiding this comment.
Long type cast IDs break lookup
What failed: Expected long-identifier casts to be uniquely encoded and resolvable before and after restart, but casts with type IDs above 255 bytes fail at creation because decoded source/target identity is truncated.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Minor
- Impact: Users who define very long type identifiers (over 255 bytes) cannot create casts because lookup resolves to an invalid identifier. Typical schemas with shorter identifiers continue to work.
- Steps to Reproduce:
- Create two types where one generated type ID exceeds 255 bytes and attempt CREATE CAST from int4 to that type.
- Observe CREATE CAST fails with
root returned table(pg_catalog)|(int4)|()|()but it could not be found. - Create a cast to a similar type whose ID is below 256 bytes and observe it succeeds, then restart and retry the long-ID cast to confirm the same failure persists.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: In
core/id/id_wrappers.go,NewCastnow manually writes segment lengths withuint8(len(sourceType))anduint8(len(targetType)), which overflows for IDs >255 bytes.core/id/id.goshowsNewIdwould otherwise switch tonewIdSecondFormatwhen any segment exceeds 255, butNewCastbypasses that guard.core/casts/collection.gobuilds lookup table names fromcastID.SourceType()/TargetType(), so truncated decode yields the observed(pg_catalog)|(int4)|()|()lookup miss. The failing path is directly in PR-changed code. - Why this is likely a bug: Runtime evidence consistently fails only when IDs cross the 255-byte boundary, and source inspection shows deterministic uint8 truncation in the changed encoding path. A targeted fix is to route
NewCastback throughNewId(or equivalent overflow-safe encoding) so long segments use the second format.
Relevant code
core/id/id_wrappers.go:94-103
return Cast(string([]byte{uint8(Section_Cast), 2, uint8(len(sourceType)), uint8(len(targetType))}) + string(sourceType) + string(targetType))core/id/id.go:102-113
if len(data) > 255 { return newIdSecondFormat(section, data) } ... if segmentLength > 255 { return newIdSecondFormat(section, data) }core/casts/collection.go:701-706
name := fmt.Sprintf(`(%s)|(%s)|(%s)|(%s)`, castID.SourceType().SchemaName(), castID.SourceType().TypeName(), castID.TargetType().SchemaName(), castID.TargetType().TypeName())Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Minor severity — Long type cast IDs break lookup**
**What failed:** Expected long-identifier casts to be uniquely encoded and resolvable before and after restart, but casts with type IDs above 255 bytes fail at creation because decoded source/target identity is truncated.
- **Impact:** Users who define very long type identifiers (over 255 bytes) cannot create casts because lookup resolves to an invalid identifier. Typical schemas with shorter identifiers continue to work.
- **Steps to reproduce:**
1. Create two types where one generated type ID exceeds 255 bytes and attempt CREATE CAST from int4 to that type.
2. Observe CREATE CAST fails with `root returned table `(pg_catalog)|(int4)|()|()` but it could not be found`.
3. Create a cast to a similar type whose ID is below 256 bytes and observe it succeeds, then restart and retry the long-ID cast to confirm the same failure persists.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** In `core/id/id_wrappers.go`, `NewCast` now manually writes segment lengths with `uint8(len(sourceType))` and `uint8(len(targetType))`, which overflows for IDs >255 bytes. `core/id/id.go` shows `NewId` would otherwise switch to `newIdSecondFormat` when any segment exceeds 255, but `NewCast` bypasses that guard. `core/casts/collection.go` builds lookup table names from `castID.SourceType()/TargetType()`, so truncated decode yields the observed `(pg_catalog)|(int4)|()|()` lookup miss. The failing path is directly in PR-changed code.
- **Why this is likely a bug:** Runtime evidence consistently fails only when IDs cross the 255-byte boundary, and source inspection shows deterministic uint8 truncation in the changed encoding path. A targeted fix is to route `NewCast` back through `NewId` (or equivalent overflow-safe encoding) so long segments use the second format.
**Relevant code:**
`core/id/id_wrappers.go:94-103`
~~~go
return Cast(string([]byte{uint8(Section_Cast), 2, uint8(len(sourceType)), uint8(len(targetType))}) + string(sourceType) + string(targetType))
~~~
`core/id/id.go:102-113`
~~~go
if len(data) > 255 { return newIdSecondFormat(section, data) } ... if segmentLength > 255 { return newIdSecondFormat(section, data) }
~~~
`core/casts/collection.go:701-706`
~~~go
name := fmt.Sprintf(`(%s)|(%s)|(%s)|(%s)`, castID.SourceType().SchemaName(), castID.SourceType().TypeName(), castID.TargetType().SchemaName(), castID.TargetType().TypeName())
~~~
There was a problem hiding this comment.
Prolly map round-trip preserves bytes but loses semantic cast identity
What failed: The system preserves raw cast ID bytes in storage, but semantic decode uses wrapped uint8 segment lengths, so cast identity is corrupted and lookup paths such as CastIDToTableName cannot reliably resolve the original cast.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Creating casts with very long type identifiers can fail and crash lookup logic, preventing that cast workflow from completing reliably.
- Steps to Reproduce:
- Create cast IDs with source or target Type IDs longer than 255 bytes and persist them through the normal cast storage path.
- Read the same cast IDs back after the prolly map round-trip and inspect SourceType()/TargetType() lengths.
- Observe that byte payloads round-trip, but decoded source/target segments are truncated (for example 257-byte source decodes as length 1), causing downstream cast lookup mismatch/failure.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: In core/id/id_wrappers.go, NewCast now manually encodes two segment lengths with uint8(len(sourceType)) and uint8(len(targetType)). For lengths above 255, those bytes wrap and the first-format decoder in Id.Segment reads the wrapped values, returning truncated SourceType()/TargetType(). core/casts/collection.go then builds table names from those truncated types in CastIDToTableName, which explains the persisted-cast lookup failure despite distinct stored bytes. The smallest practical fix is to restore NewCast to the prior NewId-based construction (or equivalent fallback to the second format when a segment exceeds 255) so encode/decode preserves full segment boundaries.
- Why this is likely a bug: A valid identifier pair should round-trip through persistence without changing cast source/target semantics, but the current implementation decodes different identities from the same persisted bytes when lengths exceed 255. That deterministic mismatch in production code directly breaks cast usability for affected IDs.
Relevant code
core/id/id_wrappers.go:89-105
func NewCast(sourceType Type, targetType Type) Cast {
if len(sourceType) == 0 && len(targetType) == 0 {
return NullCast
}
return Cast(
string(
[]byte{
uint8(Section_Cast),
2,
uint8(len(sourceType)),
uint8(len(targetType)),
}) +
string(sourceType) +
string(targetType),
)
}core/id/id.go:102-113
if len(data) > 255 {
return newIdSecondFormat(section, data)
}
...
for _, segment := range data {
segmentLength := len(segment)
if segmentLength > 255 {
return newIdSecondFormat(section, data)
}
buf.WriteByte(uint8(segmentLength))
}core/casts/collection.go:700-706
func CastIDToTableName(castID id.Cast) doltdb.TableName {
name := fmt.Sprintf(`(%s)|(%s)|(%s)|(%s)`,
castID.SourceType().SchemaName(),
castID.SourceType().TypeName(),
castID.TargetType().SchemaName(),
castID.TargetType().TypeName())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 — Prolly map round-trip preserves bytes but loses semantic cast identity**
**What failed:** The system preserves raw cast ID bytes in storage, but semantic decode uses wrapped uint8 segment lengths, so cast identity is corrupted and lookup paths such as CastIDToTableName cannot reliably resolve the original cast.
- **Impact:** Creating casts with very long type identifiers can fail and crash lookup logic, preventing that cast workflow from completing reliably.
- **Steps to reproduce:**
1. Create cast IDs with source or target Type IDs longer than 255 bytes and persist them through the normal cast storage path.
2. Read the same cast IDs back after the prolly map round-trip and inspect SourceType()/TargetType() lengths.
3. Observe that byte payloads round-trip, but decoded source/target segments are truncated (for example 257-byte source decodes as length 1), causing downstream cast lookup mismatch/failure.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** In core/id/id_wrappers.go, NewCast now manually encodes two segment lengths with uint8(len(sourceType)) and uint8(len(targetType)). For lengths above 255, those bytes wrap and the first-format decoder in Id.Segment reads the wrapped values, returning truncated SourceType()/TargetType(). core/casts/collection.go then builds table names from those truncated types in CastIDToTableName, which explains the persisted-cast lookup failure despite distinct stored bytes. The smallest practical fix is to restore NewCast to the prior NewId-based construction (or equivalent fallback to the second format when a segment exceeds 255) so encode/decode preserves full segment boundaries.
- **Why this is likely a bug:** A valid identifier pair should round-trip through persistence without changing cast source/target semantics, but the current implementation decodes different identities from the same persisted bytes when lengths exceed 255. That deterministic mismatch in production code directly breaks cast usability for affected IDs.
**Relevant code:**
`core/id/id_wrappers.go:89-105`
~~~go
func NewCast(sourceType Type, targetType Type) Cast {
if len(sourceType) == 0 && len(targetType) == 0 {
return NullCast
}
return Cast(
string(
[]byte{
uint8(Section_Cast),
2,
uint8(len(sourceType)),
uint8(len(targetType)),
}) +
string(sourceType) +
string(targetType),
)
}
~~~
`core/id/id.go:102-113`
~~~go
if len(data) > 255 {
return newIdSecondFormat(section, data)
}
...
for _, segment := range data {
segmentLength := len(segment)
if segmentLength > 255 {
return newIdSecondFormat(section, data)
}
buf.WriteByte(uint8(segmentLength))
}
~~~
`core/casts/collection.go:700-706`
~~~go
func CastIDToTableName(castID id.Cast) doltdb.TableName {
name := fmt.Sprintf(`(%s)|(%s)|(%s)|(%s)`,
castID.SourceType().SchemaName(),
castID.SourceType().TypeName(),
castID.TargetType().SchemaName(),
castID.TargetType().TypeName())
~~~
Contributor
Footnotes
|
|
Diff SummaryThis diff run surfaced This run exercised database type-conversion behavior across normal workflows and edge cases, including creating and using casts, preserving cast behavior across restarts, function/type resolution, and compatibility checks for mixed data types. Most core casting and resolution paths looked stable, but upgrade-path behavior with existing stored type metadata showed a serious reliability gap. Not safe to merge yet — the only failure tied to this PR is high severity and causes a startup crash during upgrade scenarios with existing data, which is a release-blocking stability risk. Other observed failures appear pre-existing and are useful caveats, but they are not the main merge driver for this change. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Oversized cast identifiers trigger panic
Evidence Package⚪ Built-in implicit cast mixed numeric query fails on pg_typeof resolution
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
| github.com/dolthub/eventsapi_schema v0.0.0-20260310172945-37a9265ade69 | ||
| github.com/dolthub/flatbuffers/v23 v23.3.3-dh.2 | ||
| github.com/dolthub/go-mysql-server v0.20.1-0.20260626224942-f5b97c4d9179 | ||
| github.com/dolthub/go-mysql-server v0.20.1-0.20260701095706-bed08ec49e3c |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Upgrade crash when loading pre-upgrade cast metadata
What failed: Backward compatibility failed: persisted casts that were usable before upgrade trigger a startup panic after upgrade instead of remaining readable and discoverable.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: Upgrading to this build can crash startup when existing pre-upgrade cast/type metadata is present, blocking affected databases from serving normal queries until the binary is rolled back or patched.
- Steps to Reproduce:
- Start a pre-upgrade Doltgres binary and persist user-defined types/casts in a data directory.
- Stop the pre-upgrade binary and start the PR build (with the bumped go-mysql-server version) against that same data directory.
- Observe startup panic:
GetTypesCollectionFromContext(0x0, ...)reached fromrecursiveDeserializeTypeandDeserializeTypeFromString. - Verify the post-upgrade process exits and connections to the upgraded port are refused.
- Stub / mock content: Authentication was disabled in the local QA harness to simplify scripted connections; no cast-resolution mocks, route interception, or bug-specific bypasses were applied for this test flow.
- Code Analysis:
recursiveDeserializeTypecallsGetTypesCollectionFromContext(ctx, "")whentypeCollis nil, andGetTypesCollectionFromContextdereferencesctx.SessionviagetContextValueswithout a nil-context guard. The stack trace shows this path is now reached from the new go-mysql-server dependency viaDeserializeTypeFromStringwith a nil context after the PR dependency bump. A practical fix is to make deserialization resilient to nil context on this path (for example, explicit nil guard/fallback in the recursive type-collection fetch path or restoring context-aware invocation compatibility) so startup does not panic on pre-upgrade data. - Why this is likely a bug: The failure is reproducible across a clean pre-upgrade to post-upgrade run and is supported by a concrete panic stack in production code, not by a test harness assertion alone. A server crash while loading persisted metadata violates the expected upgrade compatibility contract for existing data.
Relevant code
go.mod:12
github.com/dolthub/go-mysql-server v0.20.1-0.20260701095706-bed08ec49e3cserver/types/serialization.go:266-268
if typeColl == nil {
typeColl, err = GetTypesCollectionFromContext(ctx, "")
if err != nil {core/context.go:55-57
func getContextValues(ctx *sql.Context) (*contextValues, error) {
sess := dsess.DSessFromSess(ctx.Session)
if sess.DoltgresSessObj == nil {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 — Upgrade crash when loading pre-upgrade cast metadata**
**What failed:** Backward compatibility failed: persisted casts that were usable before upgrade trigger a startup panic after upgrade instead of remaining readable and discoverable.
- **Impact:** Upgrading to this build can crash startup when existing pre-upgrade cast/type metadata is present, blocking affected databases from serving normal queries until the binary is rolled back or patched.
- **Steps to reproduce:**
1. Start a pre-upgrade Doltgres binary and persist user-defined types/casts in a data directory.
2. Stop the pre-upgrade binary and start the PR build (with the bumped go-mysql-server version) against that same data directory.
3. Observe startup panic: `GetTypesCollectionFromContext(0x0, ...)` reached from `recursiveDeserializeType` and `DeserializeTypeFromString`.
4. Verify the post-upgrade process exits and connections to the upgraded port are refused.
- **Stub / mock content:** Authentication was disabled in the local QA harness to simplify scripted connections; no cast-resolution mocks, route interception, or bug-specific bypasses were applied for this test flow.
- **Code analysis:** `recursiveDeserializeType` calls `GetTypesCollectionFromContext(ctx, "")` when `typeColl` is nil, and `GetTypesCollectionFromContext` dereferences `ctx.Session` via `getContextValues` without a nil-context guard. The stack trace shows this path is now reached from the new go-mysql-server dependency via `DeserializeTypeFromString` with a nil context after the PR dependency bump. A practical fix is to make deserialization resilient to nil context on this path (for example, explicit nil guard/fallback in the recursive type-collection fetch path or restoring context-aware invocation compatibility) so startup does not panic on pre-upgrade data.
- **Why this is likely a bug:** The failure is reproducible across a clean pre-upgrade to post-upgrade run and is supported by a concrete panic stack in production code, not by a test harness assertion alone. A server crash while loading persisted metadata violates the expected upgrade compatibility contract for existing data.
**Relevant code:**
`go.mod:12`
~~~go
github.com/dolthub/go-mysql-server v0.20.1-0.20260701095706-bed08ec49e3c
~~~
`server/types/serialization.go:266-268`
~~~go
if typeColl == nil {
typeColl, err = GetTypesCollectionFromContext(ctx, "")
if err != nil {
~~~
`core/context.go:55-57`
~~~go
func getContextValues(ctx *sql.Context) (*contextValues, error) {
sess := dsess.DSessFromSess(ctx.Session)
if sess.DoltgresSessObj == nil {
~~~
added 4 commits
July 7, 2026 14:58
Commit: SummaryThis run focused on database type-conversion behavior, covering normal cast create/use/drop flows, catalog identity roundtrips, and consistency between validation and execution paths. It also exercised adversarial edge conditions around identifier handling, with overall healthy behavior outside one extreme boundary case. Safe to merge — the only PR-attributable failure is a minor, edge-case issue in extreme identifier lengths, while core conversion and cast lifecycle behavior remained stable. The main risk is limited to uncommon schema definitions, so this is a flag-for-later fix rather than a broad merge blocker. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
There was a problem hiding this comment.
Very long type identifiers do not silently mis-key casts
What failed: The CREATE CAST path panics during cast ID decoding instead of returning a deterministic SQL error for oversized identifiers, producing asymmetric behavior versus DROP CAST.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Minor
- Impact: Creating a cast can fail with a runtime panic when type identifiers are extremely long, so affected teams cannot complete that schema change until identifiers are shortened.
- Steps to Reproduce:
- Create a schema and source type whose schema-qualified type ID exceeds 255 bytes (for example schema length 193 and type length 164).
- Create a conversion function, then run CREATE CAST (long_type AS text) WITH FUNCTION ... in the same session.
- Observe runtime panic
slice bounds out of rangefromcore/id/id.go:211; running DROP CAST for the same pair returns a deterministic does-not-exist error.
- Stub / mock content: The run used local QA setup with authentication disabled for scripted access, but no cast-logic stubs, mocks, or bypasses were applied to this test.
- Code Analysis: The PR-changed implementation of
NewCastnow serializes cast IDs with fixed one-byte length headers (uint8(len(sourceType)),uint8(len(targetType))). For long type IDs, these headers truncate true lengths, so later decode viacastID.SourceType().SchemaName()(inCastIDToTableName) passes malformed segment metadata intoId.Segment, which slices past available data and panics atcore/id/id.go:211. Before this PR,NewCastdelegated toNewId, which falls back to second-format encoding when any segment length exceeds 255. The smallest practical fix is to makeNewCastuseNewId(Section_Cast, ...)again, or add equivalent >255 guards and second-format fallback before writing first-format bytes. - Why this is likely a bug: A runtime panic from CREATE CAST is not an acceptable deterministic SQL outcome, and the panic trace matches a concrete unsafe slice caused by truncated cast-ID segment lengths. This is a production code defect in cast ID encoding/decoding rather than a test harness artifact.
Relevant code
core/id/id_wrappers.go:89-105
func NewCast(sourceType Type, targetType Type) Cast {
if len(sourceType) == 0 && len(targetType) == 0 {
return NullCast
}
return Cast(
string(
[]byte{
uint8(Section_Cast),
2,
uint8(len(sourceType)),
uint8(len(targetType)),
}) +
string(sourceType) +
string(targetType),
)
}core/id/id.go:96-113
func NewId(section Section, data ...string) Id {
if len(data) > 255 {
return newIdSecondFormat(section, data)
}
...
for _, segment := range data {
segmentLength := len(segment)
if segmentLength > 255 {
return newIdSecondFormat(section, data)
}
buf.WriteByte(uint8(segmentLength))
}
}core/id/id.go:200-212
segmentCount := int(id[1])
data := id[2+segmentCount:]
...
for i := 0; i <= index; i++ {
start += currentLength
currentLength = int(id[2+i])
}
return string(data[start : start+currentLength])core/casts/collection.go:701-706
func CastIDToTableName(castID id.Cast) doltdb.TableName {
name := fmt.Sprintf(`(%s)|(%s)|(%s)|(%s)`,
castID.SourceType().SchemaName(),
castID.SourceType().TypeName(),
castID.TargetType().SchemaName(),
castID.TargetType().TypeName())Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Minor severity — Very long type identifiers do not silently mis-key casts**
**What failed:** The CREATE CAST path panics during cast ID decoding instead of returning a deterministic SQL error for oversized identifiers, producing asymmetric behavior versus DROP CAST.
- **Impact:** Creating a cast can fail with a runtime panic when type identifiers are extremely long, so affected teams cannot complete that schema change until identifiers are shortened.
- **Steps to reproduce:**
1. Create a schema and source type whose schema-qualified type ID exceeds 255 bytes (for example schema length 193 and type length 164).
2. Create a conversion function, then run CREATE CAST (long_type AS text) WITH FUNCTION ... in the same session.
3. Observe runtime panic `slice bounds out of range` from `core/id/id.go:211`; running DROP CAST for the same pair returns a deterministic does-not-exist error.
- **Stub / mock content:** The run used local QA setup with authentication disabled for scripted access, but no cast-logic stubs, mocks, or bypasses were applied to this test.
- **Code analysis:** The PR-changed implementation of `NewCast` now serializes cast IDs with fixed one-byte length headers (`uint8(len(sourceType))`, `uint8(len(targetType))`). For long type IDs, these headers truncate true lengths, so later decode via `castID.SourceType().SchemaName()` (in `CastIDToTableName`) passes malformed segment metadata into `Id.Segment`, which slices past available data and panics at `core/id/id.go:211`. Before this PR, `NewCast` delegated to `NewId`, which falls back to second-format encoding when any segment length exceeds 255. The smallest practical fix is to make `NewCast` use `NewId(Section_Cast, ...)` again, or add equivalent >255 guards and second-format fallback before writing first-format bytes.
- **Why this is likely a bug:** A runtime panic from CREATE CAST is not an acceptable deterministic SQL outcome, and the panic trace matches a concrete unsafe slice caused by truncated cast-ID segment lengths. This is a production code defect in cast ID encoding/decoding rather than a test harness artifact.
**Relevant code:**
`core/id/id_wrappers.go:89-105`
~~~go
func NewCast(sourceType Type, targetType Type) Cast {
if len(sourceType) == 0 && len(targetType) == 0 {
return NullCast
}
return Cast(
string(
[]byte{
uint8(Section_Cast),
2,
uint8(len(sourceType)),
uint8(len(targetType)),
}) +
string(sourceType) +
string(targetType),
)
}
~~~
`core/id/id.go:96-113`
~~~go
func NewId(section Section, data ...string) Id {
if len(data) > 255 {
return newIdSecondFormat(section, data)
}
...
for _, segment := range data {
segmentLength := len(segment)
if segmentLength > 255 {
return newIdSecondFormat(section, data)
}
buf.WriteByte(uint8(segmentLength))
}
}
~~~
`core/id/id.go:200-212`
~~~go
segmentCount := int(id[1])
data := id[2+segmentCount:]
...
for i := 0; i <= index; i++ {
start += currentLength
currentLength = int(id[2+i])
}
return string(data[start : start+currentLength])
~~~
`core/casts/collection.go:701-706`
~~~go
func CastIDToTableName(castID id.Cast) doltdb.TableName {
name := fmt.Sprintf(`(%s)|(%s)|(%s)|(%s)`,
castID.SourceType().SchemaName(),
castID.SourceType().TypeName(),
castID.TargetType().SchemaName(),
castID.TargetType().TypeName())
~~~
|
Diff SummaryThis run exercised core cast and type-conversion behavior across normal and edge conditions, including retry stability, mode-specific conversion semantics, fallback route determinism, and long-identifier handling. Overall behavior remained consistent and deterministic, with expected success and explicit error outcomes where appropriate. Safe to merge — this run found no regressions, no new failures, and no still-failing issues attributable to this PR. The validated conversion and cast-management paths remained stable under repeated and boundary-style scenarios, indicating low merge risk from the tested surface. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
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.
No description provided.