Skip to content

Further performance update for my-reviews #144

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

Merged
jmgasper merged 2 commits into from
Nov 2, 2025
Merged

Further performance update for my-reviews #144

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cab9f46
Further performance update for my-reviews
jmgasper Nov 2, 2025
b9c3b5f
Merge pull request #143 from topcoder-platform/performance
jmgasper Nov 2, 2025
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
155 changes: 109 additions & 46 deletions src/api/my-review/myReview.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,47 @@ export class MyReviewService {
)
`,
);
cteFragments.push(
Prisma.sql`
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

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

[⚠️ performance]
The use of COUNT(*)::bigint and SUM(...)::bigint is consistent with the previous implementation, but ensure that the casting to bigint is necessary for the application logic. If the counts are expected to be within the range of integer, this casting might be unnecessary and could be optimized.

review_totals AS (
SELECT
rv."resourceId",
COUNT(*)::bigint AS "totalReviews",
COALESCE(
SUM(CASE WHEN rv.status = 'COMPLETED' THEN 1 ELSE 0 END),
0
)::bigint AS "completedReviews"
FROM reviews.review rv
WHERE rv."resourceId" IN (
SELECT mr.id FROM member_resources mr
)
GROUP BY rv."resourceId"
)
`,
);
cteFragments.push(
Prisma.sql`
incomplete_reviews AS (
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

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

[⚠️ correctness]
The DISTINCT ON clause is used here to select unique resourceId entries. Ensure that this approach aligns with the intended logic, as DISTINCT ON can sometimes lead to unexpected results if the ordering is not carefully considered.

SELECT DISTINCT ON (rv."resourceId")
rv."resourceId",
TRUE AS "hasIncompleteReviews",
cp_incomplete.name AS "incompletePhaseName"
FROM reviews.review rv
JOIN challenges."ChallengePhase" cp_incomplete
ON cp_incomplete.id = rv."phaseId"
WHERE rv."resourceId" IN (
SELECT mr.id FROM member_resources mr
)
AND (rv.status IS NULL OR rv.status <> 'COMPLETED')
ORDER BY
rv."resourceId",
CASE WHEN cp_incomplete."isOpen" IS TRUE THEN 0 ELSE 1 END,
cp_incomplete."scheduledEndDate" NULLS LAST,
cp_incomplete."actualEndDate" NULLS LAST,
cp_incomplete.name ASC
)
`,
);
cteFragments.push(
Prisma.sql`
appeals_phase AS (
Expand Down Expand Up @@ -252,7 +293,9 @@ export class MyReviewService {
`,
);

const metricJoins: Prisma.Sql[] = [
const metricJoins: Prisma.Sql[] = [];

metricJoins.push(
Prisma.sql`
LEFT JOIN LATERAL (
SELECT
Expand All @@ -268,18 +311,28 @@ export class MyReviewService {
LIMIT 1
) cp ON TRUE
`,
Prisma.sql`
LEFT JOIN LATERAL (
SELECT
COUNT(*)::bigint AS "totalReviews",
COALESCE(
SUM(CASE WHEN rv.status = 'COMPLETED' THEN 1 ELSE 0 END),
0
)::bigint AS "completedReviews"
FROM reviews.review rv
WHERE rv."resourceId" = r.id
) rp ON r.id IS NOT NULL
`,
);

const reviewTotalsJoin = adminUser
? Prisma.sql`
LEFT JOIN LATERAL (
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

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

[⚠️ correctness]
The conditional logic for adminUser results in different SQL joins. Verify that the logic correctly handles all cases, especially when transitioning between admin and non-admin users, to ensure that the correct data is retrieved.

SELECT
COUNT(*)::bigint AS "totalReviews",
COALESCE(
SUM(CASE WHEN rv.status = 'COMPLETED' THEN 1 ELSE 0 END),
0
)::bigint AS "completedReviews"
FROM reviews.review rv
WHERE rv."resourceId" = r.id
) review_totals ON r.id IS NOT NULL
`
: Prisma.sql`
LEFT JOIN review_totals
ON review_totals."resourceId" = r.id
`;
metricJoins.push(reviewTotalsJoin);

metricJoins.push(
Prisma.sql`
LEFT JOIN LATERAL (
SELECT
Expand All @@ -296,36 +349,46 @@ export class MyReviewService {
WHERE w."challengeId" = c.id
) cw ON TRUE
`,
Prisma.sql`
LEFT JOIN LATERAL (
SELECT
EXISTS (
SELECT 1
FROM reviews.review rv_incomplete
WHERE rv_incomplete."resourceId" = r.id
AND (rv_incomplete.status IS NULL OR rv_incomplete.status <> 'COMPLETED')
) AS "hasIncompleteReviews",
(
SELECT cp_incomplete.name
FROM reviews.review rv_incomplete2
JOIN challenges."ChallengePhase" cp_incomplete
ON cp_incomplete.id = rv_incomplete2."phaseId"
WHERE rv_incomplete2."resourceId" = r.id
AND (rv_incomplete2.status IS NULL OR rv_incomplete2.status <> 'COMPLETED')
ORDER BY
CASE WHEN cp_incomplete."isOpen" IS TRUE THEN 0 ELSE 1 END,
cp_incomplete."scheduledEndDate" NULLS LAST,
cp_incomplete."actualEndDate" NULLS LAST,
cp_incomplete.name ASC
LIMIT 1
) AS "incompletePhaseName"
) deliverable_reviews ON r.id IS NOT NULL
`,
);

const incompleteReviewsJoin = adminUser
? Prisma.sql`
LEFT JOIN LATERAL (
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The logic for determining hasIncompleteReviews and incompletePhaseName is duplicated for admin and non-admin users. Consider refactoring to reduce redundancy and improve maintainability.

SELECT
EXISTS (
SELECT 1
FROM reviews.review rv_incomplete
WHERE rv_incomplete."resourceId" = r.id
AND (rv_incomplete.status IS NULL OR rv_incomplete.status <> 'COMPLETED')
) AS "hasIncompleteReviews",
(
SELECT cp_incomplete.name
FROM reviews.review rv_incomplete2
JOIN challenges."ChallengePhase" cp_incomplete
ON cp_incomplete.id = rv_incomplete2."phaseId"
WHERE rv_incomplete2."resourceId" = r.id
AND (rv_incomplete2.status IS NULL OR rv_incomplete2.status <> 'COMPLETED')
ORDER BY
CASE WHEN cp_incomplete."isOpen" IS TRUE THEN 0 ELSE 1 END,
cp_incomplete."scheduledEndDate" NULLS LAST,
cp_incomplete."actualEndDate" NULLS LAST,
cp_incomplete.name ASC
LIMIT 1
) AS "incompletePhaseName"
) deliverable_reviews ON r.id IS NOT NULL
`
: Prisma.sql`
LEFT JOIN incomplete_reviews deliverable_reviews
ON deliverable_reviews."resourceId" = r.id
`;
metricJoins.push(incompleteReviewsJoin);

metricJoins.push(
Prisma.sql`
LEFT JOIN reviews.review_pending_summary pending_appeals
ON pending_appeals."resourceId" = r.id
`,
];
);

if (!adminUser) {
metricJoins.push(
Expand Down Expand Up @@ -374,9 +437,6 @@ export class MyReviewService {
);
const countJoinClause = joinSqlFragments(countJoins, Prisma.sql``);

const rowWhereFragments = [...whereFragments, ...rowExtras];
const countWhereFragments = [...whereFragments, ...countExtras];

if (challengeTypeId) {
whereFragments.push(Prisma.sql`c."typeId" = ${challengeTypeId}`);
}
Expand All @@ -397,6 +457,9 @@ export class MyReviewService {
);
}

const rowWhereFragments = [...whereFragments, ...rowExtras];
const countWhereFragments = [...whereFragments, ...countExtras];

const rowWhereClause = joinSqlFragments(
rowWhereFragments,
Prisma.sql` AND `,
Expand Down Expand Up @@ -425,12 +488,12 @@ export class MyReviewService {
'topgear iterative review'
) THEN NULL
ELSE CASE
WHEN rp."totalReviews" IS NULL OR rp."totalReviews" = 0 THEN 0
WHEN review_totals."totalReviews" IS NULL OR review_totals."totalReviews" = 0 THEN 0
ELSE LEAST(
1,
GREATEST(
0,
rp."completedReviews"::numeric / rp."totalReviews"::numeric
review_totals."completedReviews"::numeric / review_totals."totalReviews"::numeric
)
)
END
Expand Down Expand Up @@ -529,8 +592,8 @@ export class MyReviewService {
cp."actualEndDate" AS "currentPhaseActualEnd",
rr.name AS "resourceRoleName",
c."endDate" AS "challengeEndDate",
rp."totalReviews" AS "totalReviews",
rp."completedReviews" AS "completedReviews",
review_totals."totalReviews" AS "totalReviews",
review_totals."completedReviews" AS "completedReviews",
cw.winners AS "winners",
deliverable_reviews."hasIncompleteReviews" AS "hasIncompleteReviews",
deliverable_reviews."incompletePhaseName" AS "incompletePhaseName",
Expand Down