search

pubnative / mysqldriver-go

GC optimized MySQL driver
https://godoc.org/github.com/pubnative/mysqldriver-go
MIT License
268 stars 20 forks source link

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

Open SchumacherFM opened 7 years ago

SchumacherFM commented 7 years ago

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 commented 7 years ago

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?