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

Avoid allocating on each call to rows.Columns #444

Merged
merged 3 commits into from
May 12, 2017
Merged

Avoid allocating on each call to rows.Columns #444

merged 3 commits into from
May 12, 2017

Conversation

nussjustin
Copy link
Contributor

@nussjustin nussjustin commented Apr 3, 2016

Description

Calling rows.Columns() always allocates a new slice, even if it was already called once. Cache the slice on the first call and return the cached value on subsequent calls.

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@methane
Copy link
Member

methane commented Aug 25, 2016

LGTM

Also if we have a prepared statement, cache the column names in the statement the first time it gets executed and set them the next time the prepared statement gets executed (as is already done with the column information).

I'm afraid that adding / removing column from another session may break the cache.
But column info caching is already there. This PR doesn't make it worse.

@julienschmidt julienschmidt added this to the v1.3 milestone Oct 25, 2016
@julienschmidt julienschmidt self-assigned this Oct 25, 2016
@julienschmidt julienschmidt modified the milestones: v1.3, v1.4 Oct 25, 2016
@nussjustin nussjustin closed this Dec 1, 2016
@nussjustin
Copy link
Contributor Author

Okay, wow. Today I learned that a wrong push closes a PR Oo

@julienschmidt
Copy link
Member

You can reopen it after making another push to the branch (or just rebase and force-push)

@julienschmidt
Copy link
Member

Or at least team members can reopen it then

@nussjustin nussjustin reopened this Dec 1, 2016
@nussjustin
Copy link
Contributor Author

That worked :-) I also updated the benchmark in the PR for the Go 1.8 beta.

@methane
Copy link
Member

methane commented May 12, 2017

Please merge master.

Also if we have a prepared statement, cache the column names in the statement the first time it gets executed and set them the next time the prepared statement gets executed (as is already done with the column information).

The information cache is removed because prepared statement can return different columns.

@coveralls
Copy link

coveralls commented May 12, 2017

Coverage Status

Coverage increased (+0.04%) to 74.573% when pulling 5cd094e on nussjustin:master into aeb7d3c on go-sql-driver:master.

@julienschmidt julienschmidt merged commit 382e13d into go-sql-driver:master May 12, 2017
@julienschmidt
Copy link
Member

Thanks for your first contribution to the project! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants