adding pull_request_id index to pull_request_commits/comments tables by sstojak1 · Pull Request #7559 · apache/devlake

sstojak1 · 2024-06-02T08:39:20Z

⚠️ Pre Checklist

Please complete ALL items in this checklist, and remove before submitting

I have read through the Contributing Documentation.
I have added relevant tests.
I have added relevant documentation.
I will add labels to the PR, such as pr-type/bug-fix, pr-type/feature-development, etc.

Summary

What does this PR do?
Adding pull_request_id index to pull_request_commits and pull_request_comments tables.

Does this close any open issues?

Closes 7557

Screenshots

Include any relevant screenshots here.

Other Information

Any other information that is important to this PR.

d4x1 · 2024-06-03T02:40:16Z

+type addPullRequestIdIndexToPullRequestCommits struct{}
+
+type pullRequestCommits20240602 struct {
+	PullRequestId string `gorm:"index"`


https://github.com/apache/incubator-devlake/blob/main/backend/core/models/domainlayer/code/pull_request_commit.go#L28, PullRequestId and CommitSha are the primary keys, and it works as expected in db tables.

So I think there is no need to add a new index in this field.

true in case you are filtering by CommitSha or CommitSha + PullRequestId.
If you are filtering only by PullRequestId the composite index won't be used - docs

with PullRequestId index:

Ahh.. The order of the primary key columns seems off to me, the PullRequestId should be placed before the CommitSha column.
However, adding a secondary index is acceptable IMO if adjusting the order is too hard to do so.

This could work I think:
ALTER TABLE pull_request_commits DROP PRIMARY KEY;
ALTER TABLE pull_request_commits ADD PRIMARY KEY (pull_request_id, commit_sha);
However, this change might impact the rest of the application, especially if there are custom queries that end users have that expect commit_sha to be first in the index sequence.

What do you think?

@sstojak1 True, we should consider which one is more likely to happen. It is easier to predict someone might need to group records by pull_request_id while it is harder to think of how commit_sha being prioritized could be beneficial for any case, maybe "Counting how many PRs contain the same commit?" which doesn't make too much sense.

I agree. I changed the pull_request_commits script. Now it's changing the primary key columns order.

# Conflicts: # backend/core/models/migrationscripts/register.go

klesh

Why are two tables handled differently?
Besides, can we support postgresql as well? Apache DevLake supports pg as a storage option.

sstojak1 · 2024-06-04T13:43:21Z

Why are two tables handled differently? Besides, can we support postgresql as well? Apache DevLake supports pg as a storage option.

Those two tables don't have the same primary key definition. That's the reason why migration scripts are different.

Is the PostgreSQL supported? There are also a few scripts where this kind of schema check is added...
Also, check this out:
https://devlake-io.slack.com/archives/C03APJ20VM4/p1713164942109869
https://devlake-io.slack.com/archives/C03APJ20VM4/p1716386100745249

sstojak1 · 2024-06-06T05:54:41Z

Hi @klesh any updates here? Did you manage to check my last comment?

klesh · 2024-06-06T08:58:51Z

@sstojak1 Hi, sorry for the late reply.
We support using PostgreSQL as a storage option for advanced users who prefer writing their own dashboards without any preset dashboards.
In other words: the devlake implementation written in Golang supports both PG and Mysql while the embedded dashboards for grafana only available for Mysql

klesh · 2024-06-06T09:03:35Z

@sstojak1 And yes, we have some scripts designated MySQL, sometimes because they are MySQL specific nuances, which were unnecessary for PG from what I know.

sstojak1 · 2024-06-06T12:31:41Z

@klesh makes sense, thanks for the info. I have added support for PostgreSQL. Please take a look.
Result when locally running Devlake with PostgreSQL:

klesh

LGTM

klesh · 2024-06-07T08:56:00Z

Hi, @sstojak1 we are so close, there is one conflict left before I can merge the PR. Would you like to resolve it? Thanks.

# Conflicts: # backend/core/models/migrationscripts/register.go

sstojak1 · 2024-06-07T09:47:18Z

@klesh done!

sstojak1 · 2024-06-10T13:14:07Z

@klesh can we merge it? :)

