adding pull_request_id index to pull_request_commits/comments tables #7559

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

backend/core/models/migrationscripts/20240603_add_pull_request_id_index_for_pr_commits_table.go

+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
9 of 10 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding pull_request_id index to pull_request_commits/comments tables #7559

adding pull_request_id index to pull_request_commits/comments tables #7559

sstojak1 commented Jun 2, 2024 •

edited

Loading

d4x1 Jun 3, 2024

sstojak1 Jun 3, 2024

sstojak1 Jun 3, 2024

klesh Jun 3, 2024

sstojak1 Jun 3, 2024 •

edited

Loading

klesh Jun 3, 2024

sstojak1 Jun 3, 2024 •

edited

Loading

klesh left a comment •

edited

Loading

sstojak1 commented Jun 4, 2024

sstojak1 commented Jun 6, 2024 •

edited

Loading

klesh commented Jun 6, 2024

klesh commented Jun 6, 2024

sstojak1 commented Jun 6, 2024 •

edited

Loading

klesh left a comment

klesh commented Jun 7, 2024

sstojak1 commented Jun 7, 2024

sstojak1 commented Jun 10, 2024

abeizn commented Jul 16, 2024

adding pull_request_id index to pull_request_commits/comments tables #7559

adding pull_request_id index to pull_request_commits/comments tables #7559

Conversation

sstojak1 commented Jun 2, 2024 • edited Loading

⚠️ Pre Checklist

Summary

Does this close any open issues?

Screenshots

Other Information

d4x1 Jun 3, 2024

Choose a reason for hiding this comment

sstojak1 Jun 3, 2024

Choose a reason for hiding this comment

sstojak1 Jun 3, 2024

Choose a reason for hiding this comment

klesh Jun 3, 2024

Choose a reason for hiding this comment

sstojak1 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

klesh Jun 3, 2024

Choose a reason for hiding this comment

sstojak1 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

klesh left a comment • edited Loading

Choose a reason for hiding this comment

sstojak1 commented Jun 4, 2024

sstojak1 commented Jun 6, 2024 • edited Loading

klesh commented Jun 6, 2024

klesh commented Jun 6, 2024

sstojak1 commented Jun 6, 2024 • edited Loading

klesh left a comment

Choose a reason for hiding this comment

klesh commented Jun 7, 2024

sstojak1 commented Jun 7, 2024

sstojak1 commented Jun 10, 2024

abeizn commented Jul 16, 2024

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