Issue 4973055: code review 4973055: exp/sql{,/driver}: new database packages (Closed)
|
Created:
13 years, 2 months ago by bradfitz Modified:
13 years, 1 month ago Reviewers:
CC:
gustavo_niemeyer.net, rsc, borman, dave_cheney.net, kevlar, nigeltao, dvyukov, kardia, fw, r, r2, crawshaw2, golang-dev Visibility:
Public. |
Descriptionexp/sql{,/driver}: new database packages
Patch Set 1 #Patch Set 2 : diff -r f6449df27a7f https://go.googlecode.com/hg/ #Patch Set 3 : diff -r f6449df27a7f https://go.googlecode.com/hg/ #Patch Set 4 : diff -r f6449df27a7f https://go.googlecode.com/hg/ #
Total comments: 19
Patch Set 5 : diff -r 4beaeaead995 https://go.googlecode.com/hg #Patch Set 6 : diff -r 4beaeaead995 https://go.googlecode.com/hg #Patch Set 7 : diff -r 7469b572b79b https://go.googlecode.com/hg #
Total comments: 52
Patch Set 8 : diff -r 9a44dedca5dd https://go.googlecode.com/hg #Patch Set 9 : diff -r 9a44dedca5dd https://go.googlecode.com/hg #
Total comments: 45
Patch Set 10 : diff -r 93c459658010 https://go.googlecode.com/hg #Patch Set 11 : diff -r 6bfb65b2ff19 https://go.googlecode.com/hg #Patch Set 12 : diff -r 6bfb65b2ff19 https://go.googlecode.com/hg #
MessagesTotal messages: 44
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Docs at:
http://p.danga.com:6060/pkg/exp/db/
http://p.danga.com:6060/pkg/exp/db/dbimpl/
Goals of the db and db/dbimpl packages:
* Provide a generic database API for a variety of SQL or SQL-like
databases. There currently exist Go libraries for SQLite, MySQL,
and Postgres, but all with a very different feel, and often
a non-Go-like feel.
* Feel like Go.
* Care mostly about the common cases. Common SQL should be portable.
SQL edge cases or db-specific extensions can be detected and
conditionally used by the application. It is a non-goal to care
about every particular db's extension or quirk.
* Separate out the basic implementation of a database driver
(implementing the db/dbimpl interfaces) vs the implementation
of all the user-level types and convenience methods.
In a nutshell:
User Code ---> db package (concrete types) ---> db/dbimpl (interfaces)
Database Driver -> db (to register) + dbimpl (implement interfaces)
* To type casting/conversions consistently between all drivers. To
achieve this, most of the type conversions are done in the db
package, not in each driver. The drivers then only have to deal
with a smaller set of types.
* Be flexible with type conversions, but be paranoid about silent
truncation or other loss of precision.
* Handle concurrency well. Users shouldn't need to care about the
database's per-connection thread safety issues (or lack thereof),
and shouldn't have to maintain their own free pools of connections.
The 'db' package should deal with that bookkeeping as needed. Given
a *db.DB, it should be possible to share that instance between
multiple goroutines, without any extra synchronization.
* Push complexity, where necessary, down into the db+dbimpl packages,
rather than exposing it to users. Said otherwise, the db package
should expose an ideal database that's not finnicky about how it's
accessed, even if that's not true.
* Provide optional interfaces in dbimpl for drivers to implement
for special cases or fastpaths. But the only party that knows about
those is the 'db' package. To user code, some stuff just might start
working or start working slightly faster.
On Fri, Sep 2, 2011 at 10:04 AM, <bradfitz@golang.org> wrote:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
>
http://codereview.appspot.com/**4973055/<http://codereview.appspot.com/4973055/>
>
Sign in to reply to this message.
Lost in the original CL description after a server error: This was designed with Russ a couple months ago. A couple things changed slightly found during implementation, but it's very close to the original design. On Fri, Sep 2, 2011 at 10:05 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > Docs at: > > http://p.danga.com:6060/pkg/exp/db/ > http://p.danga.com:6060/pkg/exp/db/dbimpl/ > > Goals of the db and db/dbimpl packages: > > * Provide a generic database API for a variety of SQL or SQL-like > databases. There currently exist Go libraries for SQLite, MySQL, > and Postgres, but all with a very different feel, and often > a non-Go-like feel. > > * Feel like Go. > > * Care mostly about the common cases. Common SQL should be portable. > SQL edge cases or db-specific extensions can be detected and > conditionally used by the application. It is a non-goal to care > about every particular db's extension or quirk. > > * Separate out the basic implementation of a database driver > (implementing the db/dbimpl interfaces) vs the implementation > of all the user-level types and convenience methods. > In a nutshell: > > User Code ---> db package (concrete types) ---> db/dbimpl (interfaces) > Database Driver -> db (to register) + dbimpl (implement interfaces) > > * To type casting/conversions consistently between all drivers. To > achieve this, most of the type conversions are done in the db > package, not in each driver. The drivers then only have to deal > with a smaller set of types. > > * Be flexible with type conversions, but be paranoid about silent > truncation or other loss of precision. > > * Handle concurrency well. Users shouldn't need to care about the > database's per-connection thread safety issues (or lack thereof), > and shouldn't have to maintain their own free pools of connections. > The 'db' package should deal with that bookkeeping as needed. Given > a *db.DB, it should be possible to share that instance between > multiple goroutines, without any extra synchronization. > > * Push complexity, where necessary, down into the db+dbimpl packages, > rather than exposing it to users. Said otherwise, the db package > should expose an ideal database that's not finnicky about how it's > accessed, even if that's not true. > > * Provide optional interfaces in dbimpl for drivers to implement > for special cases or fastpaths. But the only party that knows about > those is the 'db' package. To user code, some stuff just might start > working or start working slightly faster. > > On Fri, Sep 2, 2011 at 10:04 AM, <bradfitz@golang.org> wrote: > >> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), >> >> Please take another look. >> >> >> >> http://codereview.appspot.com/**4973055/<http://codereview.appspot.com/4973055/> >> > >
Sign in to reply to this message.
Hey Brad, > * Provide a generic database API for a variety of SQL or SQL-like > databases. There currently exist Go libraries for SQLite, MySQL, > and Postgres, but all with a very different feel, and often > a non-Go-like feel. Just one minor point from the previous conversation on the topic: given that there are so many databases out there that do not suit this API design, and given the examples you use above, should this be more properly called as sqldb? -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I never filed a patent.
Sign in to reply to this message.
On Fri, Sep 2, 2011 at 10:23 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote: > Hey Brad, > > > * Provide a generic database API for a variety of SQL or SQL-like > > databases. There currently exist Go libraries for SQLite, MySQL, > > and Postgres, but all with a very different feel, and often > > a non-Go-like feel. > > Just one minor point from the previous conversation on the topic: > given that there are so many databases out there that do not suit this > API design, and given the examples you use above, should this be more > properly called as sqldb? > I'd rather not but I'm not going to fight either way. Renaming it is easy. Note that it's in exp/ right now, too. I'd rather focus on the code first.
Sign in to reply to this message.
Is there any hope of a more general package called db? If not, then db is fine. I think there's a strong argument to be made that those things are not databases. For example, people sometimes call bigtable a database, but it's not. Russ
Sign in to reply to this message.
> Is there any hope of a more general package called db? Having nothing more general is not good reasoning for calling a well-bounded concept generically. I could argue about databases that do not follow this API, but it's way easier to just point out that the API was clearly designed with SQL databases in mind, so why not naming it as such? Besides, "db" is a fantastic variable name in that space. It'd be a shame to kill it. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I never filed a patent.
Sign in to reply to this message.
On Fri, Sep 2, 2011 at 10:45 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote: > > Is there any hope of a more general package called db? > > Having nothing more general is not good reasoning for calling a > well-bounded concept generically. I could argue about databases that > do not follow this API, but it's way easier to just point out that the > API was clearly designed with SQL databases in mind, so why not naming > it as such? > > Besides, "db" is a fantastic variable name in that space. It'd be a > shame to kill it. > If anything I'd buy that argument most strongly.
Sign in to reply to this message.
I agree. I think something like sql, sqldb, or dbsql. This CL makes me long for a go package for Berkeley DB (sleepycat)... Fortunately I don't need it for my project since cgo is not (yet?) an option for me. On Fri, Sep 2, 2011 at 10:45 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote: > > Is there any hope of a more general package called db? > > Having nothing more general is not good reasoning for calling a > well-bounded concept generically. I could argue about databases that > do not follow this API, but it's way easier to just point out that the > API was clearly designed with SQL databases in mind, so why not naming > it as such? > > Besides, "db" is a fantastic variable name in that space. It'd be a > shame to kill it. > > -- > Gustavo Niemeyer > http://niemeyer.net > http://niemeyer.net/plus > http://niemeyer.net/twitter > http://niemeyer.net/blog > > -- I never filed a patent. >
Sign in to reply to this message.
On Fri, Sep 2, 2011 at 10:23 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote: > Hey Brad, > > > * Provide a generic database API for a variety of SQL or SQL-like > > databases. There currently exist Go libraries for SQLite, MySQL, > > and Postgres, but all with a very different feel, and often > > a non-Go-like feel. > > Just one minor point from the previous conversation on the topic: > given that there are so many databases out there that do not suit this > API design, and given the examples you use above, should this be more > properly called as sqldb? Latest thinking: sql sql/driver (maybe; better than sql/sqlimpl) Then it's *sql.DB and driver.Conn, etc.
Sign in to reply to this message.
> Latest thinking: > sql > sql/driver (maybe; better than sql/sqlimpl) > Then it's *sql.DB and driver.Conn, etc. +1 -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I never filed a patent.
Sign in to reply to this message.
+1 Sent from my iPad On 03/09/2011, at 6:00, Brad Fitzpatrick <bradfitz@golang.org> wrote: > On Fri, Sep 2, 2011 at 10:23 AM, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: > Hey Brad, > > > * Provide a generic database API for a variety of SQL or SQL-like > > databases. There currently exist Go libraries for SQLite, MySQL, > > and Postgres, but all with a very different feel, and often > > a non-Go-like feel. > > Just one minor point from the previous conversation on the topic: > given that there are so many databases out there that do not suit this > API design, and given the examples you use above, should this be more > properly called as sqldb? > > Latest thinking: > > sql > sql/driver (maybe; better than sql/sqlimpl) > > Then it's *sql.DB and driver.Conn, etc.
Sign in to reply to this message.
I like the idea of this. While I don't think it's necessarily incompatible with some other interfaces to allow it to do document-oriented and object-oriented databases, it's definitely SQL-driven now. For this, I like the sql and sql/driver names, so I use those in my comments as if that's what they were named (because I can't seem to type dbimpl the first time without a typo). http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go File src/pkg/exp/db/db.go (right): http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode18 src/pkg/exp/db/db.go:18: // databases. I would put an example of how this might be used given a fictitions "foosql" package, given that the underscore-imports can take some getting-used-to: import ( "exp/sql" _ "somewhere.online.com/foosql" ) ... etc http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode34 src/pkg/exp/db/db.go:34: // a duplicate name. I would mention that this should be called in the init() method of a particular SQL driver's package. http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode61 src/pkg/exp/db/db.go:61: Ok bool // Ok is true if the String is not NULL I would call this NotNull? http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode276 src/pkg/exp/db/db.go:276: type connStmt struct { This name (and its associated function) read awkwardly for me. I don't know if calling it "prepared" or something would make matters better or worse. http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode297 src/pkg/exp/db/db.go:297: func (s *Stmt) Exec(args ...interface{}) (Result, os.Error) { Exec is envious of Query's comment. http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode346 src/pkg/exp/db/db.go:346: if _, match = s.db.connIfFree(cs.ci); match { Is there no clean way to do this without locks? Without connIfFree, you could probably make do with a buffered channel from which you popped free connections and to which you pushed ones with which you are done. connIfFree would have to pop all of them and push them back, which might be prohibitive. http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode399 src/pkg/exp/db/db.go:399: // always non-nil. Provide or refer to the example of chaining this with Scan? http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode497 src/pkg/exp/db/db.go:497: } The functional programmer in me wants: func (rs *Rows) Each(func(...interface{})) os.Error {} which should be possible by reflecting over the arguments and making new Values for each one? Overkill? Part of me thinks that the overhead of the initial reflection wouldn't be too bad when compared with a large dataset... unless I'm overestimating how much of the reflection would be precomputed and how much would happen every iteration. http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode535 src/pkg/exp/db/db.go:535: AutoIncrementId() (int64, os.Error) LastInsertId? http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode536 src/pkg/exp/db/db.go:536: RowsAffected() (int64, os.Error) AffectedRows? http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/fakedb_test.go File src/pkg/exp/db/fakedb_test.go (right): http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/fakedb_test.go... src/pkg/exp/db/fakedb_test.go:32: // fakeDriver is a fake database that implements Go's dbimpl.Driver lol. most. elaborate. test. case. ever. love it. with a bit of a parser, you could almost extend this to be a basic SQL engine and include it in the stdlib. http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/fakedb_test.go... src/pkg/exp/db/fakedb_test.go:510: } This file embodies lots of the reasons that I love Go.
Sign in to reply to this message.
On 3 September 2011 03:04, <bradfitz@golang.org> wrote: > Description: > exp/db{,/dbimpl}: new database packages > > Please review this at http://codereview.appspot.com/4973055/ Here's a bunch of half-baked thoughts. Other than a comment on licensing, I've based it more on the API docs than the actual implementation (which I've only skimmed). The code should be BSD-style license, not Apache. It seems odd that most of package db's types are structs (with no exported fields) and not interfaces. Especially since DB, Stmt and Tx all share Exec, Query and QueryRow methods. I would rename Rows to Iter or Iterator. Also, I'd paint the bike shed Txn instead of Tx, and s/dest/dst/g. If I squint, it might be possible for the App Engine datastore to fit these interfaces (it would be a GQL database instead of an SQL database). AE "row" keys are a structured type and not a string or int, but maybe it will still work if a datastore.Key implements ScannerInto. It's not as if Row has a Name() string method. Transactions in the App Engine datastore are represented as a retry-able func, instead of explicit Begin/Commit/Rollback calls. I'm not 100% sure that the functional approach is best, but it is something a little higher level that's not possible in C/C++ style APIs SQL Noob question #0: if I understand your API, I can't run a Stmt in a Tx, I have to parse it for every transaction by calling Tx.Prepare. Should Exec and Query take a Stmt instead of a string as their first arg? SQL Noob question #1: do we need to distinguish between Exec and Query? Can we just run statements, some of which have zero results? Tx.Exec has a different return type than DB.Exec or Stmt.Exec. Finally, the Rows interface and usage are: ---- // Yeah, it's actually a struct, but for argument's sake... type Rows interface { Close() os.Error Error() os.Error Next() bool Scan(dest ...interface{}) os.Error } ---- rows, err := db.Query("SELECT ...") ... for rows.Next() { var id int var name string err = rows.Scan(&id, &name) ... } err = rows.Error() ---- First, is there a need for Rows to have a separate Error and Close methods? Finally, I'll compare these with iterating over the AE datastore, and iterating over a LevelDB Go port that I'm working on (LevelDB is a key/value store). I'm not saying that these other designs are necessarily better, and neither of them are set in stone. Still, their experience may be instructive in exploring the design space. The App Engine iterator interface and usage currently are: ---- type Iterator interface { Next(dst interface{}) (*Key, os.Error) } ---- iter := query.Run(context) for { var x Widget key, err := iter.Next(&x) if err == datastore.Done { return nil } if err != nil { return err } doSomethingWith(key, &x) } ---- The LevelDB iterator interface and usage currently are: ---- type Iterator interface { Next() bool Key() []byte Value() []byte Close() os.Error } ---- iter := db.Seek(key) for iter.Next() { doSomethingWith(iter.Key(), iter.Value()) } return iter.Close() ---- For AppEngine, I didn't think of having separate Next and Scan methods. That design is growing on me. For LevelDB, those that create Iterators (via calling DB.Seek) are required to Close them, to avoid resource leaks. For both, neither Query.Run and DB.Seek returns an error. If something bad occurs, they returns an errorIterator instead: an implementation of the Iterator interface that has no results but that error.
Sign in to reply to this message.
(replies to some comments below; others later) On Fri, Sep 2, 2011 at 6:49 PM, Nigel Tao <nigeltao@golang.org> wrote: > On 3 September 2011 03:04, <bradfitz@golang.org> wrote: > > Description: > > exp/db{,/dbimpl}: new database packages > > > > Please review this at http://codereview.appspot.com/4973055/ > > Here's a bunch of half-baked thoughts. Other than a comment on > licensing, I've based it more on the API docs than the actual > implementation (which I've only skimmed). > > The code should be BSD-style license, not Apache. > whoops. I was prototyping this elsewhere. will fix. > It seems odd that most of package db's types are structs (with no > exported fields) and not interfaces. Especially since DB, Stmt and Tx > all share Exec, Query and QueryRow methods. > by design. pkg dbimpl is interfaces and pkg db is structs. > Transactions in the App Engine datastore are represented as a > retry-able func, instead of explicit Begin/Commit/Rollback calls. I'm > not 100% sure that the functional approach is best, but it is > something a little higher level that's not possible in C/C++ style > APIs > That could be added on top, too. I'm torn. I love abusing functional style, but I'm also not a huge fan of that API in App Engine. It leads to people doing non-database things in their transaction functions. > SQL Noob question #0: if I understand your API, I can't run a Stmt in > a Tx, I have to parse it for every transaction by calling Tx.Prepare. > Should Exec and Query take a Stmt instead of a string as their first > arg? > Tx stuff isn't implemented and is mostly from old notes. it's probably behind. but you talking about db or dbimpl? > SQL Noob question #1: do we need to distinguish between Exec and > Query? Can we just run statements, some of which have zero results? > it lets drivers do different things if desired to not send results back over the network if the caller's intentions is to discard them anyway. > Tx.Exec has a different return type than DB.Exec or Stmt.Exec. > originally in the notes exec was just returning os.Error, but then during implementation I found a result was desired. just didn't get around to implementing Tx yet.
Sign in to reply to this message.
On 3 September 2011 12:01, Brad Fitzpatrick <bradfitz@golang.org> wrote: >> It seems odd that most of package db's types are structs (with no >> exported fields) and not interfaces. Especially since DB, Stmt and Tx >> all share Exec, Query and QueryRow methods. > > by design. pkg dbimpl is interfaces and pkg db is structs. I know that it's deliberate. It still feels wrong to me. I feel like I should be able to make a fake DB type without needing to go through db/dbimpl. > Tx stuff isn't implemented and is mostly from old notes. it's probably > behind. > but you talking about db or dbimpl? I've looked mostly at db, since that's the consumer-facing API. >> SQL Noob question #1: do we need to distinguish between Exec and >> Query? Can we just run statements, some of which have zero results? > > it lets drivers do different things if desired to not send results back over > the network if the caller's intentions is to discard them anyway. And that's not representable by the query language? Like I said, I'm a SQL Noob...
Sign in to reply to this message.
On 3 September 2011 12:12, Nigel Tao <nigeltao@golang.org> wrote: > I know that it's deliberate. It still feels wrong to me. I feel like I > should be able to make a fake DB type without needing to go through > db/dbimpl. A stronger statement would be that, with this design, I can't implement a DB without going through db/dbimpl. If I wanted to make the App Engine datastore fit this model, I wouldn't be able to make a type assertion that the db.Row that I get from the AppEngine driver has a Key method as well as a Scan method. Oversimplifying the argument: it's weird that the 'implementation' package has the interfaces but the public-facing Application Programming Interface package has the structs. Although that's probably a better argument for renaming s/dbimpl/driver/.
Sign in to reply to this message.
On Fri, Sep 2, 2011 at 7:37 PM, Nigel Tao <nigeltao@golang.org> wrote: > On 3 September 2011 12:12, Nigel Tao <nigeltao@golang.org> wrote: > > I know that it's deliberate. It still feels wrong to me. I feel like I > > should be able to make a fake DB type without needing to go through > > db/dbimpl. > > A stronger statement would be that, with this design, I can't > implement a DB without going through db/dbimpl. If I wanted to make > the App Engine datastore fit this model, I wouldn't be able to make a > type assertion that the db.Row that I get from the AppEngine driver > has a Key method as well as a Scan method. > The point of making them structs was so we had a way to add convenience wrapper stuff like QueryRow: http://p.danga.com:6060/src/pkg/exp/db/db.go?s=5797:5859#L217 You wouldn't want QueryRow part of the interface, since it's redundant with Query. But we want stuff like that (and probably more over time) in package db because it's super nice. If you want to do driver-specific stuff, use the Driver() method and type-assert on that. Would you be happy if the db package had an OpenDriver(impl driver.Driver)? Then you don't need to register a driver name at least?
Sign in to reply to this message.
http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go File src/pkg/exp/db/db.go (right): http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode35 src/pkg/exp/db/db.go:35: func Register(name string, driver dbimpl.Driver) { If it's not intended for end user use, shouldn't it go to dbimpl (with the associated driver query function)? http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode83 src/pkg/exp/db/db.go:83: var ErrNoRows = os.NewError("db: no rows in result set") Don't we want to prevent users from assigning to it (like they do with os.Stdout)? http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode346 src/pkg/exp/db/db.go:346: if _, match = s.db.connIfFree(cs.ci); match { What is the expected number of s.css and db.freeConn?
Sign in to reply to this message.
Your Rows type is more of a Cursor or Iter then a Rows struct. For most SQL applications, I tend to buffer my result set rather then iterating it directly with an open connection. Thus, Stmt.Fill() returns Table with all data in it. I can then return and close the connection and put my logic elsewhere. I like the driver register design. bikeshed: the api is full of abbreviations. I can type Statement faster then Stmt. I'd prefer if they were full words.
Sign in to reply to this message.
On Fri, Sep 2, 2011 at 22:37, Nigel Tao <nigeltao@golang.org> wrote: > A stronger statement would be that, with this design, I can't > implement a DB without going through db/dbimpl. That is by design. There's a lot about data marshaling and general API consistency that is subtle and that we want implemented in exactly one place. sql/driver defines what the database has to supply as basic operations, and then sql builds the nicer API on top of that. In this way sql/driver is kind of like io.Reader and sql is more like io.ReadFull. Russ
Sign in to reply to this message.
* Brad Fitzpatrick: > * To type casting/conversions consistently between all drivers. To > achieve this, most of the type conversions are done in the db > package, not in each driver. The drivers then only have to deal > with a smaller set of types. Both PostgreSQL and SQLite need to distinguish between strings (encoded in UTF-8) and blobs. > * Be flexible with type conversions, but be paranoid about silent > truncation or other loss of precision. There's no good match in Go for the NUMERIC type right now. > * Handle concurrency well. Users shouldn't need to care about the > database's per-connection thread safety issues (or lack thereof), > and shouldn't have to maintain their own free pools of connections. > The 'db' package should deal with that bookkeeping as needed. Given > a *db.DB, it should be possible to share that instance between > multiple goroutines, without any extra synchronization. I think there's still a need to obtain a specific connection, without starting a transaction: temporary tables are tied to a specific connection, and some database statements cannot run in a transaction. > * Push complexity, where necessary, down into the db+dbimpl packages, > rather than exposing it to users. Said otherwise, the db package > should expose an ideal database that's not finnicky about how it's > accessed, even if that's not true. Some things are impossible to hide, like serialization failures. There's currently no way to check if an error will likely go away when a transaction is retried.
Sign in to reply to this message.
On 3 September 2011 13:17, Brad Fitzpatrick <bradfitz@golang.org> wrote: > The point of making them structs was so we had a way to add convenience > wrapper stuff like QueryRow: > http://p.danga.com:6060/src/pkg/exp/db/db.go?s=5797:5859#L217 The counterpoint is that if db.Row is a struct, then even if foo.Row embeds a db.Row, I can't convert from the latter to the former (even if I have a Driver instance). You could if both were interfaces. I still think that you can have structs and interfaces in the sql/driver package that handles the subtleties for driver implementations, but end-users of the sql package can program to interfaces. But this isn't my package. I'll get out of the kitchen; there's more than enough cooks already.
Sign in to reply to this message.
On 3 September 2011 11:49, Nigel Tao <nigeltao@golang.org> wrote: > // Yeah, it's actually a struct, but for argument's sake... > type Rows interface { > Close() os.Error > Error() os.Error > Next() bool > Scan(dest ...interface{}) os.Error > } On a tangent, but... some iterators support backwards iteration, but there are two semantic models for this: whether iterators are at a row, or are between rows. Suppose my query returns three rows (A, B, C), and on my iterator (or what you've named Rows) I call Next, Next, Prev, Scan. With the "at" model, Scan returns the data where the iterator is at: The iterator starts to the left of A. Next moves it to A. Next moves it to B. Prev moves it to A. Scan returns A. With the "between" model, Scan returns the data most recently walked over: The iterator starts at the "$" in "$ABC". Next moves it to "A$BC", having walked over A. Next moves it to "AB$C", having walked over B. Prev moves it to "A$BC", having walked over B. Scan returns B. Which model seems more intuitive to golang-dev? Sure, this question is hypothetical, since the proposed db or sql package doesn't provide backwards iteration, but it would be nice if the iteration model was consistent across other Go database packages. Getting back to the API design in the original CL, an alternative to a Next() bool method is to have two methods: Done() bool Next() and the typical loop changes from: for rows.Next() { // do stuff with rows.Scan. } to for ; !rows.Done(); rows.Next() { // do stuff with rows.Scan. } Note that the two differ in that the first Next call happens before the first Scan call in the former but after in the latter. Does the answer to the "are iterators at or between" question influence the answer to the "do iterators loop for Next or for ;!Done;Next" question?
Sign in to reply to this message.
I think you are reasoning backwards.
The goal is to have a nice, easy-to-use API, hence
for rows.Next() {
rows.Scan...
}
We can assign whatever semantics to Next
that we like as long as it produces that pattern.
If you need backward iteration, I would say that
it should be asked for explicitly as part of the
setup rather than by calling different methods
during the traversal itself (like SQL's 'DESC').
If the rows come out backwards, that's fine;
the loop shouldn't change.
Russ
Sign in to reply to this message.
On 7 September 2011 12:04, Russ Cox <rsc@golang.org> wrote: > The goal is to have a nice, easy-to-use API, hence > > for rows.Next() { > rows.Scan... > } > > We can assign whatever semantics to Next > that we like as long as it produces that pattern. I agree that this is a nice API. Either "at" or "between" iterators will work with this API. I would like leveldb-go to provide a similarly nice API, but leveldb supports backwards iteration (because it's needed by e.g. Chromium's IndexedDB support), and it was unclear to me how Prev should work. However, I had not thought of specifying the iteration direction at setup instead of traversal. That might simply be the nicest solution.
Sign in to reply to this message.
http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go File src/pkg/exp/db/db.go (right): http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode61 src/pkg/exp/db/db.go:61: Ok bool // Ok is true if the String is not NULL On 2011/09/03 01:26:10, kevlar wrote: > I would call this NotNull? Done, but it feels a bit funny in expressions like: if !s.NotNull { (double negative) We might want to name it "Null". I'm happy with Ok, Null, or NotNull. http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode83 src/pkg/exp/db/db.go:83: var ErrNoRows = os.NewError("db: no rows in result set") On 2011/09/03 10:17:54, dvyukov wrote: > Don't we want to prevent users from assigning to it (like they do with > os.Stdout)? I'm not going to fight the language in this CL. http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode297 src/pkg/exp/db/db.go:297: func (s *Stmt) Exec(args ...interface{}) (Result, os.Error) { On 2011/09/03 01:26:10, kevlar wrote: > Exec is envious of Query's comment. Done. http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode399 src/pkg/exp/db/db.go:399: // always non-nil. On 2011/09/03 01:26:10, kevlar wrote: > Provide or refer to the example of chaining this with Scan? Done.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, gustavo@niemeyer.net, rsc@golang.org, borman@google.com, dave@cheney.net, kevlar@google.com, nigeltao@golang.org, dvyukov@google.com, kardianos@gmail.com, fw@deneb.enyo.de (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go File src/pkg/exp/sql/sql.go (right): http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:45: // NullableString is type representing a string which may be s/which/that/ http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:61: NotNull bool // NotNull is true if String is not NULL i dislike negative names. you always end up negating them again in expressions. it's negative so the zero type is an empty string? how about Valid?
Sign in to reply to this message.
On Thu, Sep 15, 2011 at 5:12 PM, <r@golang.org> wrote: > > http://codereview.appspot.com/**4973055/diff/28001/src/pkg/** > exp/sql/sql.go#newcode61<http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newcode61> > src/pkg/exp/sql/sql.go:61: NotNull bool // NotNull is true if String is > not NULL > i dislike negative names. you always end up negating them again in > expressions. > agree. I already did that in the test code and it looks gross. > it's negative so the zero type is an empty string? how about Valid? Isn't Valid just a long way of spelling Ok? (which is how I had it in v0, and seems like the Go word....)
Sign in to reply to this message.
On Sep 15, 2011, at 5:14 PM, Brad Fitzpatrick wrote: > On Thu, Sep 15, 2011 at 5:12 PM, <r@golang.org> wrote: > > http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... > src/pkg/exp/sql/sql.go:61: NotNull bool // NotNull is true if String is > not NULL > i dislike negative names. you always end up negating them again in > expressions. > > agree. I already did that in the test code and it looks gross. > > it's negative so the zero type is an empty string? how about Valid? > > Isn't Valid just a long way of spelling Ok? (which is how I had it in v0, and seems like the Go word....) I prefer Valid. We use it in reflect for essentially the same idea. OK is the word (not Ok) and anyway we use it as a variable name for various things. This is asking if the string is valid, which is a narrower meaning. Wow, today really is all about how to bikeshed. We should start a discussion group about the best way to bikeshed. -rob
Sign in to reply to this message.
Valid it is. There are a few thousand other lines to review too. On Thu, Sep 15, 2011 at 5:15 PM, Rob 'Commander' Pike <r@google.com> wrote: > > On Sep 15, 2011, at 5:14 PM, Brad Fitzpatrick wrote: > > > On Thu, Sep 15, 2011 at 5:12 PM, <r@golang.org> wrote: > > > > > http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... > > src/pkg/exp/sql/sql.go:61: NotNull bool // NotNull is true if String is > > not NULL > > i dislike negative names. you always end up negating them again in > > expressions. > > > > agree. I already did that in the test code and it looks gross. > > > > it's negative so the zero type is an empty string? how about Valid? > > > > Isn't Valid just a long way of spelling Ok? (which is how I had it in > v0, and seems like the Go word....) > > I prefer Valid. We use it in reflect for essentially the same idea. OK is > the word (not Ok) and anyway we use it as a variable name for various > things. This is asking if the string is valid, which is a narrower meaning. > > Wow, today really is all about how to bikeshed. We should start a > discussion group about the best way to bikeshed. > > -rob > >
Sign in to reply to this message.
I don't know SQL worth a damn but I'll look it over for second-order stuff.
Sign in to reply to this message.
http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go File src/pkg/exp/sql/sql.go (right): http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:92: freeConn []driver.Conn So you are maintaining an underlying connection pool and not exposing the individual connections to the client. How does this work with transactions, which are only valid on a single connection? http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:235: func (db *DB) Begin() (*Tx, os.Error) { There are different types of transactions (e.g. deferred, immediate, exclusive). How about Begin(arg string)? http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:255: func (tx *Tx) Rollback() os.Error { When a DB supports nested transactions, you can do partial rollbacks to named savepoints: http://www.sqlite.org/lang_savepoint.html (This is common with PostgreSQL, Oracle, MSSQL, etc.) http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:462: func (rs *Rows) Next() bool { Probably best left for some later version of the API, but fancy DBs have different kinds of cursors. The classic also has Prev, First, Last, which you'll find in the original JDBC API. Another common cursor power is Count.
Sign in to reply to this message.
Only looking at public API. More comments in driver would help. My main concern right now is that the value conversion code seems to be scattered, possibly too general. I'd like to make sure we end up with something where you don't have different drivers supporting different kinds of values. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/convert.go File src/pkg/exp/sql/convert.go (right): http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/convert.go#n... src/pkg/exp/sql/convert.go:4: Licensed under the Apache License, Version 2.0 (the "License"); Copyright notices are still wrong. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... File src/pkg/exp/sql/driver/driver.go (right): http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:38: // The dsn parameter, the Data Source Name, contains a // The name is a string in a driver-specific format. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:44: Open(dsn string) (Conn, os.Error) s/dsn/name/ http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:50: // If not implemented by a Driver, the sql package's DB.Exec method the implicit noun here sounds like it is 'the sql package's DB.Exec method'. // If a Driver does not implement Execer, the sql package's DB.Exec method http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:55: // If not implemented by a Conn, the db package's DB.Exec will first // If a Conn does not implement Execer, the db package's DB.Exec will ... http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:62: type Conn interface { more comments... http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:90: // ColumnConverter may be optionally implemented by Stmt to signal // If a Stmt implements ColumnConverter, the sql package calls // its ColumnConverter method to perform type conversions. or whatever, but is this really necessary? Part of the rationale for having a central sql package is so that the conversions are in one place. I might not be as worried if you define what ValueConverter must be able to do. Also, is it necessary to have two different methods? You could instead have type Converter interface { Convert(index int, v interface{}) (interface{}, os.Error) } Maybe leave this out for round 1. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:93: ColumnCoverter(idx int) ValueConverter Converter is spelled wrong http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:125: var DDLSuccess Result = ddlSuccess{} ? http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go File src/pkg/exp/sql/sql.go (right): http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:32: // Register makes a database driver available by the provided name. RegisterDriver please. It's not supposed to be called from user code so a more precise name is fine (and makes it less likely that people will think they need to use it). http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:39: if _, dup := drivers[name]; dup { or if drivers[name] != nil http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:80: // ErrNoRows is returned by Scan when QueryRow doesn't return a This is kind of awkward. I think it's okay to break the leading name rule here to be less awkward. (Using complete sentences makes this possible.) // When QueryRow finds no rows, it returns a placeholder *Row; // that row's Scan method returns ErrNoRows. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:89: dsn string s/dsn/name/ http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:99: // Most users will open a database via a driver-specific connection Why? http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:101: func Open(driverName, dataSourceName string) (*DB, os.Error) { func Open(driver, name string) (*DB, os.Error) { http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:103: if !ok { or driver == nil http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:156: func (db *DB) Prepare(query string) (*Stmt, os.Error) { Comments on public API functions? http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:240: // This is non-portable and should only be used when Delete this sentence? It's not clear what non-portable means and "when needed" is of course a matter of judgment. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:297: // Exec executes a prepared statement with the provided placeholder arguments // Exec executes a prepared statement with the given arguments and // returns a Result summarizing the effect of the statement. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:375: // Query executes a prepared statement with the provided placeholder arguments // Query executes a prepared query statement with the given arguments // and returns the query results as a *Rows. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:399: // QueryRow wraps Query, for use in selecting a single row. Any errors // QueryRow executes a prepared query statement with the given arguments. // If an error occurs during the execution of the statement, that error will // be returned by a call to Scan on the returned *Row, which is always non-nil. // If the query selects no rows, the *Row's Scan will return ErrNoRows. // Otherwise, the *Row's Scan scans the first selected row and discards // the rest. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:459: // Next advances the Rows' cursor (which starts before the first ' without s is only for plurals. Rows is a singular here. but we can avoid the term cursor entirely, to avoid people thinking this is a JDBC cursor. // Next prepares the next result row for reading with the Scan method. // It returns true on success, false if there is no next result row. // Every call to Scan, even the first one, must be preceded by a call to Next. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:484: // Scan copies the columns in the current row into variables referened in dest. into the values pointed at by dest. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:520: // Row is the result of calling QueryRow to select a single row. If no I think you can drop the second sentence, leaving it for QueryRow http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:529: // Scan copies the columns from the matched row into the variables // Scan copies the columns from the matched row into the values // pointed at by dest. If more than one row matches the query, // Scan uses the first row and discards the rest. If no row matches // the query, Scan returns ErrNoRows. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:542: type Result interface { // A Result summarizes an executed SQL command.
Sign in to reply to this message.
http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... File src/pkg/exp/sql/driver/driver.go (right): http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:38: // The dsn parameter, the Data Source Name, contains a On 2011/09/16 15:17:58, rsc wrote: > // The name is a string in a driver-specific format. Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:44: Open(dsn string) (Conn, os.Error) On 2011/09/16 15:17:58, rsc wrote: > s/dsn/name/ Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:50: // If not implemented by a Driver, the sql package's DB.Exec method On 2011/09/16 15:17:58, rsc wrote: > the implicit noun here sounds like it is 'the sql package's DB.Exec method'. > > // If a Driver does not implement Execer, the sql package's DB.Exec method Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:55: // If not implemented by a Conn, the db package's DB.Exec will first On 2011/09/16 15:17:58, rsc wrote: > // If a Conn does not implement Execer, the db package's DB.Exec will ... Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:62: type Conn interface { On 2011/09/16 15:17:58, rsc wrote: > more comments... Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:90: // ColumnConverter may be optionally implemented by Stmt to signal On 2011/09/16 15:17:58, rsc wrote: > // If a Stmt implements ColumnConverter, the sql package calls > // its ColumnConverter method to perform type conversions. > > or whatever, but is this really necessary? > Part of the rationale for having a central sql package > is so that the conversions are in one place. > I might not be as worried if you define what ValueConverter > must be able to do. > > Also, is it necessary to have two different methods? > You could instead have > > type Converter interface { > Convert(index int, v interface{}) (interface{}, os.Error) > } > > Maybe leave this out for round 1. Cleaned this up. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:93: ColumnCoverter(idx int) ValueConverter On 2011/09/16 15:17:58, rsc wrote: > Converter is spelled wrong Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:125: var DDLSuccess Result = ddlSuccess{} On 2011/09/16 15:17:58, rsc wrote: > ? documented. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go File src/pkg/exp/sql/sql.go (right): http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:156: func (db *DB) Prepare(query string) (*Stmt, os.Error) { On 2011/09/16 15:17:58, rsc wrote: > Comments on public API functions? Done x 15 or so. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:235: func (db *DB) Begin() (*Tx, os.Error) { On 2011/09/16 04:16:58, crawshaw wrote: > There are different types of transactions (e.g. deferred, immediate, exclusive). > How about Begin(arg string)? We'll keep Begin() for the common case (driver-preferred default) but we can add a BeginXxx(someType) method in the future. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:240: // This is non-portable and should only be used when On 2011/09/16 15:17:58, rsc wrote: > Delete this sentence? It's not clear what non-portable means > and "when needed" is of course a matter of judgment. Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:255: func (tx *Tx) Rollback() os.Error { On 2011/09/16 04:16:58, crawshaw wrote: > When a DB supports nested transactions, you can do partial rollbacks to named > savepoints: > > http://www.sqlite.org/lang_savepoint.html > > (This is common with PostgreSQL, Oracle, MSSQL, etc.) Let's do this also in round 2. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:297: // Exec executes a prepared statement with the provided placeholder arguments On 2011/09/16 15:17:58, rsc wrote: > // Exec executes a prepared statement with the given arguments and > // returns a Result summarizing the effect of the statement. Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:375: // Query executes a prepared statement with the provided placeholder arguments On 2011/09/16 15:17:58, rsc wrote: > // Query executes a prepared query statement with the given arguments > // and returns the query results as a *Rows. Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:399: // QueryRow wraps Query, for use in selecting a single row. Any errors On 2011/09/16 15:17:58, rsc wrote: > // QueryRow executes a prepared query statement with the given arguments. > // If an error occurs during the execution of the statement, that error will > // be returned by a call to Scan on the returned *Row, which is always non-nil. > // If the query selects no rows, the *Row's Scan will return ErrNoRows. > // Otherwise, the *Row's Scan scans the first selected row and discards > // the rest. Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:459: // Next advances the Rows' cursor (which starts before the first On 2011/09/16 15:17:58, rsc wrote: > ' without s is only for plurals. Rows is a singular here. My memory and the Internet seem to imply it's either way with singular possessive ending in es. But the preferred way seems to be /'s$/. > but we can avoid the term cursor entirely, to avoid > people thinking this is a JDBC cursor. SGTM > > // Next prepares the next result row for reading with the Scan method. > // It returns true on success, false if there is no next result row. > // Every call to Scan, even the first one, must be preceded by a call to Next. > Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:484: // Scan copies the columns in the current row into variables referened in dest. On 2011/09/16 15:17:58, rsc wrote: > into the values pointed at by dest. Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:520: // Row is the result of calling QueryRow to select a single row. If no On 2011/09/16 15:17:58, rsc wrote: > I think you can drop the second sentence, leaving it for QueryRow Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:529: // Scan copies the columns from the matched row into the variables On 2011/09/16 15:17:58, rsc wrote: > // Scan copies the columns from the matched row into the values > // pointed at by dest. If more than one row matches the query, > // Scan uses the first row and discards the rest. If no row matches > // the query, Scan returns ErrNoRows. Done. http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:542: type Result interface { On 2011/09/16 15:17:58, rsc wrote: > // A Result summarizes an executed SQL command. Done.
Sign in to reply to this message.
Hello gustavo@niemeyer.net, rsc@golang.org, borman@google.com, dave@cheney.net, kevlar@google.com, nigeltao@golang.org, dvyukov@google.com, kardianos@gmail.com, fw@deneb.enyo.de, r@golang.org, r@google.com, david.crawshaw@zentus.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM within my limited understanding. there's some nice stuff in here. the interfaces are somewhat inconsistently described. the type should be presented quickly, then each method described thoroughly. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go File src/pkg/exp/sql/convert.go (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go#n... src/pkg/exp/sql/convert.go:28: // copyConvert copies to dest the value in src, converting it if possible s/$/./ http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go#n... src/pkg/exp/sql/convert.go:46: *d = s this isn't a copy. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt File src/pkg/exp/sql/doc.txt (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt#newc... src/pkg/exp/sql/doc.txt:23: * To type casting/conversions consistently between all drivers. To i can't parse the first sentence. (maybe just s/To t/T/ so it's not an infinitive, in parallel with the other entries.) maybe Make type casting and conversion consistent between all drivers. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt#newc... src/pkg/exp/sql/doc.txt:24: achieve this, most of the type conversions are done in the db s/type // http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... File src/pkg/exp/sql/driver/driver.go (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:38: // Driver is the interface that must be implemented by database s/by/by a/ http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:55: // drivers which can provide a more effcient implementation. s/which/that/ s/effcient/efficient/ http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:86: LastInsertId() (int64, os.Error) document these. the off-by-oneness should be documented too http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:101: // type as defined above. second sentence doesnt' parse. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:104: // Exec executes a query that may rows, such as a SELECT. The "that may rows"? "may return rows"? http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:122: // Columns returns the name of the columns. The number of s/name/names/ http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:125: // string should be returned. s/.$/ for that entry./ http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/types.go File src/pkg/exp/sql/driver/types.go (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/types... src/pkg/exp/sql/driver/types.go:76: return nil, fmt.Errorf("unsupported value %v (type %T) converting to int32", v, v) here and throughout the errors could start with "sql: " (or other name as appropriate) http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/types... src/pkg/exp/sql/driver/types.go:117: // IsScanSubsetType returns if v is of a valid type for a s/returns if/reports whether/ http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go File src/pkg/exp/sql/sql.go (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:34: // a duplicate name. instead of the second sentence: // If Register is called twice with the same name or if driver is nil, it panics. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:45: // NullableString is type representing a string which may be s/type/a type/ or jsut NullableString represents s/which/that/ http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:46: // null. NullableString implements the ScannerInto inteface so can be s/can/it can/ http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:74: // ScannerInto is an interface used by Scan. this seems to be the crux, so i suggest a better doc here and a proper description of ScanInto within this type declaration http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:181: // Exec executes a query without returning any rows. i don't know what this means, but i'm not a db expert. does it mean that the query cannot return rows, or that we don't care if it does? http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:298: type Stmt struct { some of these names are short, like Exec and Stmt. that jangles a bit, but it's probably ok. is this nomenclature what people expect? http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:308: func todo() string { lovely http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:529: // if encountered normally the Rows are closed automatically. Calling s/if/is/ ? s/normally/&, / s/Calling//
Sign in to reply to this message.
All done. Russ, thoughts on this as a checkpoint? http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go File src/pkg/exp/sql/convert.go (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go#n... src/pkg/exp/sql/convert.go:28: // copyConvert copies to dest the value in src, converting it if possible On 2011/09/29 01:28:37, r wrote: > s/$/./ Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go#n... src/pkg/exp/sql/convert.go:46: *d = s On 2011/09/29 01:28:37, r wrote: > this isn't a copy. It shouldn't. I've renamed the function to "convertAssign" This part (scanning lots of rows quickly from a table) should be able to work purely in []byte end-to-end, without copies. Added some docs, too. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt File src/pkg/exp/sql/doc.txt (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt#newc... src/pkg/exp/sql/doc.txt:1: Goals of the db and db/dbimpl packages: updated all the db & dbimpl to sql & driver, too http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt#newc... src/pkg/exp/sql/doc.txt:23: * To type casting/conversions consistently between all drivers. To On 2011/09/29 01:28:37, r wrote: > i can't parse the first sentence. (maybe just s/To t/T/ so it's not an > infinitive, in parallel with the other entries.) > maybe > Make type casting and conversion consistent between all drivers. Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt#newc... src/pkg/exp/sql/doc.txt:23: * To type casting/conversions consistently between all drivers. To On 2011/09/29 01:28:37, r wrote: > i can't parse the first sentence. (maybe just s/To t/T/ so it's not an > infinitive, in parallel with the other entries.) > maybe > Make type casting and conversion consistent between all drivers. Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/doc.txt#newc... src/pkg/exp/sql/doc.txt:24: achieve this, most of the type conversions are done in the db On 2011/09/29 01:28:37, r wrote: > s/type // Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... File src/pkg/exp/sql/driver/driver.go (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:38: // Driver is the interface that must be implemented by database On 2011/09/29 01:28:37, r wrote: > s/by/by a/ Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:55: // drivers which can provide a more effcient implementation. On 2011/09/29 01:28:37, r wrote: > s/which/that/ > s/effcient/efficient/ Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:86: LastInsertId() (int64, os.Error) On 2011/09/29 01:28:37, r wrote: > document these. the off-by-oneness should be documented too Done. But... what off-by-oneness? http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:101: // type as defined above. On 2011/09/29 01:28:37, r wrote: > second sentence doesnt' parse. Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:104: // Exec executes a query that may rows, such as a SELECT. The On 2011/09/29 01:28:37, r wrote: > "that may rows"? "may return rows"? Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:122: // Columns returns the name of the columns. The number of On 2011/09/29 01:28:37, r wrote: > s/name/names/ Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:125: // string should be returned. On 2011/09/29 01:28:37, r wrote: > s/.$/ for that entry./ Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/types.go File src/pkg/exp/sql/driver/types.go (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/types... src/pkg/exp/sql/driver/types.go:76: return nil, fmt.Errorf("unsupported value %v (type %T) converting to int32", v, v) On 2011/09/29 01:28:37, r wrote: > here and throughout the errors could start with "sql: " (or other name as > appropriate) Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/types... src/pkg/exp/sql/driver/types.go:117: // IsScanSubsetType returns if v is of a valid type for a On 2011/09/29 01:28:37, r wrote: > s/returns if/reports whether/ Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go File src/pkg/exp/sql/sql.go (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:34: // a duplicate name. On 2011/09/29 01:28:37, r wrote: > instead of the second sentence: > // If Register is called twice with the same name or if driver is nil, it > panics. Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:45: // NullableString is type representing a string which may be On 2011/09/29 01:28:37, r wrote: > s/type/a type/ > or jsut > NullableString represents > > s/which/that/ Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:46: // null. NullableString implements the ScannerInto inteface so can be On 2011/09/29 01:28:37, r wrote: > s/can/it can/ Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:74: // ScannerInto is an interface used by Scan. On 2011/09/29 01:28:37, r wrote: > this seems to be the crux, so i suggest a better doc here and a proper > description of ScanInto within this type declaration Done. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:181: // Exec executes a query without returning any rows. On 2011/09/29 01:28:37, r wrote: > i don't know what this means, but i'm not a db expert. does it mean that the > query cannot return rows, or that we don't care if it does? Expanded the comment. INSERT, UPDATE etc. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:298: type Stmt struct { On 2011/09/29 01:28:37, r wrote: > some of these names are short, like Exec and Stmt. that jangles a bit, but it's > probably ok. is this nomenclature what people expect? I don't know what people expect, nor have a preference. I think rsc proposed these names originally, but maybe whiteboard brevity was the cause. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:308: func todo() string { On 2011/09/29 01:28:37, r wrote: > lovely this was critical when I started with dozens of TODOs. it got hard to figure out. http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/sql.go#newco... src/pkg/exp/sql/sql.go:529: // if encountered normally the Rows are closed automatically. Calling On 2011/09/29 01:28:37, r wrote: > s/if/is/ ? > s/normally/&, / > s/Calling// Done.
Sign in to reply to this message.
Hello gustavo@niemeyer.net, rsc@golang.org, borman@google.com, dave@cheney.net, kevlar@google.com, nigeltao@golang.org, dvyukov@google.com, kardianos@gmail.com, fw@deneb.enyo.de, r@golang.org, r@google.com, david.crawshaw@zentus.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... File src/pkg/exp/sql/driver/driver.go (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/drive... src/pkg/exp/sql/driver/driver.go:86: LastInsertId() (int64, os.Error) insertion goes into the gaps. is LastInsertId the one we inserted, the one after, the one before, the one we'll do next, ....? is RowsAffected a count?
Sign in to reply to this message.
LGTM Fix copyright notices.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=d21caa7909f2 *** exp/sql{,/driver}: new database packages R=gustavo, rsc, borman, dave, kevlar, nigeltao, dvyukov, kardianos, fw, r, r, david.crawshaw CC=golang-dev http://codereview.appspot.com/4973055
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
