-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use sqlite3_column_type() for ColumnTypeScanType() #1327
base: master
Are you sure you want to change the base?
Conversation
SQLITE_NULL | ||
) | ||
|
||
var ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these all public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had my Java brain on. Making them private.
sqlite3_type.go
Outdated
TYPE_RAWBYTES = reflect.TypeOf(sql.RawBytes{}) | ||
TYPE_NULLBOOL = reflect.TypeOf(sql.NullBool{}) | ||
TYPE_NULLTIME = reflect.TypeOf(sql.NullTime{}) | ||
TYPE_ANY = reflect.TypeOf(new(any)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this is unfortunate - this means we are saying *any
instead of any
. I know that's the existing behavior, but does that even work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*any
is the expected type, see the Rows.Scan()
docs:
// If an argument has type *interface{}, Scan copies the value
// provided by the underlying driver without conversion. When scanning
// from a source value of type []byte to *interface{}, a copy of the
// slice is made and the caller owns the result.
Relevant code:
case nil:
switch d := dest.(type) {
case *any:
if d == nil {
return errNilPtr
}
*d = nil
return nil
In our case, this works:
for rr.Next() {
cc, err := rr.ColumnTypes()
if err != nil {
log.Fatal(err)
}
pointers := make([]any, 0)
for _, ct := range cc {
fmt.Printf("%s ", ct.ScanType())
pointers = append(pointers, reflect.New(ct.ScanType()).Interface())
}
fmt.Println()
if err := rr.Scan(pointers...); err != nil {
log.Fatal(err)
}
fmt.Println(pointers...)
for _, p := range pointers {
if pn, ok := p.(**any); ok {
fmt.Printf("%t ", *pn == nil)
}
}
}
sql.NullInt64 sql.NullString sql.NullFloat64
&{1 true} &{hello true} &{3.1415 true}
sql.NullInt64 *interface {} *interface {}
&{2 true} 0xc000064090 0xc000064098
true true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's unrelated. Rows.Scan
is talking about its arguments, which by necessity all have to be pointers as they are essentially output parameter. Meanwhile, ColumnTypeScanType
is talking about the type to scan into (so not a pointer).
For example, ColumnTypeScanType
mentions returning reflect.TypeOf(int64(0))
while Rows.Scan
mentions *int64
.
You'll also notice that all the other types being returned (e.g., TYPE_NULLSTRING
) here are not pointers.
In other words, you are supposed to create a variable of the type from ColumnTypeScanType
, and then pass a pointer to it to Rows.Scan
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a unit test for the new behavior. |
Should we still return the nullable types? When SQLite returns a NULL value, the column type will be type_any, so there is no need for nullable types now. |
The current implementation was changed in PR-909 stating that
sqlite3_column_type()
always returns SQLITE_NULL, but as noted in ISSUE-600, the case was probably because for SQLite3,sqlite3_column_type()
must be called after callingNext()
. sqlite3_column_type() returns the types for the columns on a given row, thus the cursor must be pointing to a valid row. See SQLite docs on API lifecycle, rows and columns methods and this forum postThe Go API does not restrain the behavior of
RowsColumnTypeScanType()
as to when it should be called. Thus, the implementation in this PR makes a compromise and falls back tosqlite3_column_decltype()
in a best effort to remain retro-compatible, but this is still a potentially breaking change.The advantage of using
sqlite3_column_type()
oversqlite3_column_decltype()
is two-fold:sqlite3_column_decltype()
is insufficient. Regarding this aspect, the existing implementation provides no way to access the values without potentially triggering a type conversion, unless the application is aware of the result type of the query beforehand.