Fix LDF datamart SPs: guard unguarded SUBSTRING idiom across 6 per-condition SPs#840
Open
nullflux wants to merge 2 commits into
Open
Fix LDF datamart SPs: guard unguarded SUBSTRING idiom across 6 per-condition SPs#840nullflux wants to merge 2 commits into
nullflux wants to merge 2 commits into
Conversation
… SPs
The "UPDATE LDF_<CONDITION> when there is no record in the
LDF_DIMENSIONAL_DATA" block in each per-condition LDF datamart SP built
@dynamiccolumnUpdate by accumulating column names from
INFORMATION_SCHEMA.COLUMNS, then unconditionally stripped a trailing
comma with SUBSTRING(@dynamiccolumnUpdate, 1, LEN(@dynamiccolumnUpdate) - 1).
When the table has only the 7 baseline-key columns (the state on a clean
liquibase-applied DB before dynamic LDF answer columns are added),
@dynamiccolumnUpdate stays as '' and SUBSTRING('', 1, -1) raises Msg 537,
which the outer TRY/CATCH swallows into a job_flow_log ERROR row.
Three peer sites in the same files (285:530, 295:527, 320:523) already
wrap an identical SUBSTRING+EXEC with
IF @<var> IS NOT NULL AND @<var>!='' BEGIN ... END. This applies the same
guard pattern to the six previously-unguarded sites: 285:603, 290:893,
295:627, 300:833, 305:1105, 320:594.
Asserts sp_ldf_tetanus_datamart_postprocessing produces no Msg 537
("Invalid length parameter passed to LEFT or SUBSTRING") row in
job_flow_log when invoked against an LDF_TETANUS table that has only
its 7 baseline-key columns (the freshly-applied liquibase state).
Tetanus is the representative case for the 6-instance bug family --
the same SUBSTRING guard fix is applied to the 5 peer per-condition
LDF datamart SPs.
ericbuckley
approved these changes
May 20, 2026
| -- Tetanus is the representative case (the bug was originally surfaced | ||
| -- there); the same fix is applied to the 5 peer per-condition LDF SPs. | ||
|
|
||
| EXEC dbo.sp_ldf_tetanus_datamart_postprocessing |
Contributor
There was a problem hiding this comment.
Is it worth making 6 different calls here, 1 for each stored proc we're updating?
nullflux
added a commit
that referenced
this pull request
May 20, 2026
…PR routines in orchestrator Two changes: 1. Extend bug #5b's patient_id-NULL fix to the 10 multi-condition nrt_investigation variants (22000010-22000100) and the Tetanus variant (22000200). The original cherry-pick (bb7cec9) only set patient_id on the foundation Inv (20000100); the Tier 3 variants left it NULL, so the same DELETE-by-sentinel-PATIENT_UID cascade that blocks HEPATITIS_DATAMART would block any condition-datamart SP whose query path goes through F_PAGE_CASE -> D_PATIENT lookup. 2. New orchestrator step 2.5 apply_pending_pr_routines pulls each pending-PR routine (PRs #837, #839, #840) from its bug branch via `git show` and applies it to the freshly-reset DB before fixtures. Without this the orchestrator aborts at step 8 on ldf_answers_tetanus.sql (bug #7 early-RETURN guard fires on a SUB data_type ldf_uid, and bug #8 SUBSTRING fires on the tetanus datamart SP). Remove a branch from the list once its PR merges and the baseline image is refreshed. End-to-end coverage uplift vs. coverage_tier_3.md baseline: HEPATITIS_DATAMART 0 -> 1 (5b) COVID_CASE_DATAMART 0 -> 1 (5b on 22000070) BMIRD_STREP_PNEUMO_DATAMART 0 -> 1 (5b on 22000100) F_PAGE_CASE 1 -> 6 (multi-condition + 5b) LDF_DIMENSIONAL_DATA 0 -> 5 (PR #839 bug-7) MORB_RPT_USER_COMMENT 0 -> 1 (PR #837 bug-3) Still 0 (separate per-condition blockers, not 5b-related): HEPATITIS_CASE, HEP100, F_STD_PAGE_CASE, STD_HIV_DATAMART, TB_DATAMART, VAR_DATAMART, LDF_TETANUS, LDF_HEPATITIS — see coverage_hep_datamart_investigation.md for per-condition diagnoses. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Six per-condition LDF datamart SPs (
bmird,foodborne,mumps,tetanus,vaccine_prevent_diseases,hepatitis) build a comma-separated@dynamiccolumnUpdatefromINFORMATION_SCHEMA.COLUMNS, then unconditionally strip the trailing comma withSUBSTRING(@dynamiccolumnUpdate, 1, LEN(@dynamiccolumnUpdate) - 1). When the per-conditionLDF_<COND>table has only its baseline-key columns (no dynamic LDF answer columns added yet), the SELECT matches zero rows,@dynamiccolumnUpdatestays'', andSUBSTRING('', 1, -1)raises Msg 537. The outer TRY/CATCH swallows it into ajob_flow_logERROR row.Three peer sites in the same files already wrap the SUBSTRING + EXEC in
IF @dynamiccolumnUpdate IS NOT NULL AND @dynamiccolumnUpdate != '' BEGIN ... END. This applies that same guard to the six unguarded sites:285-sp_ldf_bmird_datamart_postprocessing-001.sql:603290-sp_ldf_foodborne_datamart_postprocessing-001.sql:893295-sp_ldf_mumps_datamart_postprocessing-001.sql:627300-sp_ldf_tetanus_datamart_postprocessing-001.sql:833305-sp_ldf_vaccine_prevent_diseases_datamart_postprocessing-001.sql:1105320-sp_ldf_hepatitis_datamart_postprocessing-001.sql:594Verified locally: none of the six SPs emit a Msg 537 ERROR row in
job_flow_logwhen executed against an empty per-condition LDF table.Related Issue
APP-471
Additional Notes
Independent of bug #7 — fires on a clean liquibase-applied DB the first time any per-condition LDF SP runs, before its dynamic columns are ever added.
Site `290` (foodborne) was already guarded with a different form (`IF LEN(@dynamiccolumnUpdate) > 0`); rewritten to match the `IS NOT NULL AND != ''` form used at the five peer sites.
Includes a `testData/unit` fixture that EXECs the tetanus SP against an empty `LDF_TETANUS` and asserts no Msg 537 ERROR row in `job_flow_log`.
Checklist