Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements several optional database/sql/driver interfaces for the rows struct, providing metadata about column types such as scan types, database type names, and precision/scale. The implementation includes support for both GoogleSQL and PostgreSQL dialects. Review feedback highlights an inconsistency in DATE and UUID array decoding compared to scalar types and suggests returning []any instead of any for unknown array elements. Additionally, it is recommended to use standard PostgreSQL array syntax (e.g., text[]) rather than internal names (e.g., _int8) for better consistency and user familiarity.
| et := scanType(t.ArrayElementType, decodeToNativeArrays, state) | ||
| if decodeToNativeArrays { | ||
| return reflect.SliceOf(et) | ||
| } |
There was a problem hiding this comment.
There is an inconsistency in how DATE and UUID types are handled between scalar and array decoding when decodeToNativeArrays is true. Scalar DATE and UUID values are decoded as string (see lines 323 and 352), but their array counterparts are decoded as []civil.Date and []uuid.UUID (see lines 464 and 498). The current implementation of scanType would incorrectly return []string for these arrays because it recursively calls scanType on the element type.
et := scanType(t.ArrayElementType, decodeToNativeArrays, state)
if decodeToNativeArrays {
switch t.ArrayElementType.Code {
case sppb.TypeCode_DATE:
return reflect.TypeOf([]civil.Date{})
case sppb.TypeCode_UUID:
return reflect.TypeOf([]uuid.UUID{})
}
return reflect.SliceOf(et)
}| if t.ArrayElementType == nil { | ||
| return reflect.TypeOf([]any{}).Elem() | ||
| } |
There was a problem hiding this comment.
When the array element type is unknown, ColumnTypeScanType should return a slice type (e.g., []any) rather than the element type any. reflect.TypeOf([]any{}).Elem() evaluates to interface{}, which represents a scalar value.
| if t.ArrayElementType == nil { | |
| return reflect.TypeOf([]any{}).Elem() | |
| } | |
| if t.ArrayElementType == nil { | |
| return reflect.TypeOf([]any{}) | |
| } |
| if isPG { | ||
| switch t.ArrayElementType.Code { | ||
| case sppb.TypeCode_STRING: | ||
| return "_text" | ||
| case sppb.TypeCode_INT64: | ||
| return "_int8" | ||
| case sppb.TypeCode_FLOAT64: | ||
| return "_float8" | ||
| case sppb.TypeCode_BOOL: | ||
| return "_bool" | ||
| case sppb.TypeCode_BYTES: | ||
| return "_bytea" | ||
| case sppb.TypeCode_DATE: | ||
| return "_date" | ||
| case sppb.TypeCode_TIMESTAMP: | ||
| return "_timestamptz" | ||
| case sppb.TypeCode_NUMERIC: | ||
| return "_numeric" | ||
| case sppb.TypeCode_JSON: | ||
| return "_jsonb" | ||
| default: | ||
| return "_" + databaseTypeName(t.ArrayElementType, dialect) | ||
| } | ||
| } |
There was a problem hiding this comment.
The database type names for PostgreSQL arrays are inconsistent with the scalar type names and use internal PostgreSQL names (e.g., _int8 for bigint[]). It is better to use the standard typename[] syntax, which is consistent with the scalar names returned by this function and more familiar to users. This also simplifies the implementation by removing the need for a separate switch statement for PG arrays.
if isPG {
return databaseTypeName(t.ArrayElementType, dialect) + "[]"
}| if g, w := rpg.ColumnTypeDatabaseTypeName(2), "numeric"; g != w { | ||
| t.Errorf("ColumnTypeDatabaseTypeName(2) PG mismatch\n Got: %v\nWant: %v", g, w) | ||
| } | ||
| if g, w := rpg.ColumnTypeDatabaseTypeName(3), "_text"; g != w { |
There was a problem hiding this comment.
If the PostgreSQL array naming is updated to use the typename[] syntax as suggested in rows.go, this test expectation should be updated from _text to text[].
| if g, w := rpg.ColumnTypeDatabaseTypeName(3), "_text"; g != w { | |
| if g, w := rpg.ColumnTypeDatabaseTypeName(3), "text[]"; g != w { |
1 participant
No description provided.