Single-pass totals computation for Trial Balance Excel (Totaling accounts)

[mynjj]

Summary

The query-based Trial Balance Excel report (codeunit 4410 "Trial Balance") synthesized Total/End-Total rows by looping over every Total account and, for each one, re-filtering the buffer and CalcSums-ing once per distinct (Dimension 1, Dimension 2, Business Unit) combination. On charts of accounts with many totals this is O(totals × combinations) passes over the buffer and dominated the cost of large trial balances.

This change computes all totals in a single sweep:

• BuildAccountToTotalsMap maps each posting account to the list of totals whose Totaling range contains it.
• DistributePostingRowsToTotals sweeps the posting rows once, accumulating each row into every containing total's in-memory buffer (keyed by account + dimensions + business unit + period).
• MergeTotalsIntoBuffer runs the budget comparison / all-zero checks and inserts the resulting totals.

Because the map is built only from posting accounts, a parent End-Total whose range spans a nested child End-Total's number no longer re-sums the child's already-inserted buffer row — fixing a latent double-counting bug for nested totals.

Fix to allow closing dates to be specified as starting dates is also included.

Fixes AB#638251
Fixes AB#638353
Fixes AB#640630

[github-actions]

⚠️ Stale Status Check Deleted

The Pull Request Build workflow run for this PR was older than 72 hours and has been deleted.

📋 Why was it deleted?

Status checks that are too old may no longer reflect the current state of the target branch. To ensure this PR is validated against the latest code and passes up-to-date checks, a fresh build is required.

---

🔄 How to trigger a new status check:

1. 📤 Push a new commit to the PR branch, or
2. 🔁 Close and reopen the PR

This will automatically trigger a new Pull Request Build workflow run.

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Dictionary list update silently discarded on existing key

In AL, Dictionary.Get(Key, var Value) copies the stored List of [T] value into the local variable. The subsequent ContainingTotals.Add(TotalAccountNo) mutates only that local copy; the dictionary entry is never updated. Any posting account that belongs to more than one totaling range will therefore only aggregate into the first total account encountered, silently producing incorrect (incomplete) total rows for every subsequent overlapping total.

Recommendation:

• After the Add call in the if-branch, write the updated list back to the dictionary with AccountToTotals.Set(PostingAccountNo, ContainingTotals).

Suggested change

	ContainingTotals.Add(TotalAccountNo)
	if AccountToTotals.Get(PostingAccountNo, ContainingTotals) then begin
	ContainingTotals.Add(TotalAccountNo);
	AccountToTotals.Set(PostingAccountNo, ContainingTotals); // persist the mutation
	end else begin
	ContainingTotals.Add(TotalAccountNo);
	AccountToTotals.Add(PostingAccountNo, ContainingTotals);
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Record assignment across temporary instances may lose context

TrialBalanceData := TotalsBuffer copies field values from one temporary table instance into the other's record variable. In AL, assigning between two separate temporary instances of the same table type is defined behaviour for field values, but calling TrialBalanceData.Insert(true) immediately after relies on the target record variable retaining its own temporary table context. If the runtime instead routes the Insert to TotalsBuffer's context, the row is silently dropped from the output buffer.

Recommendation:

• Copy individual fields explicitly (or use a dedicated helper) and then call TrialBalanceData.Insert(true), making it unambiguous which table receives the row.

repeat
    TrialBalanceData.Init();
    TrialBalanceData."G/L Account No." := TotalsBuffer."G/L Account No.";
    TrialBalanceData."Dimension 1 Code" := TotalsBuffer."Dimension 1 Code";
    TrialBalanceData."Dimension 2 Code" := TotalsBuffer."Dimension 2 Code";
    TrialBalanceData."Business Unit Code" := TotalsBuffer."Business Unit Code";
    TrialBalanceData."Period Start" := TotalsBuffer."Period Start";
    TrialBalanceData.TransferFields(TotalsBuffer);
    TrialBalanceData.CalculateBudgetComparisons();
    TrialBalanceData.CheckAllZero();
    if not TrialBalanceData."All Zero" then
        TrialBalanceData.Insert(true);
until TotalsBuffer.Next() = 0;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

N+1 query pattern in total-account map build

Inside the repeat..until TotalAccount.Next() = 0 loop, PostingAccount.Reset() and FindSet() issue a new G/L Account database query for every Total/End-Total account found. A chart of accounts with many total headers will generate N separate round-trips, potentially causing noticeable latency when the report is run.

Recommendation:

• Load all Posting-type G/L accounts into a temporary record once before the loop, then use SetFilter / FindSet against that temporary set instead of going back to the database on each iteration.

