Update to use our new API #7
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refactor until the public API matches what we sketched out (see docs/gophercon2016hackdaynotes.md) and successfully builds. Drop the genreadme.sh script, which we're not using anymore.
Finish refactoring to achieve our desired API, and update our tests to pass. So far, our only problems are the Unmarshal tests, because I'd like to think of a way to get around including sqlite, or at least make them an integration test.
Implement the String method for query, to return the SQL (with values injected) that would be executed on the server. This is not quite perfect yet--we're using the fmt.Sprintf `%v` output for each arg right now, instead of the actual SQL syntax, but this should be good enough to give people an idea when debugging. We could improve on this by, e.g., quoting strings. I'd like, one day, to have that output be copy/paste-able valid SQL, but, as we say to death, not today. I also added a benchmark for the String function, and renamed our current benchmarks, to make room for future benchmarking.
Benchmark how long it takes to build a query object when querying, and when inserting.
By removing our hand-crafted byte management and memory allocation, and relying on the strings.Index method and building the string section by section (like we do for the String method) we get a roughly 100% performance improvement. Thanks, benchmarks!
Only read the column names from a struct using reflection once, then cache them; then we only re-read them when we need to retrieve values. I also dropped the buffer clearing from toSnake, because we only use the bytes we overwrote anyways, so there's really no point in zeroing them out. The cache brings between 10-50% improvement on cache times for inserts and queries, respectively. Obviously, it will change depending on your queries, but the more you read column names, the bigger the improvement will be, and the more you read values, the smaller the improvement will be.
Make all our tests run in parallel. Add a RWMutex to our struct reflection cache. This removes annoying race conditions we had in our caching, without a noticeable performance impact. I also added a column cache, so the Column function could only use reflection the first time it saw a type/property combo, but there was no measurable performance benefit, so I dropped it. I changed readStruct to only populate the values return variable if the needsValues argument is set to true. This has a probably immeasurable performance impact, but more importantly makes behaviour consistent, regardless of whether values are retrieved from the cache or from reflect directly. I dropped the getColumn helper, which Column called through to, as Column was the only thing calling it. So now it's just Column. I also fixed the bug that had us returning the bare column name, instead of the absolute column name, as the rest of the library does. This required an update to our sqlTable tests.
Use @agocs' README as a basis, and update it for the new syntax, and clarify some important points.
Remove the section on parseTime, as that's not a pan restriction (it depends on the driver being used), fix a typo, update wording on struct property mapping.
Take advantage of having the Column names available to us to do a better job of matching columns to properties. Drop docstrings, which are all out of date and need rewritten. Rename VariableList to Placeholders, which is really what it's generating. Update our Unmarshal test to use Postgres, not sqlite, which doesn't support table.column_name syntax, and was giving me trouble. Eventually, we should find a fix for this?
Add a Flag type that will let callers specify whether they want the
absolute column name (table.column), double-quoted ("column"), ticked
(`column`), or a combination thereof. This is exposed only on the
Columns and Column methods; convenience methods built on top only ever
use the bare column name. People can deal with it.
Stop exporting the TAG_NAME constant, there's no reason to.
`Placeholders` now generates placholders with a space after the comma.
We're back to using SQLite for the tests, now that it works again.
Add more versions of Go to .travis.yml so we can test against more versions of Go.
The go-sqlite3 library we're using in our tests has no support for Go 1.2, apparently. The tests keep breaking on it. So let's not test against that, and support 1.3 and above.
Allow people to add additional pointers to Unmarshal, so they can use the struct columns as the basis for a query, but still add things on to them. This obviates the need for the expression tag.
Add a note to our README linking to the mailing list, and add a CONTRIBUTORS file.
Add docstrings so golint is happy. Explicitly ignore the output of our String function when benchmarking, to make go vet happy. Add an ErrNeedsFlush error, which is returned when a Query is used but has dangling expressions that weren’t properly flushed. Add a test that ErrNeedsFlush is returned, as expected.
If there’s nothing for Flush to do, then just bail out. This makes sure it’s safe to call Flush multiple times.
|
Pinging @agocs to sanity check, if you have time. :) If we can merge this, I'll tag it as the 0.1.0 release. If we use it for a while and are happy with the decisions we made (or can get some feedback from other people), then we can go about tagging a 1.0.0 release that signals we stand behind the API. I know we at least have some concern over Flush that still need examination, testing, discussion, and resolution. |
Closed
|
Hey @agocs, no rush or anything, but I'll probably just merge this in and tag a 0.1.0 release at the end of today if I don't hear from you. I'm always happy to get post-merge comments and refactor from there, but I want to move this forward a bit. |
|
Ah, shoot! Sorry @paddyforan, I totally forgot about this. It looks like what we discussed/hacked on, and is a-ok by me for an 0.1.0 tag at your leisure. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This (horrible, one-giant-lump) PR basically just refactors the way our API is setup, trying to smooth out the inconsistencies, and address the problem we're solving from ground zero, this time with a bit more experience using this in production.
We purposefully tried to build everything as a convenience helper on top of the
Expressionmethod, which replaces our old (more ambiguously named)Includemethod. The idea was to make sure any of our helpers could be replicated--and therefore tweaked--in minimal amounts of code by people in application space.We also introduced a cache for reflect, making some speed improvements for column name reading.
We also pulled out all the hand-crafted rune handling and memory allocation for our postgres conversion, using the strings package instead. This yielded a ~100% improvement in speed.