…7559) * adding pull_request_id index to pull_request_commits/comments tables * change the pull_request_commits primary key columns order * adding Apache license header * only run for mysql * adding support for postgres --------- Co-authored-by: Josip Stojak <Josip.Stojak@infobip.com>

…7559) (#7744) * adding pull_request_id index to pull_request_commits/comments tables * change the pull_request_commits primary key columns order * adding Apache license header * only run for mysql * adding support for postgres --------- Co-authored-by: sstojak1 <18380216+sstojak1@users.noreply.github.com> Co-authored-by: Josip Stojak <Josip.Stojak@infobip.com>

abeizn · 2024-07-16T09:41:02Z

@sstojak1 Already released in v1.0.1-beta3

adding pull_request_id index to pull_request_commits/comments tables

b0df93a

dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. component/framework This issue or PR relates to the framework type/refactor This issue is to refactor existing code labels Jun 2, 2024

d4x1 reviewed Jun 3, 2024

View reviewed changes

Josip Stojak added 4 commits June 3, 2024 13:34

change the pull_request_commits primary key columns order

93f59df

Merge branch 'refs/heads/main' into feat-add-pull-request-id-index

61c8a6e

# Conflicts: # backend/core/models/migrationscripts/register.go

adding Apache license header

27ccff9

only run for mysql

75a2789

dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jun 3, 2024

sstojak1 requested a review from klesh June 3, 2024 19:13

klesh reviewed Jun 4, 2024

View reviewed changes

sstojak1 requested a review from klesh June 4, 2024 13:43

adding support for postgres

a03564c

Merge branch 'refs/heads/main' into feat-add-pull-request-id-index

d4a3bde

klesh previously approved these changes Jun 7, 2024

View reviewed changes

dosubot Bot added the lgtm This PR has been approved by a maintainer label Jun 7, 2024

Merge branch 'refs/heads/main' into feat-add-pull-request-id-index

a87a697

# Conflicts: # backend/core/models/migrationscripts/register.go

sstojak1 dismissed klesh’s stale review via a87a697 June 7, 2024 09:46

klesh approved these changes Jun 14, 2024

View reviewed changes

klesh merged commit 13b0b43 into apache:main Jun 14, 2024

sstojak1 deleted the feat-add-pull-request-id-index branch June 14, 2024 10:33

klesh mentioned this pull request Nov 20, 2024

cherrypick #8201 to release v1.0 #8202

Merged

Uh oh!

Conversation

sstojak1 commented Jun 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ Pre Checklist

Summary

Does this close any open issues?

Screenshots

Other Information

Uh oh!

d4x1 Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

sstojak1 Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

sstojak1 Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

klesh Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

sstojak1 Jun 3, 2024 • 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!

klesh Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

sstojak1 Jun 3, 2024 • 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!

klesh 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!

sstojak1 commented Jun 4, 2024

Uh oh!

sstojak1 commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klesh commented Jun 6, 2024

Uh oh!

klesh commented Jun 6, 2024

Uh oh!

sstojak1 commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klesh left a comment

Choose a reason for hiding this comment

Uh oh!

klesh commented Jun 7, 2024

Uh oh!

sstojak1 commented Jun 7, 2024

Uh oh!

sstojak1 commented Jun 10, 2024

Uh oh!

abeizn commented Jul 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sstojak1 commented Jun 2, 2024 •

edited

Loading

sstojak1 Jun 3, 2024 •

edited

Loading

sstojak1 Jun 3, 2024 •

edited

Loading

klesh left a comment •

edited

Loading

sstojak1 commented Jun 6, 2024 •

edited

Loading

sstojak1 commented Jun 6, 2024 •

edited

Loading