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: sql.IsNull #34593

Closed
qaisjp opened this issue Sep 29, 2019 · 9 comments

Comments

@qaisjp
Copy link
Contributor

@qaisjp qaisjp commented Sep 29, 2019

An addition to database/sql to quickly tell you whether one of the NullX types are in fact, "null".

It would be the addition of essentially this:

// IsNull will return true if the value given is a "null type"
//  (NullBool, NullString, etc) that is marked "invalid".
//
// Always returns false otherwise.
func IsNull(val interface{}) (invalid bool) {
	switch v := val.(type) {
	case sql.NullBool:
		invalid = !v.Valid
	case sql.NullFloat64:
		invalid = !v.Valid
	case sql.NullInt32:
		invalid = !v.Valid
	case sql.NullInt64:
		invalid = !v.Valid
	case sql.NullString:
		invalid = !v.Valid
	case sql.NullTime:
		invalid = !v.Valid
	}
	return
}

Am happy to send PR, just wanted to quickly get feedback on whether this would be fine? Or should I have used the golang-dev mailing list instead?

@gopherbot gopherbot added this to the Proposal milestone Sep 29, 2019
@gopherbot gopherbot added the Proposal label Sep 29, 2019
@smasher164

This comment has been minimized.

Copy link
Member

@smasher164 smasher164 commented Sep 29, 2019

How often are NullX values declared as interface{} in the wild, where one couldn't just check v.Valid?
/cc @bradfitz @kardianos @kevinburke

@renthraysk

This comment has been minimized.

Copy link

@renthraysk renthraysk commented Sep 29, 2019

SQL drivers also define their own Null* types. So IsNull() is never guaranteed to work as expected with a type switch.

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Sep 29, 2019

This proposal would not be robust for user types. Good to keep in mind for v2 however.

@qaisjp

This comment has been minimized.

Copy link
Contributor Author

@qaisjp qaisjp commented Sep 29, 2019

Yeah, you're totally right about it not being robust for user types.

Is it perhaps worth making them implement an interface that expects IsValid() bool?

I suppose one could also check if Value() (in interface driver.Valuer) returns nil? Although I think that's a typed nil, so the only way to compare against that nil is by using reflection?

@qaisjp

This comment has been minimized.

Copy link
Contributor Author

@qaisjp qaisjp commented Sep 29, 2019

How often are NullX values declared as interface{} in the wild, where one couldn't just check v.Valid?

I'm using it to generate SQL queries based on the type of a value. Unfortunately = null cannot be used in SQL, is null must be be used, hence the need to do this

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 14, 2019

@qaisjp, you say that you are holding an interface{} that may contain one of these types, and if it is a value of one of these types that represents null, you want to format the SQL query as is null.

I'm with you so far.

But then what if it's not null?
How do you format the = value query?
Doesn't that still require a type switch to extract the value?
Assuming that it does, then the null case can go into the same type switch, at which point the IsNull is not saving much.

This doesn't seem like it needs to be in the standard library when it can be so easily provided outside the library and would not be used that often.

@qaisjp

This comment has been minimized.

Copy link
Contributor Author

@qaisjp qaisjp commented Nov 14, 2019

When it's not null I did something like this: stmt += "= " + "$" + strconv.Itoa(i+1), so it looks like = $2.

Then we pass in the vals []interface{} when executing: preparedStmt.ExecContext(ctx, vals...)

If it's worth mentioning, I'm no longer composing the statement this way, so no longer need an sql.IsNull...

@rsc rsc added this to Incoming in Proposals Nov 27, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

Given the discussion so far and especially @renthraysk's #34593 (comment) comment and the fact that @qaisjp no longer needs this function anyway, this seems like a likely decline.

Leaving open for a week for final comments.

@rsc rsc moved this from Incoming to Active in Proposals Dec 4, 2019
@rsc rsc moved this from Active to Likely Decline in Proposals Dec 4, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2019

No change in consensus, so declining.

@rsc rsc closed this Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Likely Decline
7 participants
You can’t perform that action at this time.