#112 fix(dbt): balance GL — derive debit/credit from signed amount + strip IsCredit quotes

[Anonymous]

Originally created by: grynn-in

Problem

On real D365 data the GL didn't balance — debit ≠ credit → trial balance off → cash flow off, with every fiscal year imbalanced (millions to trillions). The Close Run assertion suite was Red on the whole warehouse.

The raw D365 source is a clean, balanced double-entry set (all 91,984 vouchers net to 0; AccountingCurrencyAmount sums to 0). Two compounding bugs in our transformation broke it:

1. stg_d365_fo__gl_entries — IsCredit arrives JSON-quoted ("Yes"/"No") from Airbyte, but the parser matched unquoted yes/true/1, so is_credit was ~always 0 → almost every line booked as a debit (512,697 debit / 922 credit).
2. silver_gl_entries — derived debit/credit via abs(amount) keyed on is_credit, which inherited the bad flag and discarded the real sign on contra/reversal lines.

Fix

1. Strip surrounding quotes/whitespace before parsing IsCredit.
2. Derive debit/credit from the sign of the already-signed accounting_currency_amount (positive = debit, negative = credit); net = signed amount. The sign is authoritative (raw provably balances), so this also fixes the contra/reversal lines and no longer depends on the flag.

Verification (real D365, dbt test --select test_type:singular via Close Run)

Assertion	Before	After
assert_silver_gl_debit_credit_balance	188	0
assert_trial_balance_balances	1,165	0
assert_cf_categories_equal_net_change	1,165	0
assert_cta_zero_for_same_currency	144	0

Every fiscal year's TB now nets to 0. Close Run went 67/3/2 → 70/2/0 (remaining: fiscal-calendar [#111], a dynamic-step-count check).

Deploy note

bronze_general_journal_account_entries is incremental — a deployment with pre-fix rows needs a one-time dbt run --full-refresh to backfill is_credit and clear stale rows.

🤖 Generated with Claude Code

[Anonymous]

Originally posted by: grynn-in

Review — fix(dbt): balance GL — derive debit/credit from signed amount + strip IsCredit quotes

Reviewed adversarially in two passes (correctness/data-integrity, then nits/edge-cases) in an isolated worktree off origin/main. The core change is sound and actually brings silver_gl_entries into line with the documented canonical contract.

Why this is correct

• The canonical contract is "amount = debit − credit (signed)", spelled out in models/staging/erpnext/stg_erpnext__gl_entries.sql:14-16, and both adapters honour it: D365 passes raw signed AccountingCurrencyAmount; ERPNext emits debit - credit.
• New derivation: debit_amount = max(amount,0), credit_amount = max(-amount,0). Therefore debit_amount - credit_amount = amount exactly. The TB balances iff sum(amount) = 0 per voucher/entity — which is the same invariant the old code relied on, just sourced from the sign instead of a flag.
• It is also equivalent to the old is_credit-based split whenever is_credit agreed with the sign, and strictly more robust when it doesn't (the very bug being fixed). Mutually exclusive (one leg is 0 unless amount=0), so no double-counting. Decimal128 comparisons/negation are well-defined in ClickHouse.

Blocking issues

None.

Non-blocking nits

1. Stale doc comment — models/staging/d365_fo/stg_d365_fo__gl_entries.sql:7-9 still says "bronze/silver handle debit/credit splitting using is_credit." That is now false; silver splits on the sign. Please update.
2. Stale doc comment — models/staging/erpnext/stg_erpnext__gl_entries.sql:14-16 says "silver takes abs(amount) and splits debit/credit by the is_credit flag." Also no longer true (and the abs() it references is gone).
3. is_credit is now dead for computation — after this PR no model consumes is_credit for debit/credit splitting (only carried through stg_gl_entries → bronze_general_journal_account_entries). So the staging quote-strip fix (stg_d365_fo__gl_entries.sql:48) no longer affects the trial balance at all — it only corrects the stored is_credit column for ad-hoc/analytics use. The two halves of this PR are independent: only the silver change moves the TB. Worth a one-line note in the PR/commit so nobody assumes the staging fix is load-bearing.
4. Balance guard is only a warning — tests/assert_silver_gl_debit_credit_balance.sql:3 is severity='warn'. With the new derivation sum(debit)-sum(credit) ≡ sum(amount), so this test now is the verification of the PR's central claim ("every voucher nets to 0"). If the sign assumption is ever violated (e.g. an entity that stores AccountingCurrencyAmount unsigned), everything books as a debit and you get only a warning, not a failure. Consider escalating to error for D365 real data, or documenting why warn is intentional.
5. Trim edge case — trim(both '"' from trim(toString(...))) (stg_d365_fo__gl_entries.sql:48) strips outer whitespace then quotes, but not whitespace inside quotes (e.g. "Yes "). Harmless given nit [#3], but if you ever rely on is_credit again, normalize inside the quotes too.

Verification

• dbt parse --no-partial-parse --profiles-dir . → exit 0 (clean). Only pre-existing, unrelated MissingArgumentsPropertyInGenericTestDeprecation warnings on gold_bs_movement.
• Did not run dbt run/build (would mutate live ClickHouse).

MERGE RECOMMENDATION: MERGE-WITH-NITS

[Anonymous]

Ticket changed by: grynn-in

• status: open --> closed