Search for issues/pulls #530

ethantkoenig · 2016-12-29T14:04:08Z

Issue #379. This PR is the same as the now-closed #387; I had to reissue the PR for logistical reasons.

Uses a bleve indexer to search issues/pulls by keyword.

ethantkoenig · 2017-01-10T16:17:34Z

I realize this PR is a lot to review, but I don't see any good ways to split it into smaller parts. If anyone has suggestions on how to make it more digestible, let me know.

lunny · 2017-01-11T01:25:50Z

models/issue.go

+		}
+		sess = x.Limit(setting.UI.IssuePagingNum, start)
+	} else {
+		sess = x.NewSession()


defer sess.Close()

lunny · 2017-01-11T01:29:36Z

models/issue.go

+		search.SortBy([]string{"-CreatedUnix"})
+	}
+
+	index, err := bleve.Open(setting.IndexPath)


One index file could be reopen twice? Maybe the index service should a separated goroutine to receive add/remove/search index command and return results.

I've refactored it so that the indexer is accessed only once.

lunny · 2017-01-11T01:31:12Z

models/issue.go

+	}
+
+	search.Fields = []string{
+		"ID",


It only need to store index or order columns, don't store all the data And then we can load the other data from database?

I'm a little confused: in #379 (comment) you said "If we use bleve, SQL query will be no need". I thought that meant that all of the issue columns should be in the indexer. @tboerger's comment (#387 (comment)) also says to add all of the columns to the indexer.

I personally prefer the approach of only storing the title and content in the indexer, and filtering by and loading other columns from the database, and would be happy to change my PR to use this approach. I just want to make sure that I'm understanding you before moving forward.

If I made you confuse I'm sorry. I think only store keyword and ID, order and sorter, and then load data from database via In or Or.

Bwko · 2017-01-12T16:46:44Z

I've tested it with several large issues(between 1-5mb of text) and the search feature works very fast.
Is it still a WIP?

ethantkoenig · 2017-01-12T16:58:15Z

@Bwko That's good to hear! There are a few minor things that I need to clean up, and then it should be ready

lunny · 2017-01-13T06:44:23Z

models/issue_comment.go

 			return err
 		}
 	}

-	return sess.Commit()
+	return sess.Commit();


lunny · 2017-01-13T06:46:40Z

models/issue_indexer.go

+	if err != nil {
+		return nil, err
+	}
+	result, err := index.Search(search)


defer index.Close()?

Similar to what I've said in other comment, I'd like to check if index.Close() returns an error. I've refactored the code to make it a little clearer that index.Close() always gets called.

lunny · 2017-01-13T06:48:25Z

models/issue_indexer.go

+			return err
+		}
+	}
+	indexerUpdateQueue = make(chan *Issue, 10)


Make QueueSize as config in [indexer] and default is 10.

lunny · 2017-01-13T06:54:20Z

models/issue_indexer.go

+	for {
+		select {
+		case issue := <-indexerUpdateQueue:
+			index, err := bleve.Open(setting.IssueIndexerPath)


Why not always open on the begging of the function? And defer Close it.

Only one thread can open an indexer at a time. If the update thread always held the indexer open, no other threads could ever access the indexer. I'll add a comment

lunny · 2017-01-13T07:01:40Z

models/issue_indexer.go

+
+// PopulateIssuesIndexer write all issues in the database to the issues indexer
+func PopulateIssuesIndexer() error {
+	index, err := bleve.Open(setting.IssueIndexerPath)


index.Close() will always be called; every return statement is preceded by a call to index.Close(). I decided not to use defer index.Close() since index.Close() returns an error, and I wanted to be able to check if closing the index was successful.

If you want, I can change it to use defer index.Close()

Actually, I think the best solution is to break PopulateIssuesIndexer into two functions: an "outer" function PopulateIssuesIndexer which manages the opening and closing of the indexer, and an "inner" function populateIssuesIndexer which is passed an indexer, and adds issues to it.

This will allow us to check if an error is returned by index.Close(), while also easily ensuring that every code path will call index.Close().

lunny · 2017-01-13T07:03:19Z

models/issue_indexer.go

+				return err
+			}
+			go func() {
+				if err := PopulateIssuesIndexer(); err != nil {


I prefer remove the go keyword and then this should be called when Gitea start. And if there are err, it should stop and print the error msg on console.

ethantkoenig · 2017-01-13T16:43:55Z

@lunny I believe I have addressed all of your requests.

lunny · 2017-01-14T02:04:12Z

models/issue_indexer.go

+	search := bleve.NewSearchRequestOptions(indexerQuery, 2147483647, 0, false)
+	search.Fields = []string{"ID"}
+
+	index, err := bleve.Open(setting.Indexer.IssuePath)


It seems we have to keep only one index opened all round the program? It should be created or opened at the start up and closed before Gitea will be closed.

@lunny Fixed 😄

lunny · 2017-01-14T02:06:23Z

And bleve's default analysis don't support CJK languages, we could support them in step 2.

bkcsoft · 2017-01-20T08:21:08Z

don't support CJK languages

@lunny is there an upstream issue for that? otherwise I suggest making one 😉

lunny · 2017-01-20T11:21:39Z

Not a upstream problem. We need a CJK custom plugin for bleve to do that.

bkcsoft · 2017-01-21T02:47:14Z

@lunny an indexer in 2017 that doesn't handle CJK isn't a very good indexer and should be fixed 😆

lunny · 2017-01-21T14:28:10Z

@bkcsoft, but I think that will be a big PR. So it could be splitted as two PRs to do that.

lunny · 2017-01-22T05:45:27Z

LGTM

thibaultmeyer · 2017-01-24T15:50:59Z

LGTM

ethantkoenig · 2017-01-24T15:55:26Z

This should be rebased down to a single commit before getting merged in, right?

thibaultmeyer · 2017-01-24T15:59:40Z

Yeah it would be nice if you can squash to a single commit (with git reset HEAD~n by exemple)

lunny · 2017-01-25T02:42:55Z

let L-G-T-M work.

sha-red

Improvements to make the search feature actually useful.

sha-red · 2017-02-23T16:28:49Z

models/issue_indexer.go

+// SearchIssuesByKeyword searches for issues by given conditions.
+// Returns the matching issue IDs
+func SearchIssuesByKeyword(repoID int64, keyword string) ([]int64, error) {
+	fields := strings.Fields(strings.ToLower(keyword))


"fields" should actually be "terms", API: NewPhraseQuery(terms []string, field string)

The variable name has since been updated to terms (#1031)

sha-red · 2017-02-23T16:30:24Z

models/issue_indexer.go

+	indexerQuery := bleve.NewConjunctionQuery(
+		numericQuery(repoID, "RepoID"),
+		bleve.NewDisjunctionQuery(
+			bleve.NewPhraseQuery(fields, "Title"),


At least we should have a Match Phrase Query, if not a Match Query or best a Query String Query.

For both fields "Title" and "Content".

sha-red · 2017-02-23T16:30:48Z

models/issue_indexer.go

+		bleve.NewDisjunctionQuery(
+			bleve.NewPhraseQuery(fields, "Title"),
+			bleve.NewPhraseQuery(fields, "Content"),
+		))


Comments should be indexed, too, and queried here.

sha-red · 2017-02-23T16:32:33Z

models/issue_indexer.go

+	textFieldMapping.Analyzer = simple.Name
+	docMapping.AddFieldMappingsAt("Title", textFieldMapping)
+	docMapping.AddFieldMappingsAt("Content", textFieldMapping)
+


Issue comments should also be indexed here.

sha-red · 2017-02-23T16:37:28Z

models/issue_indexer.go

+	textFieldMapping.Analyzer = simple.Name
+	docMapping.AddFieldMappingsAt("Title", textFieldMapping)
+	docMapping.AddFieldMappingsAt("Content", textFieldMapping)
+


More token filters should be added (http://www.blevesearch.com/docs/Token-Filters/), to allow partial matches. My suggestion:

Short n-grams to allow partial matches (an issue tracker won't work without partial matching)

Strong Unicode normalizing (because of diacritical letters)

Multi-lingual stemming

@sha-red exactly. Would you have time to send some PRs to help improve it?

@lunny Basically I'd love to help with actual code, with pleasure, but because I'd have to set up a whole Go development environment and lot's of other work to do, this will take some time to get everything running.

Please also have a look at our minimalistic issue search path for gogs: gogs/gogs#4015

IMHO whole phrase search is better than nothing, but at least comment indexing should be added before releasing.

We can improve the search in v1.2, comment index should be added in that release cycle and of course the UI should be changed for keyword highlight.

lunny added this to the 1.1.0 milestone Dec 29, 2016

andreynering added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Dec 29, 2016

ethantkoenig force-pushed the search_bar branch 4 times, most recently from ce901cd to a7ea326 Compare January 5, 2017 18:46

ethantkoenig force-pushed the search_bar branch 3 times, most recently from f22128e to 39f0a4a Compare January 9, 2017 21:30

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 10, 2017

ethantkoenig force-pushed the search_bar branch from 39f0a4a to 98c3cb7 Compare January 10, 2017 20:19

lunny requested changes Jan 11, 2017

View reviewed changes

ethantkoenig force-pushed the search_bar branch from 98c3cb7 to aab0fcc Compare January 11, 2017 20:21

ethantkoenig changed the title ~~[WIP] Search for issues/pulls~~ Search for issues/pulls Jan 12, 2017

lunny requested changes Jan 13, 2017

View reviewed changes

ethantkoenig force-pushed the search_bar branch from 0f3991d to 57284db Compare January 13, 2017 22:15

lunny requested changes Jan 14, 2017

View reviewed changes

lunny mentioned this pull request Jan 17, 2017

Issues: Assigned to me #503

Closed

ethantkoenig force-pushed the search_bar branch from cff1cd1 to 4764d00 Compare January 22, 2017 03:03

lunny approved these changes Jan 22, 2017

View reviewed changes

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 22, 2017

lunny removed the pr/wip This PR is not ready for review label Jan 22, 2017

lunny mentioned this pull request Jan 24, 2017

search by keyword in issue #743

Closed

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 24, 2017

ethantkoenig force-pushed the search_bar branch from 4764d00 to 0893aa5 Compare January 24, 2017 16:20

Search bar for issues/pulls

e0e49aa

ethantkoenig force-pushed the search_bar branch from 0893aa5 to e0e49aa Compare January 24, 2017 16:22

lunny merged commit 833f8b9 into go-gitea:master Jan 25, 2017

ethantkoenig deleted the search_bar branch January 25, 2017 03:47

lunny added a commit to lunny/gitea that referenced this pull request Jan 25, 2017

bug fixed caused by go-gitea#530

6214c79

lunny mentioned this pull request Jan 25, 2017

Can't show "Issues" page (Error 500) #754

Closed

6 tasks

lunny added a commit that referenced this pull request Jan 25, 2017

bug fixed caused by #530 (#755)

2a80e5a

lunny mentioned this pull request Jan 25, 2017

CJK languages for issues search #759

Closed

Bwko mentioned this pull request Jan 26, 2017

Added webkit-font-smoothing #763

Merged

sha-red reviewed Feb 23, 2017

View reviewed changes

ethantkoenig mentioned this pull request Aug 25, 2017

Improve issue search #2387

Merged

xak2000 mentioned this pull request Feb 2, 2018

Support stemming / partial matches / unicode normalizing in issue search #3448

Closed

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Search for issues/pulls #530

Search for issues/pulls #530

Conversation

ethantkoenig commented Dec 29, 2016 • edited Loading

ethantkoenig commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bwko commented Jan 12, 2017

ethantkoenig commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Jan 14, 2017

bkcsoft commented Jan 20, 2017

lunny commented Jan 20, 2017

bkcsoft commented Jan 21, 2017

lunny commented Jan 21, 2017

lunny commented Jan 22, 2017

thibaultmeyer commented Jan 24, 2017 • edited Loading

ethantkoenig commented Jan 24, 2017

thibaultmeyer commented Jan 24, 2017

lunny commented Jan 25, 2017

sha-red left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig commented Dec 29, 2016 •

edited

Loading

thibaultmeyer commented Jan 24, 2017 •

edited

Loading

ethantkoenig Feb 24, 2017 •

edited

Loading