bug fix: load non-built-in types when looking for an array type's element type#2729
Conversation
|
|
Ito Test Report ❌13 test cases ran. 1 failed, 2 additional findings, 10 passed. Overall, 10 of 13 test cases passed, showing broad stability in schema/type resolution, context-aware array base-type handling, composite-array parsing, malformed-literal rejection, and server resilience after controlled errors. The most important findings are three high-severity backend defects: a newly introduced panic risk when serializing SQL function results of user-defined composite arrays (aggtype[]) and two pre-existing polymorphic anyarray invocation/overload failures that raise unbound variable "v1" at runtime. ❌ Failed (1)
|
Hydrocharged
left a comment
There was a problem hiding this comment.
Overall looks fine, but definitely have some comments/questions. Additionally, it seems like the primary reason that we needed to thread context through ArrayBaseType was due to not fully resolving types upon their creation. This is something I'm running into concurrently and am working on changing that.
Ito Diff Report ❌Tested: Overall, the unified run failed with 14 total tests, with 8 passing checks confirming stable behavior for many polymorphic, domain/enum-array, NULL-handling, ANY-predicate, and parser-recovery scenarios, but 6 reproducible failures remained. The most important findings were real production defects where ❌ Failures (5)
|
| Category | Summary | Screenshot |
|---|---|---|
| Composite | Malformed literals failed immediately with explicit arity mismatch errors for too-few and too-many fields. | N/A |
| Composite | Malformed composite/array literals returned controlled parser errors and the backend remained responsive. | N/A |
| Composite | Mixed composite[] insert failed atomically on invalid input, then succeeded cleanly after correction. | N/A |
| Context | All malformed array literals failed with explicit parse/lexical errors and did not insert any data (COUNT(*)=0), demonstrating failure is critical and state remains consistent. |
N/A |
| Resolve | Unqualified casts and ARRAY constructors resolved to the active schema's same-named type in each search_path context. | N/A |
| Resolve | Blank-schema array type fallback correctly resolved to the current schema (s2) for acctype[]. | N/A |
| Resolve | Inferred unresolved ARRAY casts matched explicit schema-qualified casts across schema-ordering contexts. | N/A |
⚠️ Additional Findings (1)
These findings are unrelated to the current changes but were observed during testing.
| Origin | Category | Severity | Summary | Screenshot |
|---|---|---|---|---|
| 🆕 New | Composite | Consuming an accepted aggtype[] value triggered a panic instead of returning a controlled SQL error or result. |
![]() |
⚠️ Composite array consumption triggers ArrayBaseType panic
- What failed: The engine panics with
cannot get base type from: _aggtype6during downstream evaluation instead of producing a stable typed result or a controlled SQL error. - Impact: Queries that pass user-defined composite arrays through polymorphic paths can crash execution, breaking a core SQL workflow. This can terminate request handling for affected operations and has no reasonable SQL-side workaround.
- Steps to reproduce:
- Create aggtype and define a function that appends ROW values into an aggtype[] array.
- Execute downstream calls that consume the produced array, such as consume_arr(make_arr()).
- Repeat with a direct array literal cast to aggtype[] and observe the panic path during evaluation.
- Stub / mock context: SCRAM authentication was temporarily disabled in server/authentication_scram.go so local SQL execution could proceed without auth setup; no route interception or response stubbing was used for this case.
- Code analysis: I inspected the polymorphic evaluation and array base-type resolution paths.
CompiledFunctionstill usesArrayBaseType()in polymorphic validation, which drops context, andArrayBaseTypeCtxthen panics when user-defined element type lookup cannot complete without context. - Why this is likely a bug: The production code takes a context-free array base-type path for polymorphic arrays and can deterministically hit an unconditional panic for user-defined array element types.
Relevant code:
server/functions/framework/compiled_function.go (lines 760-763)
baseExprType := exprTypes[i]
if baseExprType.IsArrayType() {
baseExprType = baseExprType.ArrayBaseType()
}server/types/type.go (lines 146-150)
func (t *DoltgresType) ArrayBaseType() *DoltgresType {
// TODO: Remove this method and rename ArrayBaseTypeCtx(ctx)
// to ArrayBaseType(ctx) when all callers are migrated.
return t.ArrayBaseTypeCtx(nil)
}server/types/type.go (lines 169-180)
if t.IsUnresolved && t.Elem != id.NullType {
return NewUnresolvedDoltgresType(t.Elem.SchemaName(), t.Elem.TypeName())
}
if ctx != nil && t.Elem != id.NullType {
if typeColl, err := GetTypesCollectionFromContext(ctx); err == nil && typeColl != nil {
if elemType, err := typeColl.GetType(ctx, t.Elem); err == nil && elemType != nil {
newElem := *elemType.WithAttTypMod(t.attTypMod)
return &newElem
}
}
}
panic(fmt.Sprintf("cannot get base type from: %s", t.Name()))Tell us how we did: Give Ito Feedback
























Various bug fixes for regressions reported in PR #2642, primarily related to resolving arrays of non-built-in types. The major change is adding
ctxto theArrayBaseType()method, but not all call sites could be updated, so unfortunately results in a newArrayBaseTypeCtx(ctx)method until we can find a way to always provide a sql context and completely remove the old method form.