feat/demand-det
safead71b23 · incrementalPR #341reviewed 2026-07-23 04:53 UTC0H · 1M · 3L · 2I- Purpose
- Add demand-based finding detectors (D1 punta_management + D2b overnight_load_factor) fed by per-period TOU demand from bill historicData — the first detectors to use the per-period demand seam rather than the consumption series.
- Goal
- Ship two GDMTH detectors: D1 identifies buildings that can shed punta-window demand but do so inconsistently (avoidable $/yr); D2b identifies buildings with base-period utilization far above cohort (overnight waste signal).
- Sub-goals
- SG-1: consumption-series base — clean kWh series
- SG-2: repoint consumption_yoy_spike to the clean series
- SG-3: peer-benchmark detector — cohort-relative YoY
- SG-4: demand detectors D1 punta_management + D2b overnight_load_factor
- SG-5: address loop-review feedback from prior pass
- SG-6: harden per-period demand scalar subqueries against dup line items (this commit)
- What
- Added LIMIT 1 to three scalar subqueries inside fetchPerPeriodDemandForOrg's base CTE: historicData key lookup, baseKwh conceptId lookup, and ESTIMACION key lookup. Added a 3-line SQL comment explaining the invariant.
- Why
- Previous loop-review (correctness) flagged that unconstrained JSONB line_items could cause 'more than one row returned by a subquery' errors if a re-processing bug duplicates a concept — crashing the entire org's detector run. LIMIT 1 prevents crash at the cost of silently picking the first dup (safe when dups are identical, which is the re-processing scenario).
- Areas
- domains/utility/src/finding+799−318domains/utility/src/consumption-series+678−0docs/energia+183−0
- Blast
- 13 files, ~2100 adds / ~370 dels across domains/utility (finding + consumption-series) and docs. This incremental commit touches 1 file, +4 lines (3 LIMIT 1 additions + 3-line comment).
Findings · 6
correctness2
LIMIT 1 without ORDER BY is non-deterministic if duplicate entries have different values
domains/utility/src/finding/finding.queries.ts:703
jsonb_array_elements expands in array order but PostgreSQL makes no ORDER BY guarantee with LIMIT 1. For re-processing dups (identical values), this is safe. If a re-processing run also corrected an incorrect value (old + new entry both present), LIMIT 1 silently picks one non-deterministically — could return the stale value. Edge case but the same pattern applied 3× (hist, base_kwh, estimacion).
Catalog JOIN fan-out risk for base_kwh is zero
domains/utility/src/finding/finding.queries.ts:705
bill_concepts_catalog.concept_name has a UNIQUE constraint, so WHERE concept_name='baseKwh' can return at most one catalog row. The only dup risk for base_kwh is duplicated JSONB line items on the bill, not a catalog fan-out. LIMIT 1 correctly targets the actual risk.
conventions1
Reviewer-tool attribution suffix in production SQL comment
domains/utility/src/finding/finding.queries.ts:701
'(loop-review · correctness)' names the process that caught the issue, not a constraint or design rule. Future maintainers gain nothing from the attribution. The preceding two comment lines already give full WHY context. Drop the suffix. Same pattern appears in finding.decisions.ts and finding.shells.ts — not documented in CLAUDE.md conventions.
tests2
fetchPerPeriodDemandForOrg has zero integration-test coverage — dup guard is unverifiable at the SQL layer
domains/utility/src/finding/finding.queries.ts:681
The function has no test block in finding.integration.test.ts (pre-existing gap, not introduced here). fetchBillFactRowsForOrg has a dedicated 'concept extraction' section for exactly this class of stringly-typed JSONB risk; fetchPerPeriodDemandForOrg has identical exposure — JSONB key names 'historicData', 'ESTIMACION', concept_name 'baseKwh' — with no equivalent coverage. The LIMIT 1 fix is correct but unverifiable without a test that seeds a bill with duplicate line items and asserts the query returns one row rather than throwing.
fetchBillFactRowsForOrg uses jsonb_object_agg — structurally immune, no LIMIT 1 needed
domains/utility/src/finding/finding.queries.ts:529
The other jsonb_array_elements usage in finding.queries.ts uses LEFT JOIN LATERAL + jsonb_object_agg which aggregates duplicates (last-writer-wins) rather than returning multiple rows. The scalar subquery pattern in fetchPerPeriodDemandForOrg is the only vulnerable form, and this commit correctly covers all three instances.
improvement1
Three separate jsonb_array_elements unnestings where one LEFT JOIN LATERAL pass would suffice
domains/utility/src/finding/finding.queries.ts:702
The base CTE unnests b.line_items three times — once for historicData (by key), ESTIMACION (by key), baseKwh (by conceptId). The bill_facts_base CTE in the same function (lines ~529–541) already demonstrates the canonical single-LATERAL pattern with jsonb_object_agg. Refactoring to one or two laterals would eliminate 2–3× redundant JSONB array scans per bill row and align with the established file pattern. Not blocking — LIMIT 1 is safe as-is — but worth a follow-up.