Safwan Ahmed (2ab7b36c) at 18 Mar 00:25

Thanks for the work here @vyaklushin LGTM! 👍🏽. I was wondering if the main source of redundancy is overall_approver_ids. Given that we pass the same MR to all approval rules, and the MR model memoizes committer_ids_to_filter_from_approvers and committers_to_filter_from_approvers internally already

What does this MR do and why?

Contributes to #589598

Problem

When evaluating approval rules for a merge request, each ApprovalWrappedRule independently computes overall_approver_ids and committer filtering data. This causes N redundant computations where N is the number of approval rules, leading to slow performance on MRs with many rules.

Solution

Introduce Approvals::Context class to hold pre-computed shared data across all approval rules. The context is created once in ApprovalState and passed through to wrapped rules, providing:

• overall_approver_ids: Set of user IDs who approved the MR
• committer_ids_for_filtering: Committer IDs for AR::Relation filter
• committers_for_filtering: Committer users for Array filtering

References

• #589598

Screenshots or screen recordings

N/A - Performance optimization with no UI changes.

How to set up and validate locally

1. Create an MR with multiple approval rules (especially CODEOWNERS rules)
2. Query the approvalSummary GraphQL endpoint
3. Verify reduced CPU time compared to before this change

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

What does this MR do and why?

Follow up on !225446. It appears the namespace.traversal_ids datatype on local is bigint[] while on staging (and possibly other deployments) is int[]. This can lead to type incompatibility when comparing bigint[] and int[].

This MR puts an existing check into a utility method to determine the traversl_ids type, to help determine what to cast arguments to.

References

#585222

Screenshots or screen recordings

Before	After

How to set up and validate locally

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Safwan Ahmed (7fd32478) at 17 Mar 12:00

Safwan Ahmed (ce9c501e) at 16 Mar 17:07

👋🏿 @patrickbajao can I get a Merge Requests backend review on this please? Thanks!

Something to note:

I was going to query nil records as well, but it seems the flag would only be enabled when the backfill for IDs is complete right? Basing this on the changes in Make merge_request_diff_commits queries partiti... (!214768 - merged) also do not query nil records when the FF is enabled.

Note

This is a literal command with the same query; AFAIK, it should not require a new plan.

Note

This is a literal command with the same query; AFAIK, it should not require a new plan.

Note

We want to decouple the flag from saving project_id.

See comment https://gitlab.com/gitlab-org/gitlab/-/work_items/527237#note_3141964530

Note

Used in specs for the pre-backfill world, this will be removed along with diff_commit_without_metadata, when backfill is complete

Safwan Ahmed (0c8bb2b5) at 16 Mar 13:03

👋🏼 @egrieff I made some changes and the approvals reset, mind giving another look please? thanks!

Safwan Ahmed (f84c3ff9) at 16 Mar 10:17

... and 1360 more commits

@tskorupa-gl I was thinking of not making it a scope now since it is experimental and may not yield the intended results. Keeping it in this class makes it local for now, I can move it when I can confirm it is stable, WDYT?

Safwan Ahmed (36452294) at 16 Mar 10:00

Safwan Ahmed (e529ee07) at 16 Mar 09:57