Skip to content

Filter out Copr builds without SRPM in SQL #2863

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

Open
m-blaha wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Filter out Copr builds without SRPM in SQL #2863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 14 additions & 0 deletions packit_service/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
desc,
func,
null,
or_,
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 19, 2025

Choose a reason for hiding this comment

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

medium

To improve the readability of the filter condition being added, consider importing and_ and not_ instead of or_. This will allow expressing the filter logic in a way that more directly mirrors the case being excluded, making the code easier to understand.

Suggested change
or_,
and_,
not_,

select,
)
from sqlalchemy.dialects.postgresql import array as psql_array
Expand Down Expand Up @@ -2240,6 +2241,19 @@ def get_merged_chroots(
"packit_id_per_chroot",
),
)
.filter(
# Exclude builds without build_id (to not mix targets from
# different builds together)
CoprBuildTargetModel.build_id.isnot(None),
# Exclude builds waiting for SRPM - not actual Copr builds yet
CoprBuildTargetModel.status != BuildStatus.waiting_for_srpm,
# Exclude SRPM build failures (no build_start_time/build_logs_url)
or_(
CoprBuildTargetModel.status != BuildStatus.failure,
CoprBuildTargetModel.build_start_time.isnot(None),
CoprBuildTargetModel.build_logs_url.isnot(None),
),
Comment on lines +2251 to +2255
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 19, 2025

Choose a reason for hiding this comment

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

medium

While this or_ condition is logically correct, it represents the inverted logic of what is being excluded. Using not_(and_(...)) would be more readable as it directly translates the condition for an SRPM build failure that should be excluded: status == failure AND build_start_time IS NULL AND build_logs_url IS NULL. This makes the intent of the filter clearer and closer to the original Python logic that was removed.

Suggested change
or_(
CoprBuildTargetModel.status != BuildStatus.failure,
CoprBuildTargetModel.build_start_time.isnot(None),
CoprBuildTargetModel.build_logs_url.isnot(None),
),
not_(
and_(
CoprBuildTargetModel.status == BuildStatus.failure,
CoprBuildTargetModel.build_start_time.is_(None),
CoprBuildTargetModel.build_logs_url.is_(None),
)
),

)
.group_by(
CoprBuildTargetModel.build_id,
) # Group by identical element(s)
Expand Down
10 changes: 0 additions & 10 deletions packit_service/service/api/copr_builds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from flask_restx import Namespace, Resource

from packit_service.models import (
BuildStatus,
CoprBuildGroupModel,
CoprBuildTargetModel,
optional_timestamp,
Expand Down Expand Up @@ -35,15 +34,6 @@ def get(self):
first, last = indices()
for build in CoprBuildTargetModel.get_merged_chroots(first, last):
build_info = CoprBuildTargetModel.get_by_build_id(build.build_id, None)
if build_info.status == BuildStatus.waiting_for_srpm:
continue
if (
build_info.status == BuildStatus.failure
and not build_info.build_start_time
and not build_info.build_logs_url
):
# SRPM build failed, it doesn't make sense to list this build
continue
project_info = build_info.get_project()
Comment on lines 35 to 37
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 4, 2025

Choose a reason for hiding this comment

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

medium

This loop currently causes an N+1 query problem. For each build returned by get_merged_chroots, you're making at least two more database queries: get_by_build_id and then get_project (which itself can trigger multiple lazy-loads). This can lead to significant performance degradation, especially with a large number of builds.

To resolve this, I recommend modifying CoprBuildTargetModel.get_merged_chroots to fetch all the necessary information in a single query by using joins and returning all required fields. This would eliminate the need for extra queries inside the loop.

For example, you could extend the query in get_merged_chroots to join with CoprBuildGroupModel, PipelineModel, ProjectEventModel, and GitProjectModel to retrieve fields like project_name, build_submitted_time, web_url, commit_sha, and project details. You would need to use an aggregate function (like min or max) on these additional fields within the group_by clause, since they will be the same for all chroots of a given build.

This would make the API endpoint much more performant.

Copy link
Member Author

@m-blaha m-blaha Nov 4, 2025

Choose a reason for hiding this comment

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

This makes sense, but is out of scope here. If we wanted to optimize the code, it should be tracked as a separate issue.

build_dict = {
"packit_id": build_info.id,
Expand Down
Loading