-
Notifications
You must be signed in to change notification settings - Fork 372
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
columnToFieldIndex memorize func #407
base: main
Are you sure you want to change the base?
Conversation
@paulquerna-okta You based your work on |
da98f55
to
aa82be2
Compare
@nelsam rebased onto master as requested |
mapping_test.go
Outdated
BestFriends []string `db:"best_friends"` | ||
} | ||
|
||
func BenchmarkCcolumnToFieldIndex(b *testing.B) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor spelling mistake here: Ccolumn
should be Column
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in b9b8642
One other minor note: we've generally been using onpar for unit tests, for readability reasons. You can see the general structure in our dialect tests. The tests you've written here are perfectly reasonable as-is, but I thought I'd offer to let you change them before merge, if you'd like. Otherwise, I'll just merge this PR and then convert them when I find time. |
Cache Cache | ||
} | ||
|
||
type Cache interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a doc comment here, just to satisfy golint
?
Are there any plans to move this forward? We also noticed some bottlenecks related to |
Could be considered a way to supersede #406
Adds a Cache interface + uses it in
columnToFieldIndex
. This can be simply backed by&sync.Map{}
, or a more complex cache library like ristretto. No new deps, but still gets the same category of performance increases described in #406