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: add NullTime support #30305

Closed
isakbm opened this issue Feb 18, 2019 · 10 comments
Closed

proposal: database/sql: add NullTime support #30305

isakbm opened this issue Feb 18, 2019 · 10 comments

Comments

@isakbm
Copy link

isakbm commented Feb 18, 2019

Would it be an idea to extend the Nullable types to include NullTime as well?

Something like

type NullTime struct {
    Time  time.Time
    Valid bool
}

// Scan implements the Scanner interface.
func (nt *NullTime) Scan(value interface{}) error {
    if value == nil {
        nt.Time, nt.Valid = time.Time{}, false
        return nil
    }
    nt.Valid = true
    return convertAssign(&nt.Time, value)
}

// Value implements the Valuer interface.
func (nt NullTime) Valuer() (driver.Value, error) {
    if !nt.Valid {
        return nil, nil
    }
    return nt.Time, nil
}

Just copy paste into ...go/src/database/sql/sql.go, should work.

@odeke-em odeke-em changed the title database/sql: add NullTime support proposal: database/sql: add NullTime support Feb 19, 2019
@gopherbot gopherbot added this to the Proposal milestone Feb 19, 2019
@agnivade
Copy link
Contributor

This seems to be taken from https://godoc.org/github.com/lib/pq#NullTime ?

@isakbm
Copy link
Author

isakbm commented Feb 19, 2019

@agnivade

That's interesting, looks similar, but I was just mimicking the way NullString is handled in go/src/database/sql/sql.go

See this line

pasted here for ease :

// from go/src/database/sql/sql.go

type NullString struct {
	String string
	Valid  bool // Valid is true if String is not NULL
}

// Scan implements the Scanner interface.
func (ns *NullString) Scan(value interface{}) error {
	if value == nil {
		ns.String, ns.Valid = "", false
		return nil
	}
	ns.Valid = true
	return convertAssign(&ns.String, value)
}

// Value implements the driver Valuer interface.
func (ns NullString) Value() (driver.Value, error) {
	if !ns.Valid {
		return nil, nil
	}
	return ns.String, nil
}

The implementation in pq is different, note how in pq there is no call to convertAssign.

@mattn
Copy link
Member

mattn commented Feb 19, 2019

convertAssign will need to look some possible time format for each RDBMS's serialization formats like below.

https://github.com/mattn/go-nulltype/blob/3e10e98e82afdeaf9510344bc71d4d9b8eea788f/time.go#L45-L56

@isakbm
Copy link
Author

isakbm commented Feb 19, 2019

@mattn

convertAssign already supports time.Time

see

case time.Time:

@mattn
Copy link
Member

mattn commented Feb 19, 2019

Yes that is a part of code that driver return time.Time. I mean case that driver return string instead of time.Time. For example, NullString is possible to convert number to string. If add NullTime, most of users expect convertion between string and time.Time. But some RDBMS does not return time.Time alawys. For example select current_timestamp on SQLite3 return string. Some of database driver return Time and Date separatedly. I guess that adding NullTime into standard package is hard to do.

@isakbm
Copy link
Author

isakbm commented Feb 19, 2019

@mattn

I see! Will have to look into this deeper. Very good point!

@agnivade
Copy link
Contributor

/cc @kardianos

@beoran
Copy link

beoran commented Feb 20, 2019

@mattn While that is true, the current situation is that NullTime is now defined separately for various databases such as postgresql, sqlite, etc, in packages all over the place. Having a NullTime in database/sql would greatly simplify this. What we need to is a mechanism to also allow parsing strings and separate fields as a time. It's then up to the driver writers contribute to the conversion what the driver returns to this type. Like this example for sqlite3: https://github.com/xo/xoutil/blob/master/xoutil.go.

@rsc
Copy link
Contributor

rsc commented Feb 27, 2019

@kardianos, any thoughts? It seems fine to me.

@gopherbot
Copy link

Change https://golang.org/cl/170699 mentions this issue: database/sql: add NullTime

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

7 participants