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.

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

proposal: database/sql: value converter interface for rows.Scan #22544

Open
kshvakov opened this issue Nov 2, 2017 · 17 comments

Comments

Projects
None yet
9 participants
@kshvakov
Copy link

commented Nov 2, 2017

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

@gopherbot gopherbot added this to the Proposal milestone Nov 2, 2017

@gopherbot gopherbot added the Proposal label Nov 2, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2017

@kardianos

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2017

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

This comment has been minimized.

Copy link
Author

commented Nov 7, 2017

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

This comment has been minimized.

Copy link

commented Nov 8, 2017

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

This comment has been minimized.

Copy link

commented Nov 24, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2017

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

@kardianos

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2017

@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.

@kardianos kardianos self-assigned this Dec 4, 2017

@posener

This comment has been minimized.

Copy link

commented Dec 5, 2017

@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

This comment has been minimized.

Copy link
Author

commented Dec 5, 2017

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

@hasSalil

This comment has been minimized.

Copy link

commented Dec 19, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2018

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

@posener

This comment has been minimized.

Copy link

commented Jan 27, 2018

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

@kardianos

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2018

@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

This comment has been minimized.

Copy link

commented Apr 19, 2018

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

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.