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

database/sql/driver: Scan.Next documentation incorrect #6497

Closed
gopherbot opened this issue Sep 27, 2013 · 18 comments
Closed

database/sql/driver: Scan.Next documentation incorrect #6497

gopherbot opened this issue Sep 27, 2013 · 18 comments

Comments

@gopherbot
Copy link

by Christopher.Pfohl:

What is the expected output?

String types from a database should have a go type of 'string' in Scanner.Scan.

What do you see instead?

String types have a go type of `[]byte`

Which compiler are you using (5g, 6g, 8g, gccgo)?

Whatever installed when I apt-get'ed from ppa:duh/golang

Which operating system are you using?

Ubuntu 13.04

Which version are you using?  (run 'go version')

1.1.1 linux/amd64

Please provide any additional information below.

database/sql/driver.Rows.Next's documentation states that all strings ought to be
converted to []byte

database/sql.Scanner.Scan's documentation suggests that a string is a legitimate type to
expect...but all the types that actually *are* strings actually wind up being []byte
because the driver's obey the above documentation (see mgodbc for a case in point, line
916).

Note, that removing the conversion to []byte still works, so I suspect that the
database/sql/driver.Rows.Next documentation is simply out of date.
@gopherbot
Copy link
Author

Comment 1 by Christopher.Pfohl:

s/driver's/drivers'/

@gopherbot
Copy link
Author

Comment 2 by Christopher.Pfohl:

s/driver's/drivers/

@bradfitz
Copy link
Contributor

Comment 3:

I don't see the problem.
Just because something happens to work, that doesn't mean it's guaranteed.  I believe
the implementation of database/sql and database/sql/driver are consistent with each
other.  I don't know the mgodbc codebase and whether it's correct or not.
Please explain again what is wrong?
I will be gone for two weeks, though, so likely won't reply until then.

Status changed to WaitingForReply.

@gopherbot
Copy link
Author

Comment 4 by Christopher.Pfohl:

So, the instructions for drivers state to cast all strings to []byte when calling Next.
That means that the Scanner.Scan implementations will never receive a string type unless
the driver is implemented "incorrectly".  Of course I could be reading the docs and
source wrong.

@alexbrainman
Copy link
Member

Comment 5:

Value returned by driver gets converted to value returned to user in convertAssign.
Isn't it? Looking at the code, if both are strings, then no conversion is done. So that
explains why strings returned by driver reach their user. That is, I would say, quite
common scenario. And, I agree with you, perhaps documentation shouldn't be as strict.
Even if things work, we might end up with extra memory allocations that otherwise aren't
needed.
I don't see your point about Scanner.Scan. Can you explain more?
Alex

@gopherbot
Copy link
Author

Comment 6 by cpfohl@valetude.com:

Sorry for the long pause, work required a work around, and I didn't have time to get
back here.
I'll explain what I was trying to do, perhaps I'm doing it wrong, thinking about it
wrong, or something.  Should have done that in the first place, programmers write the
worst bug reports...I am a programmer.
I want to take the results of a query and scan them into `map[string]interface{}`, where
the key is the column name, and the value is the actual value at that location.  This is
handy for a variety of reasons, for me it's because I want to return the results of a
query as JSON without an intermediate struct that will be 100% coupled to my query.
Currently I'm doing this like so: http://play.golang.org/p/-vYciy-s9D
It works great as long the source data is typed correctly, but if the source data comes
from a driver which follows the 'convert to []byte' rule it fails, because deserializing
[]byte into interface{} results in the same thing when the original value was a varchar,
blob, or fixed width char(n); heck, xml in sqlserver is a []byte.
Does that make more sense?
The example I passed along doesn't run because I'm not really sure how to fake a
*sql.Rows...so if someone creates (or points me at instructions to create) the
mockRows() func it'll better exemplify this for you.

@gopherbot
Copy link
Author

Comment 7 by Christopher.Pfohl:

