code review 5557063: database/sql: add NullInt64, NullFloat64, NullBool, Nul...

database/sql: add NullInt64, NullFloat64, NullBool

Fixes issue 2699

[coopernurse, 2012-01-20 00:27:19]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2012-01-25 16:34:44]

http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/fakedb_t...
File src/pkg/database/sql/fakedb_test.go (right):

http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/fakedb_t...
src/pkg/database/sql/fakedb_test.go:597: return
driver.NotNull{driver.DefaultParameterConverter}
this works, but a type-specific converter would be better. maybe add a TODO here
and in the below cases.

http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:73: // NullInt64 represents a int64 that may be
null.
an int64

http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:75: // it can be used as a scan destination, similar
to NullString
end in a period, here and below.

http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:154: type NullBytes struct {
is NullBytes actually useful? a []byte can already be nil. we can distinguish
between a nil []byte and a []byte{} empty slice.

[coopernurse, 2012-01-26 00:38:18]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[coopernurse, 2012-01-26 00:39:20]

PTAL

http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/fakedb_t...
File src/pkg/database/sql/fakedb_test.go (right):

http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/fakedb_t...
src/pkg/database/sql/fakedb_test.go:597: return
driver.NotNull{driver.DefaultParameterConverter}
On 2012/01/25 16:34:45, bradfitz wrote:
> this works, but a type-specific converter would be better. maybe add a TODO
here
> and in the below cases.

Done.

http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:73: // NullInt64 represents a int64 that may be
null.
On 2012/01/25 16:34:45, bradfitz wrote:
> an int64

Done.

http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:75: // it can be used as a scan destination, similar
to NullString
On 2012/01/25 16:34:45, bradfitz wrote:
> end in a period, here and below.

Done.

http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:154: type NullBytes struct {
On 2012/01/25 16:34:45, bradfitz wrote:
> is NullBytes actually useful? a []byte can already be nil. we can distinguish
> between a nil []byte and a []byte{} empty slice.

Ok, I'll remove it.  In my gorp integration tests I get an error when I try to
select a null column into a []byte, but I suspect this can be solved at the
mymysql driver level:

panic: sql: Scan error on column index 5: unsupported driver -> Scan pair: <nil>
-> *[]uint8

[bradfitz, 2012-01-26 00:49:59]

On Wed, Jan 25, 2012 at 4:39 PM, <jamespcooper@gmail.com> wrote:

> PTAL
>
>
>
> http://codereview.appspot.com/**5557063/diff/2001/src/pkg/**
>
database/sql/fakedb_test.go<http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/fakedb_test.go>
> File src/pkg/database/sql/fakedb_**test.go (right):
>
> http://codereview.appspot.com/**5557063/diff/2001/src/pkg/**
>
database/sql/fakedb_test.go#**newcode597<http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/fakedb_test.go#newcode597>
> src/pkg/database/sql/fakedb_**test.go:597: return
> driver.NotNull{driver.**DefaultParameterConverter}
> On 2012/01/25 16:34:45, bradfitz wrote:
>
>> this works, but a type-specific converter would be better. maybe add a
>>
> TODO here
>
>> and in the below cases.
>>
>
> Done.
>
>
> http://codereview.appspot.com/**5557063/diff/2001/src/pkg/**
>
database/sql/sql.go<http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go>
> File src/pkg/database/sql/sql.go (right):
>
> http://codereview.appspot.com/**5557063/diff/2001/src/pkg/**
>
database/sql/sql.go#newcode73<http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go#newcode73>
> src/pkg/database/sql/sql.go:**73: // NullInt64 represents a int64 that may
> be null.
> On 2012/01/25 16:34:45, bradfitz wrote:
>
>> an int64
>>
>
> Done.
>
>
> http://codereview.appspot.com/**5557063/diff/2001/src/pkg/**
>
database/sql/sql.go#newcode75<http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go#newcode75>
> src/pkg/database/sql/sql.go:**75: // it can be used as a scan destination,
> similar to NullString
> On 2012/01/25 16:34:45, bradfitz wrote:
>
>> end in a period, here and below.
>>
>
> Done.
>
>
> http://codereview.appspot.com/**5557063/diff/2001/src/pkg/**
>
database/sql/sql.go#newcode154<http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go#newcode154>
> src/pkg/database/sql/sql.go:**154: type NullBytes struct {
> On 2012/01/25 16:34:45, bradfitz wrote:
>
>> is NullBytes actually useful? a []byte can already be nil. we can
>>
> distinguish
>
>> between a nil []byte and a []byte{} empty slice.
>>
>
> Ok, I'll remove it.  In my gorp integration tests I get an error when I
> try to select a null column into a []byte, but I suspect this can be
> solved at the mymysql driver level:
>
> panic: sql: Scan error on column index 5: unsupported driver -> Scan
> pair: <nil> -> *[]uint8
>

Please file a separate bug on that.  I think that's easy to fix, but still
in the sql package.

[bradfitz, 2012-01-26 00:56:50]

LGTM but please first fix the commit description:

-- remove reference to NullBytes
-- blank line after the first line
-- end in "Fixes issue nnnnnn" (and don't reference the old issue id)


On Wed, Jan 25, 2012 at 4:39 PM, <jamespcooper@gmail.com> wrote:

> PTAL
>
>
>
> http://codereview.appspot.com/**5557063/diff/2001/src/pkg/**
>
database/sql/fakedb_test.go<http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/fakedb_test.go>
> File src/pkg/database/sql/fakedb_**test.go (right):
>
> http://codereview.appspot.com/**5557063/diff/2001/src/pkg/**
>
database/sql/fakedb_test.go#**newcode597<http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/fakedb_test.go#newcode597>
> src/pkg/database/sql/fakedb_**test.go:597: return
> driver.NotNull{driver.**DefaultParameterConverter}
> On 2012/01/25 16:34:45, bradfitz wrote:
>
>> this works, but a type-specific converter would be better. maybe add a
>>
> TODO here
>
>> and in the below cases.
>>
>
> Done.
>
>
> http://codereview.appspot.com/**5557063/diff/2001/src/pkg/**
>
database/sql/sql.go<http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go>
> File src/pkg/database/sql/sql.go (right):
>
> http://codereview.appspot.com/**5557063/diff/2001/src/pkg/**
>
database/sql/sql.go#newcode73<http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go#newcode73>
> src/pkg/database/sql/sql.go:**73: // NullInt64 represents a int64 that may
> be null.
> On 2012/01/25 16:34:45, bradfitz wrote:
>
>> an int64
>>
>
> Done.
>
>
> http://codereview.appspot.com/**5557063/diff/2001/src/pkg/**
>
database/sql/sql.go#newcode75<http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go#newcode75>
> src/pkg/database/sql/sql.go:**75: // it can be used as a scan destination,
> similar to NullString
> On 2012/01/25 16:34:45, bradfitz wrote:
>
>> end in a period, here and below.
>>
>
> Done.
>
>
> http://codereview.appspot.com/**5557063/diff/2001/src/pkg/**
>
database/sql/sql.go#newcode154<http://codereview.appspot.com/5557063/diff/2001/src/pkg/database/sql/sql.go#newcode154>
> src/pkg/database/sql/sql.go:**154: type NullBytes struct {
> On 2012/01/25 16:34:45, bradfitz wrote:
>
>> is NullBytes actually useful? a []byte can already be nil. we can
>>
> distinguish
>
>> between a nil []byte and a []byte{} empty slice.
>>
>
> Ok, I'll remove it.  In my gorp integration tests I get an error when I
> try to select a null column into a []byte, but I suspect this can be
> solved at the mymysql driver level:
>
> panic: sql: Scan error on column index 5: unsupported driver -> Scan
> pair: <nil> -> *[]uint8
>
>
http://codereview.appspot.com/**5557063/<http://codereview.appspot.com/5557063/>
>

[coopernurse, 2012-01-26 01:05:42]

PTAL

[coopernurse, 2012-01-26 01:47:32]

BTW, I logged a separate issue:

http://code.google.com/p/go/issues/detail?id=2788

with a failing test for the null value -> []byte conversion.  It looks like 
this convertAssign() is probably the place to deal with this, but I could 
misunderstand.

cheers

-- James

[bradfitz, 2012-01-26 01:47:39]

*** Submitted as http://code.google.com/p/go/source/detail?r=a3dddd7405a3 ***

database/sql: add NullInt64, NullFloat64, NullBool

Fixes issue 2699

R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/5557063

Committer: Brad Fitzpatrick <bradfitz@golang.org>