Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Rows.Null* misleading return bool value / reducing allocs #6

Open
SchumacherFM opened this issue May 4, 2017 · 1 comment
Open

Comments

@SchumacherFM
Copy link

Null behaviour

I've used your library now more in depth for performance analyses (will publish it soon on my blog) and figured out that the pattern for Rows.Null* functions is really confusing.

// NullString returns string as a value and
// NULL indicator. When value is NULL, second parameter is true.
func (r *Rows) NullString() (string, bool) { ... }

When you come from database/sql background ;-) you expect sql.NullString.Valid to be true when the column value is not null. But in your case it returns true when the string is null. Yes it's documented but it still is the total opposite of how database/sql handles.

Would you like to change that behaviour? Would be a breaking change.

Allocs

Every NullInt* function makes a call to NullString which causes an allocation and then again you call strconv.Parse* which creates another allocation. Maybe you can directly transform the byte slice into an int like they do it here: https://github.com/go-sql-driver/mysql/blob/master/utils.go#L499

Also Rows.Time and Rows.NullTime is missing ;-) I would pull in https://github.com/go-sql-driver/mysql/blob/master/utils.go#L277 to implement it ... but I don't if that matches with your license model.

Maybe I can be wrong with the two above points ... so please correct me.

@kostyantyn
Copy link
Contributor

Hello @SchumacherFM, thanks for such detailed report.

About Null Behaviour, agree that interface is different than in database/sql. At this moment we don't plan to change it. I'd rather update the README to make it clear what the second parameter stands for.

The reason the second parameter is true when your field is null in DB is that we expect to read values in this way:

name, null := rows.NullString()
if null {
  // ...
}

However, we might rethink the interface, but in this case, we rather release mysqldriver2-go than changing existing package.

About Allocs. Thanks for the advice. Agree that transforming bytes directly into ints sounds more beneficial. Since you have already researched this part, would you be willing to make a PR?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants