Search: enable `repo:has.topic()` for GitLab #57649

jasonhawkharris · 2023-10-16T21:51:03Z

add support to has.topic for gitlab topics. Previously only worked for github topics.

Test plan

Confirm new query does not break github topics
Confirm new index works for gitlab topics
Run EXPLAIN query locally to confirm idx_repo_gitlab_topics is being used

sourcegraph=# EXPLAIN SELECT *
FROM repo
WHERE external_service_type = 'gitlab'
AND metadata->'topics' @> '["test-topic"]';
                                                        QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------
 Bitmap Heap Scan on repo  (cost=12.69..300.47 rows=89 width=687)
   Recheck Cond: (((metadata -> 'topics'::text) @> '["test-topic"]'::jsonb) AND (external_service_type = 'gitlab'::text))
   ->  Bitmap Index Scan on idx_repo_gitlab_topics  (cost=0.00..12.67 rows=89 width=0)
         Index Cond: ((metadata -> 'topics'::text) @> '["test-topic"]'::jsonb)
(4 rows)

Preview 🤩

Preview Link

sourcegraph-bot · 2023-10-16T21:53:06Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff a9f2ba5...feae35b.

Notify	File(s)
@eseliger	internal/database/repos.go internal/extsvc/gitlab/projects.go internal/repos/gitlab_test.go
@fkling	client/shared/src/search/query/predicates.ts client/shared/src/search/query/predicates.ts

sourcegraph-bot · 2023-10-16T21:53:06Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff a3dbd371eb14ca5a56e250d068e6dabad296b638...959d1f27a799b17c3c3fbe9744590b3e796fe0ef.

Notify	File(s)
@eseliger	internal/database/repos.go internal/extsvc/gitlab/projects.go

CHANGELOG.md

internal/database/repos.go

doc/code_search/reference/queries.md

migrations/frontend/1697489421_add_gitlab_topics/up.sql

BolajiOlajide

This looks good to me.

Can you update the test plan for this PR in case there's anything specific the reviewers need to test?

Also, it seems you need to run the sg gen command to update the database schema docs - I believe that's what's causing the current CI failure.

sourcegraph-bot · 2023-10-17T15:08:00Z

📖 Storybook live preview

camdencheek

Looks great!

It would still be nice to add a SQL comment on the new index like was done with the previous index

COMMENT ON INDEX idx_repo_github_topics IS 'An index to speed up listing repos by topic. Intended to be used when TopicFilters are added to the RepoListOptions';

debug.test1352377631

eseliger

Thanks for also caring about DB perf and generating a query plan for the test plan!

However, the plan generated might differ quite a lot from what is actually run in production for two reasons:

We usually apply way more filters than just this one tested for, about visibility, deleted status, and so forth and it might be that with all these other conditions combined postgres no longer decides to actually use the index you would think it is using
Your local dataset seemed to be a few rows only, to get real-world results it should be run against a database that has a ton of data, like sourcegraph.coms database. Postgres behaves very different when there's 10 rows vs when there's 10000 rows

Also left an inline comment about adding new indexes for every code host.

Don't see this as blocking, rather as some food for thought before this goes into production :)

internal/database/repos.go

migrations/frontend/1697489421_add_gitlab_topics/down.sql

migrations/frontend/1697489421_add_gitlab_topics/up.sql

migrations/frontend/1697558292_comment_on_gitlab_topic_idx/down.sql

migrations/frontend/1697558292_comment_on_gitlab_topic_idx/up.sql

eseliger · 2023-10-19T14:38:06Z

internal/database/schema.md

    "idx_repo_github_topics" gin (((metadata -> 'RepositoryTopics'::text) -> 'Nodes'::text)) WHERE external_service_type = 'github'::text
+    "idx_repo_gitlab_topics" gin ((metadata -> 'topics'::text)) WHERE external_service_type = 'gitlab'::text


Hmm the repo table has an absolute overload of indexes already, I wonder if we could simplify this by normalizing the DB a bit:
We introduce a new column topics text[] and the syncer decides what to write to that field. Then GitHub and GitLab can both do that, and in the future some other code host could also simply add to that proper column, vs us adding a new index every time.

That would also simplify the query we're making, because we can simply operate on a single column vs adding multiple OR-d together statements about the different ways topics can be retrieved.

Agreed that we should simplify it, but I expect the index to be quite small. The github topics index is 122MB on dotcom (select pg_table_size('idx_repo_github_topics');), and we have many more github repos than gitlab repos.

I'd be a bit concerned about denormalizing the DB. The syncer logic is a bit complex, and I honestly wouldn't feel comfortable changing it for a small feature like this, but what about a generated column? something like topics GENERATED STORED AS CASE WHEN external_service_type = 'gitlab' THEN ... WHEN external_service_type = 'github' THEN ... ELSE [] END

That would work, too I assume! I haven't dealt with generated columns much in the past, but trust that they work well.

Co-authored-by: Erik Seliger <erikseliger@me.com>

…n.sql Co-authored-by: Erik Seliger <erikseliger@me.com>

Co-authored-by: Erik Seliger <erikseliger@me.com>

camdencheek · 2023-10-19T21:12:20Z

@jasonhawkharris did you intend to merge this?

jasonhawkharris requested a review from a team October 16, 2023 21:51

cla-bot bot added the cla-signed label Oct 16, 2023

jasonhawkharris requested a review from camdencheek October 16, 2023 21:56

camdencheek reviewed Oct 16, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

internal/database/repos.go Outdated Show resolved Hide resolved

doc/code_search/reference/queries.md Outdated Show resolved Hide resolved

migrations/frontend/1697489421_add_gitlab_topics/up.sql Outdated Show resolved Hide resolved

camdencheek changed the title ~~Jhh/search gitlab topics~~ Search: enable repo:has.topic() for GitLab Oct 16, 2023

BolajiOlajide approved these changes Oct 17, 2023

View reviewed changes

jasonhawkharris force-pushed the jhh/search-gitlab-topics branch 2 times, most recently from d73f2b2 to 9a84dd3 Compare October 17, 2023 15:00

jasonhawkharris enabled auto-merge (squash) October 17, 2023 15:45

camdencheek approved these changes Oct 17, 2023

View reviewed changes

This was linked to issues Oct 17, 2023

Extend repo:has.topic() Support to GitLab #57326

Closed

Support repo:has.topic for GitLab #56279

Closed

jasonhawkharris disabled auto-merge October 17, 2023 15:56

jasonhawkharris enabled auto-merge (squash) October 17, 2023 16:09

jasonhawkharris force-pushed the jhh/search-gitlab-topics branch from 3fa939b to e500952 Compare October 18, 2023 20:42

jasonhawkharris added 8 commits October 19, 2023 09:08

add has.topic support for gitlab

951383e

doc and changelog update

1fc1279

add PR link to changelog entry

0c862e6

comment and doc edits address new has.topic support

8a14452

fix hover predicate to include gitlab

b08991d

schema updates

f594802

add comment on gitlab topic idx

163aafe

unit test improvements

54ed6aa

jasonhawkharris force-pushed the jhh/search-gitlab-topics branch from e500952 to b01a5cf Compare October 19, 2023 14:08

eseliger reviewed Oct 19, 2023

View reviewed changes

debug.test1352377631 Outdated Show resolved Hide resolved

fix listSql so that it passes all unit tests

279e043

jasonhawkharris force-pushed the jhh/search-gitlab-topics branch from b01a5cf to 279e043 Compare October 19, 2023 14:32

camdencheek approved these changes Oct 19, 2023

View reviewed changes

eseliger reviewed Oct 19, 2023

View reviewed changes

jasonhawkharris and others added 9 commits October 19, 2023 10:38

Update migrations/frontend/1697489421_add_gitlab_topics/down.sql

bdc6742

Co-authored-by: Erik Seliger <erikseliger@me.com>

Update migrations/frontend/1697489421_add_gitlab_topics/up.sql

db11647

Co-authored-by: Erik Seliger <erikseliger@me.com>

Update migrations/frontend/1697558292_comment_on_gitlab_topic_idx/dow…

accfbba

…n.sql Co-authored-by: Erik Seliger <erikseliger@me.com>

Update migrations/frontend/1697558292_comment_on_gitlab_topic_idx/up.sql

d72e6d6

Co-authored-by: Erik Seliger <erikseliger@me.com>

query construction more readable

34bebd0

Merge branch 'main' into jhh/search-gitlab-topics

f14b85a

adjust golden testdata to account for topics

69dc303

Merge branch 'main' into jhh/search-gitlab-topics

8fa8e35

Merge branch 'main' into jhh/search-gitlab-topics

feae35b

jasonhawkharris merged commit 43bf93c into main Oct 19, 2023

jasonhawkharris deleted the jhh/search-gitlab-topics branch October 19, 2023 20:46

camdencheek mentioned this pull request Oct 20, 2023

Clean up querying of repository topics #57755

Merged

		"idx_repo_github_topics" gin (((metadata -> 'RepositoryTopics'::text) -> 'Nodes'::text)) WHERE external_service_type = 'github'::text
		"idx_repo_gitlab_topics" gin ((metadata -> 'topics'::text)) WHERE external_service_type = 'gitlab'::text

Search: enable repo:has.topic() for GitLab #57649

Search: enable repo:has.topic() for GitLab #57649

Uh oh!

Conversation

jasonhawkharris commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Preview 🤩

Uh oh!

sourcegraph-bot commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sourcegraph-bot commented Oct 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BolajiOlajide left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sourcegraph-bot commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camdencheek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eseliger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eseliger Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

camdencheek Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

eseliger Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

camdencheek commented Oct 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Search: enable `repo:has.topic()` for GitLab #57649

Search: enable `repo:has.topic()` for GitLab #57649

jasonhawkharris commented Oct 16, 2023 •

edited

Loading

sourcegraph-bot commented Oct 16, 2023 •

edited

Loading

BolajiOlajide left a comment •

edited

Loading

sourcegraph-bot commented Oct 17, 2023 •

edited

Loading