Suggested change

	PostingAccount.Reset();
	// Before the TotalAccount loop, load all posting accounts into a temp record:
	TempPostingAccount.Reset();
	TempPostingAccount.DeleteAll();
	PostingAccount.SetRange("Account Type", "G/L Account Type"::Posting);
	if PostingAccount.FindSet() then
	repeat
	TempPostingAccount := PostingAccount;
	TempPostingAccount.Insert();
	until PostingAccount.Next() = 0;
	// Inside the loop, query the temp table:
	TempPostingAccount.SetFilter("No.", TotalAccount.Totaling);
	if TempPostingAccount.FindSet() then
	repeat
	AddTotalForAccount(AccountToTotals, TempPostingAccount."No.", TotalAccount."No.");
	until TempPostingAccount.Next() = 0;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

GetOpeningBalanceCutoff comment contradicts behaviour

When StartDate is already a closing date, the function returns NormalDate(StartDate) — the same calendar day, not the day before. The inline comment says "the date immediately before the starting date", which is misleading and risks future maintainers misreading the intent, potentially introducing an off-by-one regression when this function is modified.

Recommendation:

• Rewrite the comment to describe what the returned value means in terms of BC's date-filter semantics: it is the latest date that should fall inside the opening balance filter, keeping the closing-date entry (or the normal entries on StartDate when StartDate is a closing date) inside the reported period.

Suggested change

	// We return the date immediately before the starting date, considering BC's date ordering and the presence of closing dates
	local procedure GetOpeningBalanceCutoff(StartDate: Date): Date
	begin
	// Returns the upper boundary for the opening-balance filter ('..X').
	// If StartDate is already a closing date, the normal date of that day is the boundary:
	// closing entries for that day fall INSIDE the period (not the opening balance).
	// If StartDate is a normal date, all closing entries of StartDate-1 belong to the opening balance.
	if StartDate = ClosingDate(StartDate) then
	exit(NormalDate(StartDate));
	exit(ClosingDate(StartDate - 1));
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Nested-totals test missing dimension and budget assertions

QueryPathDoesNotDoubleCountNestedTotals only verifies Balance on the default (empty) dimension combination. The AddRowToTotal function keys on (G/L Account No., Dimension 1 Code, Dimension 2 Code, Business Unit Code, Period Start), so a posting account with dimension values in multiple overlapping totaling ranges is untested. Additionally, the budget accumulation path in AddRowToTotal is not exercised by this test, meaning a regression there would go undetected.

Recommendation:

• Add a second scenario with at least one non-empty dimension combination and verify that both the child and parent total rows carry the correct dimension-specific balance. Add a parallel budget-data variant using ConfigureTrialBalance(false, true) and assert Budget (Net) on the total rows.

Suggested change

	TrialBalance.InsertTrialBalanceReportData(GLAccount, TempDimension1Values, TempDimension2Values, TempTrialBalanceData);
	// Extend the test or add a new procedure:
	// [GIVEN] Post entries with Dimension 1 = 'DEPT1'
	// [THEN] Child End-Total row for Dimension 1 = 'DEPT1' has Balance = EntryAmount
	// [THEN] Parent End-Total row for Dimension 1 = 'DEPT1' also has Balance = EntryAmount (not doubled)
	// Similarly assert Budget (Net) after ConfigureTrialBalance(false, true)

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

TotalAccount (G/L Account) is iterated via FindSet in BuildAccountToTotalsMap without a preceding SetLoadFields call.

Inside the loop only the Totaling field is read; No. and Account Type are included automatically as the primary-key and filter fields respectively. G/L Account is a wide table and the loop may iterate well above ten Total/End-Total accounts in a large chart of accounts, which is the threshold above which the best practice requires SetLoadFields. Adding SetLoadFields("Totaling") before FindSet limits each row transfer to just the one field the body needs.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

        TotalAccount.SetLoadFields("Totaling");
        if not TotalAccount.FindSet() then

Knowledge:

• microsoft/knowledge/performance/use-setloadfields-for-partial-records.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

Copilot PR Review

Iteration 1 · Outcome: completed

Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630

Findings by domain

Findings split into Knowledge-backed (cite a BCQuality article) and Agent (the agent's own judgement, no matching BCQuality rule).

Domain	Findings	Knowledge-backed	Agent	Inline	Fallback
Performance	1	1	0	1	0

Totals: 1 knowledge-backed · 0 agent findings.

Orchestrator pre-filter (13 file(s) excluded)

• layer-disabled (knowledge) : 13 file(s)

_{Findings produced by the Copilot CLI agent against BCQuality at 822cae1b2771ac25f665f73369f69093bd4fd630. Reply 👎 on any inline comment to flag false positives.}