Skip to content

Use subquery to instead In #10874

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 30, 2020
Merged

Use subquery to instead In #10874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
415c179
Use subquery to instead In
lunny Mar 29, 2020
355b0c7
Support excludedLabelNames on issues options
lunny Mar 29, 2020
9ba4136
Fix tests
lunny Mar 29, 2020
056842e
Merge branch 'master' into lunny/use_sub_query_instead_in
zeripath Mar 29, 2020
769aa8e
Merge branch 'master' into lunny/use_sub_query_instead_in
lunny Mar 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions models/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1045,16 +1045,18 @@ func GetIssuesByIDs(issueIDs []int64) ([]*Issue, error) {
// IssuesOptions represents options of an issue.
type IssuesOptions struct {
ListOptions
RepoIDs []int64 // include all repos if empty
AssigneeID int64
PosterID int64
MentionedID int64
MilestoneID int64
IsClosed util.OptionalBool
IsPull util.OptionalBool
LabelIDs []int64
SortType string
IssueIDs []int64
RepoIDs []int64 // include all repos if empty
AssigneeID int64
PosterID int64
MentionedID int64
MilestoneID int64
IsClosed util.OptionalBool
IsPull util.OptionalBool
LabelIDs []int64
IncludedLabelNames []string
ExcludedLabelNames []string
SortType string
IssueIDs []int64
// prioritize issues from this repo
PriorityRepoID int64
}
Expand Down Expand Up @@ -1153,6 +1155,14 @@ func (opts *IssuesOptions) setupSession(sess *xorm.Session) {
}
}
}

if len(opts.IncludedLabelNames) > 0 {
sess.In("issue.id", BuildLabelNamesIssueIDsCondition(opts.IncludedLabelNames))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guillep2k can we do better here? I'm suspicious that this In could be quite expensive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WHERE EXISTS (SELECT ...) is the equivalent idiom, but depending on the engine it could be better optimized.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It all depends on how sophisticated the engine is. Some engines will rearrange the whole query and be very clever on optimizing the access... others will just take your statement more or less literally, and therefore your syntax might influence the results drastically. And versions are important too. I read from time to time "XX SQL now takes this construct and treats it like xxx for better performance".

Formally, WHERE EXISTS (SELECT ...) is equivalent to IN (SELECT ....); but if the engine is not too smart, EXISTS is a kind of join that is more optimizable than a plain LOOP JOIN (e.g. using a HASH JOIN).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But performing the IN() in a subquery is definitely more performant that SELECT ids, and then `SELECT ... WHERE ... IN(id list).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like @guillep2k said, it depends on the database engine. The sub query maybe cached or not. It may be faster than before or not. But it will give the right result however many there are labels. But for In with id, when there are over 500 variables, database will return error.

}

if len(opts.ExcludedLabelNames) > 0 {
sess.And(builder.NotIn("issue.id", BuildLabelNamesIssueIDsCondition(opts.ExcludedLabelNames)))
}
}

// CountIssuesByRepo map from repoID to number of issues matching the options
Expand Down
11 changes: 11 additions & 0 deletions models/issue_label.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,17 @@ func GetLabelIDsInRepoByNames(repoID int64, labelNames []string) ([]int64, error
Find(&labelIDs)
}

// BuildLabelNamesIssueIDsCondition returns a builder where get issue ids match label names
func BuildLabelNamesIssueIDsCondition(labelNames []string) *builder.Builder {
return builder.Select("issue_label.issue_id").
From("issue_label").
InnerJoin("label", "label.id = issue_label.label_id").
Where(
builder.In("label.name", labelNames),
).
GroupBy("issue_label.issue_id")
}

// GetLabelIDsInReposByNames returns a list of labelIDs by names in one of the given
// repositories.
// it silently ignores label names that do not belong to the repository.
Expand Down
30 changes: 14 additions & 16 deletions routers/api/v1/repo/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ func SearchIssues(ctx *context.APIContext) {
opts.Private = true
opts.AllLimited = true
}

issueCount := 0
for page := 1; ; page++ {
opts.Page = page
Expand Down Expand Up @@ -127,15 +128,6 @@ func SearchIssues(ctx *context.APIContext) {
issueIDs, err = issue_indexer.SearchIssuesByKeyword(repoIDs, keyword)
}

labels := ctx.Query("labels")
if splitted := strings.Split(labels, ","); labels != "" && len(splitted) > 0 {
labelIDs, err = models.GetLabelIDsInReposByNames(repoIDs, splitted)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetLabelIDsInRepoByNames", err)
return
}
}

var isPull util.OptionalBool
switch ctx.Query("type") {
case "pulls":
Expand All @@ -146,6 +138,12 @@ func SearchIssues(ctx *context.APIContext) {
isPull = util.OptionalBoolNone
}

labels := strings.TrimSpace(ctx.Query("labels"))
var includedLabelNames []string
if len(labels) > 0 {
includedLabelNames = strings.Split(labels, ",")
}

// Only fetch the issues if we either don't have a keyword or the search returned issues
// This would otherwise return all issues if no issues were found by the search.
if len(keyword) == 0 || len(issueIDs) > 0 || len(labelIDs) > 0 {
Expand All @@ -154,13 +152,13 @@ func SearchIssues(ctx *context.APIContext) {
Page: ctx.QueryInt("page"),
PageSize: setting.UI.IssuePagingNum,
},
RepoIDs: repoIDs,
IsClosed: isClosed,
IssueIDs: issueIDs,
LabelIDs: labelIDs,
SortType: "priorityrepo",
PriorityRepoID: ctx.QueryInt64("priority_repo_id"),
IsPull: isPull,
RepoIDs: repoIDs,
IsClosed: isClosed,
IssueIDs: issueIDs,
IncludedLabelNames: includedLabelNames,
SortType: "priorityrepo",
PriorityRepoID: ctx.QueryInt64("priority_repo_id"),
IsPull: isPull,
})
}

Expand Down