Sorry for the long pause, work required a work around, and I didn't have time to get
back here.
I'll explain what I was trying to do, perhaps I'm doing it wrong, thinking about it
wrong, or something.  Should have done that in the first place, programmers write the
worst bug reports...I am a programmer.
I want to take the results of a query and scan them into `map[string]interface{}`, where
the key is the column name, and the value is the actual value at that location.  This is
handy for a variety of reasons, for me it's because I want to return the results of a
query as JSON without an intermediate struct that will be 100% coupled to my query.
Currently I'm doing this like so: http://play.golang.org/p/-vYciy-s9D
It works great as long the source data is typed correctly, but if the source data comes
from a driver which follows the 'convert to []byte' rule it fails, because deserializing
[]byte into interface{} results in the same thing when the original value was a varchar,
blob, or fixed width char(n); heck, xml in sqlserver is a []byte.
Does that make more sense?
The example I passed along doesn't run because I'm not really sure how to fake a
*sql.Rows...so if someone creates (or points me at instructions to create) the
mockRows() func it'll better exemplify this for you.

@alexbrainman
Copy link
Member

Comment 8:

> ... but if the source data comes from a driver which follows the 'convert to []byte'
rule it fails, because deserializing []byte into interface{} results in the same thing
when the original value was a varchar, blob, or fixed width char(n); heck, xml in
sqlserver is a []byte.
Yes, you can change your (*Column).Scan to have:
case []byte:
        c.Val = string(x)
but this way you cannot distinguish between string columns and others (blobs, ...).
> ... types that actually *are* strings actually wind up being []byte because the
driver's obey the above documentation (see mgodbc for a case in point, line 916)
I am not sure mgodbc is doing good here. string to []byte conversions and back are not
free, you can look at generated asm (go build -ldflags "-a" ...) to see what happens
under covers.
Alex

@gopherbot
Copy link
Author

Comment 9 by Christopher.Pfohl:

@Alex, if mgodbc is not doing good here, then it's doing bad because it's obeying the
documentation (http://golang.org/pkg/database/sql/driver/#Rows) which says for type
Rows, method Next:
    // Next is called to populate the next row of data into
        // the provided slice. The provided slice will be the same
        // size as the Columns() are wide.
        //
        // The dest slice may be populated only with
        // a driver Value type, but excluding string.
        // All string values must be converted to []byte.
        //
        // Next should return io.EOF when there are no more rows.
Note the second to last line of text: "All string values must be converted to []byte"
How is mgodbc doing bad? It's obeying exactly what the documentation says to do.

@alexbrainman
Copy link
Member

Comment 10:

> ... How is mgodbc doing bad? ...
Most user programs will use "*string" in Scan to receive "string" database data. This is
how this data gets around:
1) mgodbc receives that data as utf16 from ODBC;
2) mgodbc uses utf16ToString to convert it to "string";
3) mgodbc converts it to "[]byte";
4) database/sql converts that back to "string";
I think steps 3) and 4) are waste of computer resources.
Alex

@gopherbot
Copy link
Author

Comment 11 by Christopher.Pfohl:

@Alex it doesn't do step 4, it leaves that to the user.  But worse yet, the user can't
tell if it was originally a string.
Also I repeat: mgodbc is doing *exactly* what the documentation says to do.  Do we agree
that the documentation is wrong?

@alexbrainman
Copy link
Member

Comment 12:

> ... it doesn't do step 4, it leaves that to the user. ...
Not the user, database/sql does that:
https://code.google.com/p/go/source/browse/src/pkg/database/sql/convert.go#109
> ... mgodbc is doing *exactly* what the documentation says to do.  Do we agree that the
documentation is wrong?
Yes, I don't understand many things in that doco. But I am not familiar with all design
details. You have to convince Brad.
Alex

@alexbrainman
Copy link
Member

Comment 13:

@Christopher,
It seems that reading any "sql string" value into sql.RawByte or []byte fails, if
database/sql/driver.Scan.Next returns string. I had to change my own driver to fix that.
Would you like to review it: https://golang.org/cl/18270043/ ?
Alex

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 14:

Labels changed: added go1.3maybe.

@dsymonds
Copy link
Contributor

dsymonds commented Dec 2, 2013

Comment 15:

Labels changed: added priority-soon, documentation, size-s, removed priority-triage.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 16:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 17:

Labels changed: added repo-main.

@gopherbot
Copy link
Author

CL https://golang.org/cl/19439 mentions this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants