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

Jump to bottom

database/sql: add NullInt16 support #40082

Closed
kyleconroy opened this issue Jul 6, 2020 · 15 comments
Closed

database/sql: add NullInt16 support #40082

kyleconroy opened this issue Jul 6, 2020 · 15 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted
Projects
Proposals (old)
Milestone
Backlog

Comments

@kyleconroy
Copy link

Hey Go team!

I maintain a database code generator. I'm having trouble correctly supporting null smallint columns because the database/sql package does not have a NullInt16 type. It would look almost identical to Nullint32 and NullInt64.

// NullInt16 represents an int32 that may be null.
// NullInt16 implements the Scanner interface so
// it can be used as a scan destination, similar to NullString.
type NullInt16 struct {
	Int16 int16
	Valid bool // Valid is true if Int16 is not NULL
}

// Scan implements the Scanner interface.
func (n *NullInt16) Scan(value interface{}) error {
	if value == nil {
		n.Int16, n.Valid = 0, false
		return nil
	}
	n.Valid = true
	return convertAssign(&n.Int16, value)
}

// Value implements the driver Valuer interface.
func (n NullInt16) Value() (driver.Value, error) {
	if !n.Valid {
		return nil, nil
	}
	return int64(n.Int16), nil
}
@gopherbot gopherbot added this to the Proposal milestone Jul 6, 2020
@ianlancetaylor
Copy link
Contributor

CC @kardianos

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 7, 2020
@kardianos
Copy link
Contributor

@kyleconroy This looks reasonable. Can you send a CL?

On a side note, these nullable wrappers would probably be a great use for generics...

@kyleconroy
Copy link
Author

Yeah, can do. A bit swamped right now, but should have it done by the end of the week

@kyleconroy
Copy link
Author

On a side note, these nullable wrappers would probably be a great use for generics...

I know! Here's a four-line replacement for the various sql.Null* types in the standard library.

    type Null(type T) struct {
        Val   T
        Valid bool // Valid is true if Val is not NULL
    }

Here's what it looks like in practice: https://go2goplay.golang.org/p/Qj8MqYWWAc3

@rsc
Copy link
Contributor

rsc commented Aug 12, 2020

Do we need NullByte for tinyint too?
(No luck for 24-bit mediumint)

@kardianos
Copy link
Contributor

NullByte and NullInt16 both sound good. Might as well complete the family.

@rsc rsc moved this from Incoming to Active in Proposals (old) Aug 14, 2020
@rsc rsc mentioned this issue Aug 14, 2020
Open
@rsc
Copy link
Contributor

rsc commented Aug 26, 2020

Based on the discussion above, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Aug 26, 2020
@rsc
Copy link
Contributor

rsc commented Sep 2, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Sep 2, 2020
@rsc rsc modified the milestones: Proposal, Backlog Sep 2, 2020
@rsc rsc changed the title proposal: database/sql: add NullInt16 support database/sql: add NullInt16 support Sep 2, 2020
@muhlemmer
Copy link
Contributor

I have some spare time and like to contribute more to the Go project. May I volunteer for a CL @kyleconroy, or do you prefer to do it yourself?

@odeke-em
Copy link
Member

odeke-em commented Mar 7, 2021

@muhlemmer the Go tree is open for Go1.17, and it would be awesome to have you as a contributor. If you are unavailable, no biggie I’ll send one or someone from my team will send one. Thank you for the experience report @kyleconroy, and for the discussions @kardianos, @ianlancetaylor, and @rsc.

@a8m
Copy link
Contributor

a8m commented Apr 19, 2021

Hey @kyleconroy and @muhlemmer, do you have plans to submit CLs for these new types? I need them for ent and prefer to have them in the next release. Let me know if you don't have time and I can take it from here. Thanks

@odeke-em
Copy link
Member

@alexander-melentyev sent this CL https://go-review.googlesource.com/c/go/+/305929 (thank you Alexander!) but it adds for NullUint64 which wasn’t approved, as this proposal is for NullByte and NullInt64. While it makes sense to me to also add it, we are adhering to the proposal process.

@gopherbot
Copy link

Change https://golang.org/cl/311572 mentions this issue: database/sql: add NullInt16

@gopherbot
Copy link

Change https://golang.org/cl/311989 mentions this issue: database/sql: add NullByte

@cagedmantis
Copy link
Contributor

golang.org/cl/311572 is waiting for a trust +1. Is a package owner available to review?

@rsc rsc mentioned this issue Jun 10, 2021
Closed
83 tasks
@egtann egtann mentioned this issue Jan 14, 2022
Merged
@golang golang locked and limited conversation to collaborators May 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-Accepted
Projects
No open projects
Proposals (old)
Accepted
Milestone
Backlog
Development

No branches or pull requests
9 participants
@kyleconroy @rsc @cagedmantis @kardianos @ianlancetaylor @odeke-em @muhlemmer @a8m @gopherbot