Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+49 −20
Open

use sqlite3_column_type() for ColumnTypeScanType() #1327

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a6a5dc
use sqlite3_column_type() for ColumnTypeScanType()
alkemir Mar 17, 2025
27e8f68
add clarifying comment for skipping C.SQLITE_NULL
alkemir Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 49 additions & 20 deletions sqlite3_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,27 @@ import (
"strings"
)

const (
SQLITE_INTEGER = iota
SQLITE_TEXT
SQLITE_BLOB
SQLITE_REAL
SQLITE_NUMERIC
SQLITE_TIME
SQLITE_BOOL
SQLITE_NULL
)

var (
Copy link
Collaborator

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?

TYPE_NULLINT = reflect.TypeOf(sql.NullInt64{})
TYPE_NULLFLOAT = reflect.TypeOf(sql.NullFloat64{})
TYPE_NULLSTRING = reflect.TypeOf(sql.NullString{})
TYPE_RAWBYTES = reflect.TypeOf(sql.RawBytes{})
TYPE_NULLBOOL = reflect.TypeOf(sql.NullBool{})
TYPE_NULLTIME = reflect.TypeOf(sql.NullTime{})
TYPE_ANY = reflect.TypeOf(new(any))
Copy link
Collaborator

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?

)

// ColumnTypeDatabaseTypeName implement RowsColumnTypeDatabaseTypeName.
func (rc *SQLiteRows) ColumnTypeDatabaseTypeName(i int) string {
return C.GoString(C.sqlite3_column_decltype(rc.s.s, C.int(i)))
Expand All @@ -39,42 +60,50 @@ func (rc *SQLiteRows) ColumnTypeNullable(i int) (nullable, ok bool) {
}

// ColumnTypeScanType implement RowsColumnTypeScanType.
// In SQLite3, this method should be called after Next() has been called, as sqlite3_column_type()
// returns the column type for a specific row. If Next() has not been called, fallback to
// sqlite3_column_decltype()
func (rc *SQLiteRows) ColumnTypeScanType(i int) reflect.Type {
//ct := C.sqlite3_column_type(rc.s.s, C.int(i)) // Always returns 5
switch C.sqlite3_column_type(rc.s.s, C.int(i)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the SQL statement does not currently point to a valid row, or if the column index is out of range, the result is undefined.

We cannot call this if we haven't called Next yet, or if we have iterated past the last row, even if it happens to work today.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if you mean

  1. SQLiteRows.ColumnTypeScanType() should make sure that we have a valid row so it does not run into undefined behavior, in which case it should signal an error condition, or
  2. The driver should not call C.sqlite3_column_type() except inside Next(), which is the only point at which we can assure that we have a valid row.

If it is the first, the method signature does not allow a clear way of signaling errors (except for the empty interface, which signals not-supported) and I can't find a way to know if we have a valid row. The design of the packages (sql and go-sqlite3) seems to imply this should not be a concern beyond Next() and Scan(). Also the docs do not prescribe a context in which it is ok to call this method, but they do for other methods like Close() and Scan() implying it should always be ok to call SQLiteRows.ColumnTypeScanType(). The risk here is that the undefined behavior might mask as a type that is actually invalid for scanning. But, the implication is that the calling code would then try to Scan(), at which point it would get an error, so I see the risk as mitigated.

If it is the second, it is troublesome because that means there is no way of getting the rows without risking a type conversion unless the types are known by the calling code.

case C.SQLITE_INTEGER:
return TYPE_NULLINT
case C.SQLITE_FLOAT:
return TYPE_NULLFLOAT
case C.SQLITE_TEXT:
return TYPE_NULLSTRING
case C.SQLITE_BLOB:
return TYPE_RAWBYTES
// This case can signal that the value is NULL or that Next() has not been called yet.
// Skip it and return the fallback behaviour as a best effort. This is safe as all types
// returned are Nullable or any, which is the expected value for SQLite3.
//case C.SQLITE_NULL:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented out?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it doesn't apply, then just remove the code?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its to document that the omission is intentional and not a miss, this is the actual value that we would be getting in 2 cases:

  1. If we call before we have a valid row
  2. If the value for the row-column is NULL

Happy to remove it or add a comment clarifying.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like a mistake. An explicit comment is better.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment. Please take another look.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

// return TYPE_ANY
}

// Fallback to schema declared to remain retro-compatible
return scanType(C.GoString(C.sqlite3_column_decltype(rc.s.s, C.int(i))))
}

const (
SQLITE_INTEGER = iota
SQLITE_TEXT
SQLITE_BLOB
SQLITE_REAL
SQLITE_NUMERIC
SQLITE_TIME
SQLITE_BOOL
SQLITE_NULL
)

func scanType(cdt string) reflect.Type {
t := strings.ToUpper(cdt)
i := databaseTypeConvSqlite(t)
switch i {
case SQLITE_INTEGER:
return reflect.TypeOf(sql.NullInt64{})
return TYPE_NULLINT
case SQLITE_TEXT:
return reflect.TypeOf(sql.NullString{})
return TYPE_NULLSTRING
case SQLITE_BLOB:
return reflect.TypeOf(sql.RawBytes{})
return TYPE_RAWBYTES
case SQLITE_REAL:
return reflect.TypeOf(sql.NullFloat64{})
return TYPE_NULLFLOAT
case SQLITE_NUMERIC:
return reflect.TypeOf(sql.NullFloat64{})
return TYPE_NULLFLOAT
case SQLITE_BOOL:
return reflect.TypeOf(sql.NullBool{})
return TYPE_NULLBOOL
case SQLITE_TIME:
return reflect.TypeOf(sql.NullTime{})
return TYPE_NULLTIME
}
return reflect.TypeOf(new(any))
return TYPE_ANY
}

func databaseTypeConvSqlite(t string) int {
Expand Down