database/sql: value converter interface for rows.Scan

[kshvakov]

Starting with Go 1.8 database/sql have an interface driver.ColumnConverter for convert value. I think it will be better to provide the interface for Scan too. Currently, row.Scan using convertAssign and it very slow

[ianlancetaylor]

CC @kardianos

[kardianos]

I could get behind such an interface potentially. There is some room for optimization there. Right now the interface scans an entire row at a time. And as you say there isn't a way to override the convert / assignment behavior. I haven't put any time into attempting to design such an interface.

Do you have any concrete proposal you'd like to put forward?

[kshvakov]

You can add interface ConvertAssigner like this

type ConvertAssigner interface {
	ConvertAssign(dest, src interface{}) error
}

And check it in Scan

func (rs *Rows) Scan(dest ...interface{}) error {
	rs.closemu.RLock()
	if rs.closed {
		rs.closemu.RUnlock()
		return errors.New("sql: Rows are closed")
	}
	rs.closemu.RUnlock()

	if rs.lastcols == nil {
		return errors.New("sql: Scan called without calling Next")
	}
	if len(dest) != len(rs.lastcols) {
		return fmt.Errorf("sql: expected %d destination arguments in Scan, not %d", len(rs.lastcols), len(dest))
	}

	convertAssign := defaultConvertAssign // defaultConvertAssign is a new name for the convertAssign function 
	if rows, ok := rs.rowsi.(ConvertAssigner); ok {
		convertAssign = rows.ConvertAssign
	}

	for i, sv := range rs.lastcols {
		if err := convertAssign(dest[i], sv); err != nil {
			return fmt.Errorf("sql: Scan error on column index %d: %v", i, err)
		}
	}
	return nil
}

Concrete driver can be more effective than defaultConvertAssign

[tgulacsi]

Do I read it correctly, that the driver could convert its value to the Scan'd destination, and know the type of the destination?
I'd love this possibility!
An Oracle NUMBER can hold 38 digits, and now I convert it to string, to not lose information.
But if I'd know that the result is int64, I could shorten then OCINumber -(driver)-> string -(database/sql)-> int64 conversion path and provide an OCINumber -(driver)-> int64 conversion.

Another use case is what we're using interface{} for know: for LOBs we now return a goracle.Lob, and the user must type-assert it; if the driver could get anything, an io.Reader or []byte or string could be filled directly.
But maybe this is just too much magic...

[posener]

I met the same issue.
Another thing here is that maybe I don't even want to use the dest interface{} argument list.
Another idea to improve performance in certain use cases is to expose Rows.lastcols in a certain way.
I think this make sense, since if you have your own custom scanner, you don't even need the conversion to the interfaces in the Scan method...

Lets say that exposing Rows.lastcos is available with a function Values() ([]driver.Value, error), In that case, the use case would be:

rows, err := db.Query(stmt, args...)
if err != nil {
	return nil, err
}
defer rows.Close()
for rows.Next() {
	item, err := myCustomScanner(rows.Values())
	if err != nil {
		return nil, err
	}
	// use item...
}

[rsc]

@kardianos, is there a concrete proposal to be evaluated yet? I'm not clear on the current status.

[kardianos]

@rsc No concrete proposal. The closest is #22544 (comment) and that doesn't document all of the effects it would have / corner cases / ways it is used. The suggested implementation would need some work as well.

I'll self assign and look into this in the next month.

[posener]

@kardianos why not just expose lastcols, as I said before, I see a lot of advantages of doing so:

• Much simpler
• No need for interface conversion of types you already know.
• The ConvertAssigner approach enforced use of reflections, which in some cases is not needed.
• There is no much logic in the Scan function, and you can let the user write it's own implementation.

Please consider that,
Cheers.

[kshvakov]

Exposing lastcols is a good solution for some cases too. New functionality like ConvertAssigner can speed up the existing applications without rework it.

[hasSalil]

why not have a generalized interface for Scan that leaves parsing and conversions up to the user (instead of exposing rs.lastcols):

type RowScanner interface {
        Scan(row []driver.Value) error
}

func (rs *Rows) ScanCustom(dest RowScanner) error {
	rs.closemu.RLock()
	if rs.closed {
		rs.closemu.RUnlock()
		return errors.New("sql: Rows are closed")
	}
	rs.closemu.RUnlock()

	if rs.lastcols == nil {
		return errors.New("sql: Scan called without calling Next")
	}
	return dest.Scan(rs.lastcols)
}

Doing this lets the developer do direct conversions, or marshaling to whatever form they want without needing to buffer the data in arrays

[kardianos]

Myself and my family has been sick a bit this last month. Pushing out again.

[posener]

Hi, @kardianos
Feel well! :-)
Is it planned to be implemented? which go version? (it is still in the proposal milestone)
Thanks!

[kardianos]

@rsc I think there is value in this proposal. I have two proposals for this. I lean to the first one, but that would require an interface assertion for each value which may be expensive, but it would remove the type assertions for the src that convertAssign does anyway because the src.Scan will knows its own type.

Define an interface for value sources to be scanned from

Right now a user may pass in to Rows.Scan a sql.Scanner type https://tip.golang.org/pkg/database/sql/#Scanner . But I would argue that this is the wrong direction. Really, the database driver value should have the Scan(dest interface{}) error method to make the appropriate client side contract for a given column type (driver promise to user code it may pass in X, Y, Z types for a given db column type). If the dest is actually a plain *interface{}, then it may choose to store the value as a particular type.

The only code that would change would be:

convert.go

func convertAssign(dest, src interface{}) error {
+	if scanner, ok := src.(Scanner); ok { // Will this be slow?
+		err := scanner.Scan(dest)
+		if err != driver.ErrSkip {
+			return err
+		}
+	}

Define a generic custom converter a connection can implement

In func (db *DB) queryDC and func (s *Stmt) QueryContext we test if the dc.ci (driver.Conn) implements the interface driver.ConvertAssign, defined below, and store it in the sql.Rows struct for use in Rows.Scan.

type ConvertAssign interface {
    Conn

    // ConvertAssign will convert the src into the dest value.
    // This may be called concurrently with other driver code.
    //
    // Returning driver.ErrSkip will use default converter.
    ConvertAssign(dest, src interface{}) error
}

Then in Rows.Scan we first attempt to use the ConvertAssign interface defined on the driver.Conn. If it returns a drvier.ErrSkip it uses the existing convertAssign function.

---

This does not address inefficiencies with storing data in an intermediate []interface{}. However, we should probably wait until a Go2 to attempt to change that.

For Go2: If Rows.Scan was restricted to one all per Next, or if Rows.Scan was allowed to progress the columns it scanned on secuessive Scan calls rows.Scan(&c1, &c2); rows.Scan(&c3) then would be no reason to store the row buffer at all, but rather have the following defined on driver.Rows:

package driver

type EfficientScanner interface {
    Rows
    
    // Point to the next row.
    EfficientNext() error

    // Each call scans the next column for the current row into dest.
    // Drivers may call database/sql/driver.ConvertAssign (previously was sql.convertAssign).
    EfficientScanValue(dest interface{}) error
}

But this would not be compatible with Go1.

[AlekSi]

I think we should implement both proposals: Scanner will be useful for the uses of existing drivers when they want to tweak the behavior for individual values/columns, while ConvertAssign will be useful for the specific driver implementations like @kshvakov's ClickHouse driver, or my own MySQL X Protocol driver.

[kardianos]

@AlekSi Both proposals would depend on driver authors. Users have a Scanner type they can use today to pass into the Scan method. Proposal 1 would put a scanner call to src (not dest), so the drivers would create a custom type and put that in the row buffer

package mysql-driver

type UUID []byte
func (u UUID) Scan(dest interface{}) error {
    // In here we know that the src is a UUID. Not a string or bytea.
    // We can then make a more informed, targeted, decision when the dest is
    // inspected.
}
type RowID [19]byte // Oracle ROWID
func(rid RowID) Scan(dest interface{}) error {
    // The driver passes back RowID back from the driver from Rows.Next slice.
    // Then the user can scan a string or decimal, or byte slice and the RowID type the database driver
    // provides can act on any of these representations. The user doesn't need to pass in a special type,
    // though they can, to interact with this type.
}

package user-code
func main() {
   // ...
    var rowid string
    row.Scan(&rowid)
}

To contrast with what exists today, a user would need to pass a special type into Scan.

package user-code
func main() {
   // ...
    var dv mydriver.RowID
    row.Scan(&v)
    v := dv.String()
}

---

The two proposals have the same effect. The difference is that with proposal 1 you don't need to do a type assertion on the dest, because the dest type is known already.

[gopherbot]

Change https://golang.org/cl/107995 mentions this issue: database/sql: add the driver.ValueScanner interface

[rsc]

@kardianos, does CL 107995 mean that you agree that this proposal should be accepted? Please mark proposal-accepted if so.

[jba]

This proposal was accepted over five years ago, but it's not even clear what the proposal is. The only CL link, https://golang.org/cl/107995, is broken (at least for me). I don't see evidence in the code of either of the additions suggested in #22544 (comment), so I assume this was never implemented.

Can someone clarify the state of this issue?

[seankhliao]

Let's close as superseded by #67546