This repository has been archived by the owner on Oct 17, 2020. It is now read-only.
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.
Added GetByAliases function in db and repository #539
Added GetByAliases function in db and repository #539
Changes from 15 commits
0cc18b3
5071836
c42e35e
38de94e
3d3fd2f
3bb21a3
0bc2956
32f1603
44741eb
fcb8bd0
deca9ca
2405877
dc9d937
1016e42
264c13f
e50e35a
fac0bf6
d36a571
09b97a0
53f7d73
480c5d1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Couldn't find the
for
loop andQueryRow
in the PR. Please checkout this example mentioned in one of the previous articles: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.
according to official document for function
QueryRow
:// If the query selects no rows, the *Row's Scan will return ErrNoRows.
// Otherwise, the *Row's Scan scans the first selected row and discards
// the rest.
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.
it seems that the document is only selecting a single row for testing purpose.
Here we want to select a series of rows, I believe we have to use
Query
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.
Please read this whole thread: golang/go#5171
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.
@byliuyang In that thread, they talk about bulk operations
a way to insert multiple rows at once
.Our case is a standard SQL query, which requires to call
Query
and pass the list of args.Please read this http://go-database-sql.org/prepared.html
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.
@alldroll I want to make everything explicit to avoid magics.
@dawntomm I don't want to block your changes. Let's add a TODO here to further discuss our SQL handling strategy. I'll approve this PR after the TODO is added.
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.
@byliuyang Just added a TODO
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.
@byliuyang sorry I still did not get why you think we should use QueryRow instead of Query, if we use QueryRow then it will cost us more connections with DB, we could definitely do performance test, but intuitively I feel it's slower
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.
@dawntomm All queryRows in a single prepare statement share one connection.