#121 fix: review follow-ups #118 (GL sign) + #119 (scope/period filters) + #120 (demo FX data)

[Anonymous]

Originally created by: grynn-in

Resolves the review follow-ups filed after #112/#115/#104: #118, #119, #120.

[#118] — code (GL sign refactor follow-ups)

• Retire dead is_credit — grepped all of dbt + the konsol app; is_credit was only ever carried, never used for logic (silver dropped it at [#112]). Removed from d365/erpnext/canonical staging, bronze, the schema test that selected it, and a macro docstring.
• Stale comment — rewrote the erpnext abs()+flag wording to the sign-based contract.
• Escalate balance test assert_silver_gl_debit_credit_balance warn→error. Verified PASS on live data. ⚠️ Risk: an unbalanced voucher in any future/different load will now fail dbt build — intended, but flagging.

[#119] — code (orchestrator scope/period filters)

• Cash-flow/YTD scope — cash flow gets full period_filter()+scope_filter(); YTD gets scope_filter() + period_filter(include_period=false) (single-period slicing suppressed — it would corrupt the cumulative window; year filter still applied). Documented.
• LIKE escaping — ClickHouse has no ESCAPE clause (syntax error); backslash is the built-in LIKE escape. Double it in the literal so CH sees one. Verified live: GROUP\_CORP/% matches GROUP_CORP/DEMF + grandchildren but not GROUPXCORP.
• Period validation — non-integer fiscal_year/period now raises a compiler error; no-var no-op unchanged.
• Macro tests — new tests/integration/test_orchestrator_filters.py (6 cases: no-op, GROUP expansion+escaping, full/year-only period, bad-value error, live LIKE proof).
• Seed-path depth — confirmed the slash-bounded path like patterns already match any depth; the seed fallback is intrinsically flat (no grandchildren possible), so no gap — documented.

[#120] — data (via the generator; demo-only)

• Drop dead acq rows — removed the 12 unused 2020 exchange_rates rows. Kept 12 discarded uid() draws so downstream UUIDs don't resequence → diff is exactly the intended rows.
• AMG IAS 21 — added 6 historical_equity_rates rows (AMHQ/AMUS/AMDE × 3010/3100), keyed to consolidation_group AMG (matches the seed). Judgment call: AMG reports in CHF (not USD), so stored functional→CHF rates (AMUS 0.855, AMDE 0.935, AMHQ 1.0), sourced from the earliest 2024 CHF series as a 2020 proxy (no separate 2020 FX series in the demo). ⚠️ AMG consolidated output is unverified at runtime (live CH has real-D365 data with no AMG entities).

Validation

dbt parse exit 0; affected models dbt compile exit 0; is_credit absent from compiled SQL; 6 new macro tests pass; balance test PASS on live data; demo-data.sql diff = exactly 13/13 intended lines.

🤖 Generated with Claude Code

[Anonymous]

Originally posted by: grynn-in

Review [#2] (data/SQL-semantics focus) — SAFE TO MERGE

Reviewed from scratch in an isolated worktree off origin/fix/review-followups-118-119-120. No blocking issues. The escaping, period semantics, AMG FX, and is_credit removal all check out, and I independently re-verified the generator determinism claim.

Verification done (commands + results)

• #120 determinism (the key claim): python3 scripts/generate_demo_data.py then git diff --stat clickhouse/demo-data.sql → empty (zero drift). The committed SQL is byte-for-byte what the generator produces; diff is exactly 13 ins / 13 del = the 6-row AMG HER block + collapse of the 12 dead 2020 acq rows to one kept row. The "keep 12 discarded uid() draws" trick works — no downstream UUID resequencing. ✓
• #120b AMG semantics:
• consolidation_group 'AMG' matches seeds/consolidation_groups.csv exactly (no silent no-join like the [#104] bug). ✓
• FX direction correct: FX_CHF_USD/FX_CHF_EUR are documented "CHF per 1 foreign unit" (functional→CHF, the AMG reporting currency). Stored rates AMUS 0.855 / AMDE 0.935 / AMHQ 1.0 are functional→reporting, matching rate_lookup's from=accounting→to=reporting direction. ✓
• Equity accounts: 3010/3100 are the only two Equity-typed accounts in the COA; gold_consolidated_trial_balance joins HER on (consolidation_group, data_area_id, main_account) with an ASOF period_date >= rate_date, and is_equity keys off account_type_name in ('Equity','Stockholders equity'). Full coverage. AMHQ (CHF→CHF) is forced to 1.0 by the same-currency guard regardless of the stored 1.0. ✓
• #119b LIKE escaping: compiled literal = 'GROUP\\_CORP/%'. Live CH test of the exact pattern: GROUP_CORP/DEMF→1, GROUP_CORP/.../DEMF(grandchild)→1, GROUPXCORP/DEMF→0, GROUP_CORPX/DEMF→0. So \\_ → CH parses to \_ → LIKE literal underscore. Exactly one literal underscore, no over/under-escape. Single-quote escaping from [#115] preserved (replace("'", "''"), equality uses un-escaped s). ✓
• #119 period_filter: full → and fiscal_year=2024 and fiscal_period=6; include_period=false → year predicate only; fiscal_year: 2024Q1 → compiler error ("must be a non-negative integer"); no-var → empty. ✓
• #119a YTD correctness: period_filter(include_period=false) + scope_filter() applied in the WHERE before the window. Window partitions by (data_area_id, fiscal_year, main_account, dims), so the year filter keeps every period within the kept year (cumulative window intact) and the scope filter never touches another entity's partition. Single-period slicing correctly suppressed. Cash-flow applies full both filters pre-aggregation; whole periods/entities kept, so the "ties to zero" invariant holds. ✓
• #118 is_credit removal: grepped both repos (excl. target/). Gone from canonical/bronze/staging models, the schema test, and macro docstring. Remaining refs are correct: silver_gl_entries.sql comment (documentation of the D365 IsCredit source field) and generate_demo_data.py producing the raw IsCredit source-field value. konsol app: none. Canonical schema test not weakened — is_credit is simply no longer part of the contract it asserts. ✓
• #118 balance test warn→error: live run epm_silver.silver_gl_entries → 0 failing (data_area, year) groups / 455,812 rows. Escalation is safe on current data. ✓
• dbt parse --no-partial-parse exit 0; affected macros compile; new test_orchestrator_filters.py assertions are sound and ch fixture lives in tests/integration/conftest.py.

Non-blocking

• models/silver/silver_gl_entries.sql:41-43 comment still says "is_credit is ~always 0…". Now that the canonical is_credit column is removed this wording is slightly stale, though it still accurately describes the D365 IsCredit source-field rationale. Optional cleanup.
• AMG acquisition rate uses the 2024-Jan CHF series (FX_CHF_*[0]) as a 2020 proxy — documented judgment call. AMG consolidated output is not runtime-verified (live CH carries real-D365 data with no AMG entities); only the data layer was validated.
• assert_silver_gl_debit_credit_balance.sql header has a minor grammar slip ("Since … so …").

Merge recommendation: SAFE TO MERGE

[Anonymous]

Originally posted by: grynn-in

Adversarial review — PR [#121] (fix/review-followups-118-119-120)

Reviewed in an isolated worktree against origin/main. Verified empirically against live ClickHouse, not just by reading the diff. Verdict: SAFE TO MERGE — no blocking issues.

Verification performed (commands + results)

• dbt parse --no-partial-parse → exit 0 (only pre-existing, unrelated MissingArgumentsPropertyInGenericTestDeprecation warnings).
• pytest tests/integration/test_orchestrator_filters.py -q → 6 passed (live CH).
• dbt test -s assert_silver_gl_debit_credit_balance → PASS on live data.
• Compiled gold_cash_flow_indirect / gold_ytd_trial_balance / GL staging chain → all exit 0.

#118a — retire is_credit: Removed from bronze, canonical stg_gl_entries, both adapters (stg_d365_fo__, stg_erpnext__), the schema test, and the dimension_helpers doc example. Grep across dbt_project/ and the konsol app (docker/frappe/konsol/) finds no live reference — only -- comments in silver_gl_entries.sql (lines 38/40) explaining why the sign is the source of truth. silver_gl_entries derives debit/credit from sign(accounting_currency_amount), never the dropped column; whole GL chain compiles clean.

#118c — balance test warn→error: config(severity='error') is syntactically correct and the test PASSES on live data. Grain is per (data_area_id, fiscal_year) with having abs(Σdebit−Σcredit) > 0.01. Non-blocking risk: this now gates the governed gold build, so any future external-ERP imbalance >0.01 for any entity-year hard-fails the build. That is the intended governance behavior and currently passes — flagging it as a known consequence, not a defect.

#119a — cash-flow / YTD filters: Invoked with the same pattern as gold_consolidated_trial_balance. Cash flow applies both period_filter() + scope_filter() (per-period, safe). YTD uses period_filter(include_period=false) — compiled output with {fiscal_year: 2024, fiscal_period: 3} correctly yields only and fiscal_year = 2024 (period predicate suppressed, so the running sum keeps every prior period). The new include_period arg exists and works.

#119b — LIKE escaping (the subtle one): Confirmed correct. The Jinja replace('\\','\\\\\\\\') | replace('%','\\\\%') | replace('_','\\\\_') chain compiles GROUP_CORP → SQL literal 'GROUP\\_CORP/%' (two backslashes). Verified against live ClickHouse:

• 'GROUP_CORP/DEMF' LIKE 'GROUP\\_CORP/%' → 1 (literal _ matches)
• 'GROUPXCORP/DEMF' LIKE 'GROUP\\_CORP/%' → 0 (no over-selection)
• 'GROUPXCORP/DEMF' LIKE 'GROUP_CORP/%' → 1 (confirms the bug the escape fixes)

ClickHouse string parser collapses \\→\, LIKE then reads \_ as literal — chain is right. The claim that ClickHouse has no ESCAPE clause is accurate; none is emitted. Equality comparisons correctly use the un-escaped s.

#119c — period validation: _require_period_int raises a compiler error on non-integers (verified: {fiscal_year: "20x4"} → Compilation Error ... must be a non-negative integer, got '20x4'). No-var path renders to a clean no-op (where 1 = 1, nothing appended).

#120a — dead acq FX rows: The 12 2020-01-01 rows (4 ccy × 3 types) are removed from epm_raw.exchange_rates; the generator still draws 12 uid() calls (discarded) so the deterministic RNG stream is preserved. Re-ran generate_demo_data.py → committed demo-data.sql matches byte-for-byte (only intended rows changed).

#120b — AMG IAS 21 equity rates: Cross-checked against seeds/consolidation_groups.csv:

• consolidation_group='AMG' matches the seed (the join key in gold_consolidated_trial_balance is eo.consolidation_group = hr.consolidation_group + entity + account + asof rate_date) — no silent fallback.
• AMG entities AMHQ/AMUS/AMDE report in CHF per the seed; functional currencies CHF/USD/EUR. Stored rates 1.0 / 0.855 / 0.935 are functional→CHF (CHF per 1 functional unit), matching FX_CHF_USD[0]/FX_CHF_EUR[0].
• Direction is right: model does local_amount * translation_rate (USD × 0.855 = CHF). historical_rate is read RAW (toFloat64, no /100), so the per-1 rate is stored directly. AMHQ's 1.0 is redundant (same-currency guard forces 1.0) but harmless.

Non-blocking nits

1. Compiled scope SQL has ...'GROUP_CORP'or path like... (missing space before or, from the -#} trim). ClickHouse parses it fine (verified the subquery runs and returns the 4 descendants) — purely cosmetic.
2. silver_gl_entries.sql lines 38–46 still narrate the is_credit rationale; accurate but now describes a removed column — could be trimmed for clarity (same dead-comment class [#118] targeted in the erpnext model).

Merge recommendation: SAFE TO MERGE.

[Anonymous]

Ticket changed by: grynn-in

• status: open --> closed