Skip to content

RdmpCohortBuildBreakdownByGroups plugin package (9.2.3)#2368

Draft
mtinti wants to merge 13 commits into
HicServices:developfrom
mtinti:release/RdmpCohortBuildHealthBoardBreakdown
Draft

RdmpCohortBuildBreakdownByGroups plugin package (9.2.3)#2368
mtinti wants to merge 13 commits into
HicServices:developfrom
mtinti:release/RdmpCohortBuildHealthBoardBreakdown

Conversation

@mtinti

@mtinti mtinti commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Proposed Change

Adds a self-contained package for the RdmpCohortBuildBreakdownByGroups plugin (built against the released RDMP 9.2.3), under the top-level RdmpCohortBuildBreakdownByGroups/ folder: the built .rdmp, its source, and install/usage notes. The full source is included under src/ and builds standalone from this checkout (see below).

What the plugin does. Adds one command, ExportCohortBuildBreakDownByGroups, which reproduces the Cohort Builder's count tree (each set's and container's FinalCount plus the cumulative running totals as UNION/INTERSECT/EXCEPT are applied) split by an arbitrary group column, for example Scottish health board. Output is a wide CSV: one row per count point, a Total column (RDMP's own unfiltered number), one column per group recognised by a user-supplied lookup table, then Other (present codes the lookup does not recognise) and NotKnown (not in the reference table, or NULL group), with two percentage rows at the bottom (% of final cohort and % of reference population). Groups plus Other plus NotKnown reconcile to Total on every row.

How it works. The cohort is built once through RDMP's own CohortCompiler, which populates the query cache and provides the baseline counts; the lookup table is read once up front. Every count point is then recomposed from the cached per-set identifier tables (the cohort-set source queries are never re-run) and split by the group column with one GROUP BY per node, joining the reference table on the cache server. This is why the reference table must be on the same server as the query cache (validated with RDMP's single-server check; on PostgreSQL it must also be the same database, since one connection cannot cross databases).

Inputs. Four ColumnInfo objects, mappable from the CLI (ColumnInfo:1234); the tables are derived from the columns:

  • group-by column (its table is the reference table; the patient identifier is that table's single IsExtractionIdentifier column, and a transformed identifier expression is refused)
  • lookup key column (its table is the lookup table), lookup label column, and an optional lookup grouping column used to order the output

Default for the original use case. The engine itself has no hardcoded defaults, but the plugin ships a ready-made default for its original use case: breaking a cohort down by Scottish health board on SHARE. SharePreset.cs resolves the SHARE demography catalogue and its health-board lookup (SHARE_Demography.Region and z_hb_lookup.Region/HB_Name) by name at runtime, giving a one-click GUI menu entry that works out of the box on a SHARE deployment; a second menu entry prompts for the inputs, for any other grouping. On other deployments the preset simply finds nothing and the command prompts as normal.

Requirements and preconditions (also in INSTALL.md): the cohort identification configuration must have a query caching server; the reference table must be on the same database server as the cache (same database on PostgreSQL); each identifier must map to at most one group (a single-valued identifier-to-group mapping, e.g. patient to health board). SQL identifiers, fully-qualified names and set-operation keywords are produced via the RDMP/FAnsi dialect helpers; execution is currently tested on SQL Server only.

Validation. The source builds standalone from this checkout via a pinned HIC.RDMP.Plugin 9.2.3 package reference (dotnet build RdmpCohortBuildBreakdownByGroups/src/RdmpCohortBuildBreakdownByGroups.csproj). NUnit unit and DB-integration tests live with the canonical source (feature branch): a deterministic synthetic fixture (top EXCEPT over an inclusion INTERSECT minus four exclusion sets, driven by a synthetic z_hb_lookup table) asserts the key national and per-group counts and cumulatives explicitly, asserts the unfiltered column equals RDMP's own CohortCompiler counts, and asserts a reconciliation invariant (groups + Other + NotKnown == Total) on every row. The packaged .rdmp is built from a clean committed source revision.

Type of change

What types of changes does your code introduce? Tick all that apply.

  • Bugfix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation-Only Update
  • Other (if none of the other choices apply)

Checklist

By opening this PR, I confirm that I have:

  • Ensured that the PR branch is in sync with the target branch (i.e. it is automatically merge-able). Additive: one new folder.
  • Created or updated any tests if relevant. NUnit unit and DB-integration tests exist with the canonical source; validated against a deterministic synthetic fixture (SQL Server).
  • Have validated this change against the Test Plan. Validated against the synthetic fixture instead.
  • Requested a review by one of the repository maintainers. Draft; will re-request once the plugin's home (this repo, a HIC-owned plugin repo, or standalone) is agreed, per the review discussion.
  • Have written new documentation or updated existing documentation to detail any new or updated functionality and how to use it (README.md, INSTALL.md).
  • Have added an entry into the changelog. Not applicable for this plugin-package PR.

Self-contained package for the cohort-build health-board breakdown plugin:
the built .rdmp, install/usage notes, plugin source, and design + technical
documentation. Splits the Cohort Builder's per-step count tree by Scottish
health board (wide CSV: Metric, Total, per-board columns, Other, NotKnown, and
a % of final cohort row). Cache-only recompose, validated against a
deterministic synthetic fixture.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0169JCnaL3fhhZjseDx2XXT2
mtinti and others added 2 commits July 1, 2026 15:41
Keep the package to the built .rdmp + INSTALL + README + source. Design and
technical docs remain on the proposals branch.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0169JCnaL3fhhZjseDx2XXT2
@mtinti mtinti marked this pull request as ready for review July 1, 2026 20:39

@JFriel JFriel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change would be a better tagret for a plugin, rather than core RDMP. Can discuss HIC owned plugins via teams


namespace RdmpCohortBuildHealthBoardBreakdown;

public class CohortBuildHealthBoardBreakdownPluginUserInterface : PluginUserInterface

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PluginUserInterface is really for use by plugins, not code that has access to the RDMP internals. If this is something you want to keep as a plugin, it may be best to create it as a standalone outside of the RDMP codebase, or target the chodechange against the HIC specific Plugin Codebase (can share seperatly). If you want to keep in in core RDMP, this functionality is best moved to the ContetxStripMenu or the standard RightClick options function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping it as a plugin, so PluginUserInterface stays as the mechanism.

@@ -0,0 +1,24 @@
// Surfaces the cohort build health board breakdown in the RDMP desktop GUI (right-click a cohort

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should be in the existing directory structure. Something like RDMP.Core/CohortCreation/Aggregates or similar

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this stays a plugin, the file lives in the plugin's own src folder rather than the RDMP.Core tree

public const string PercentMetric = "% of final cohort";

/// <summary>One count point of the build tree with its per-region counts (known boards only).</summary>
public sealed class NodeBreakdown

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put these nested classes into their own file

@mtinti mtinti Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, the node and buckets classes are now top-level in CohortBuildBreakdownModels.cs, with constructors.

CohortIdentificationConfiguration cic,
[DemandsInitialization("CSV file to write. Defaults to <cic>-build-healthboard.csv in the current directory")]
FileInfo toFile = null,
[DemandsInitialization("Demography catalogue holding the region column", DefaultValue = "SHARE_Demography")]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cant use this as a default value in the RDMP codebase. Probably shouldn't have a default value

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The hardcoded string defaults are removed and the engine itself has no defaults at all: the inputs are RDMP objects and the command prompts for anything not supplied. I did decide to keep a default in one place, the plugin's SharePreset.cs, which resolves the SHARE demography and health board lookup by name at runtime. The reasoning is that breaking a cohort down by health board on SHARE will likely be the most used (possibly the only) case for this plugin, and without a preset every run would mean searching for the catalogue and columns in the object picker, which gets tedious. On any other deployment the preset finds nothing and the command simply prompts as normal.

private readonly Dictionary<int, (string fqn, string col)> _setCacheTable = new();
private readonly Dictionary<(bool isContainer, int id), (int final, int? cumulative)> _baseline = new();

public ExecuteCommandExportCohortBuildHealthBoardBreakdown(IBasicActivateItems activator,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth making this more geenric i.e. ExportCohortBuildBreakDownByGroups or similar

@mtinti mtinti Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done as suggested. The command is now ExportCohortBuildBreakDownByGroups, and the plugin RdmpCohortBuildBreakdownByGroups. The groups come from any column, labelled by a user-supplied lookup table, and the SHARE configuration ships only as a named preset in the plugin.

CohortIdentificationConfiguration cic,
[DemandsInitialization("CSV file to write. Defaults to <cic>-build-healthboard.csv in the current directory")]
FileInfo toFile = null,
[DemandsInitialization("Demography catalogue holding the region column", DefaultValue = "SHARE_Demography")]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should pass in a catalogue, not a string. It can also be mapped from the cli like Catalogue:1234

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The inputs are now RDMP objects mappable from the CLI. In the final design they are four ColumnInfo arguments (the group by column plus the lookup key, label and optional grouping columns); the catalogue and tables are derived from the columns.

[DemandsInitialization("Demography catalogue holding the region column", DefaultValue = "SHARE_Demography")]
string demographyCatalogue = "SHARE_Demography",
[DemandsInitialization("Region (health board cipher) column on the demography catalogue", DefaultValue = "Region")]
string regionColumn = "Region",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should pass in a ColumnInfo

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. All four inputs are ColumnInfo objects (ColumnInfo:1234 on the CLI).

if (parent != null && indexInParent > 0 && cCum.HasValue)
cCumByRegion = RunRegionCounts(CumulativeSql(parent, indexInParent));

nodes.Add(new CohortBuildHealthBoardBreakdownReport.NodeBreakdown

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add a constructor to nodebreakdown that maps the key/values rather than have to manually map them here and further down

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. CohortBuildBreakdownNode has a constructor and the command uses it at both call sites.


private IReadOnlyDictionary<string, int> RunRegionCounts(string idListSql)
{
var sql =

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RMDP supports multible db types (sql,postgres,oracle). This SQL will need to be rewritten using FansiSQL

@mtinti mtinti Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Identifiers and fully qualified names now go through the query syntax helper (IQuerySyntaxHelper.EnsureWrapped / GetFullyQualifiedName, tables via the discovery API), results are read by column position rather than by name, and the set operations are rendered per DatabaseType (Oracle EXCEPT becomes MINUS), mirroring CohortQueryBuilderResult.GetSetOperationSql. Server co-location is validated with DataAccessPointCollection, and on PostgreSQL the reference table must additionally be in the same database as the query cache, since one PostgreSQL connection cannot access another database. The qualification: execution is currently tested on SQL Server only; the other DBMS use the dialect helpers but are not yet exercised by the automated tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update on execution coverage, after standing up PostgreSQL and Oracle containers to test this properly.

PostgreSQL: the plugin's end-to-end fixture (cohort build, query cache creation and population, cache recompose, GROUP BY join, all count/cumulative/reconciliation assertions) now runs and passes on PostgreSQL 16 as well as SQL Server. One deployment note fell out of it: on PostgreSQL everything, including the query cache, must live in a single database, since one PostgreSQL connection cannot access another database; the command validates this and the test fixture creates the cache schema in the same database as the data.

Oracle: blocked upstream, independently of this plugin. RDMP's own test QueryCachingCrossServerTests.Create_QueryCache(Oracle) fails against a live Oracle 26ai Free container with "Table name 'CachedAggregateConfigurationRe' is too long for the DBMS (Oracle supports maximum length of 30)". The cache bookkeeping table CachedAggregateConfigurationResults is 35 characters, and FAnsi's OracleQuerySyntaxHelper caps identifiers at 30 (the code comments note it "can be longer, but Oracle RAC limits to 30"). The cap is client side: creating that exact 35 character table directly in the same Oracle container via sqlplus succeeds, since Oracle 12.2 and later allow 128 character identifiers. So query caching, and anything that depends on it, cannot currently work on Oracle at all; happy to raise this on the FAnsiSql repository if useful.

public const string UnknownNode = "Unknown";

// keyed by the single-letter Region cipher held in SHARE_Demography.Region
private static readonly Dictionary<string, HealthBoard> ByRegion = new(System.StringComparer.OrdinalIgnoreCase)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be in RMDP and if something like this is required, it should reference a user-defined lookup

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The hardcoded lookup is deleted. The mapping is loaded at runtime from a user defined lookup table via the new GroupLookup class, with the key, label and optional grouping columns supplied by the caller, so nothing about the table's shape is hardcoded either. The lookup content is validated on load: one row per code, unique labels, and labels may not collide with the report's fixed column headers, since the labels become the CSV columns. For example a group labelled "Total" or "Other" would be indistinguishable from the report's own Total and Other columns, so it is rejected with a clear error.

mtinti and others added 2 commits July 9, 2026 07:01
Refresh the built .rdmp and the src snapshot for the new "% of demography"
reference row (each board's share of the whole demography population, shown
under "% of final cohort" for a cohort-vs-population comparison). README updated.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0169JCnaL3fhhZjseDx2XXT2
Refresh the package for the reworked plugin: region -> name/node mapping now
comes from a user-supplied lookup table (RegionLookup) instead of a hardcoded
list; command takes ICatalogue / ColumnInfo / TableInfo (mappable from the CLI,
no string defaults, prompted in the GUI); SQL quoting via the query-syntax
helper. Adds RegionLookup + CohortBuildBreakdownModels sources, drops
HealthBoardLookup, rebuilt .rdmp, README/INSTALL updated for the new inputs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0169JCnaL3fhhZjseDx2XXT2
@mtinti mtinti marked this pull request as draft July 10, 2026 09:03
mtinti and others added 2 commits July 10, 2026 10:22
…HARE preset)

Address the review's "make it more generic" comment fully: the command is now
ExportCohortBuildBreakDownByGroups taking four ColumnInfo inputs (group-by
column + lookup key/label/optional grouping; the reference and lookup tables
are derived from the columns and the patient identifier is the reference
table's single IsExtractionIdentifier column). GroupLookup loads caller-named
columns (nothing hard-coded in the engine); the SHARE names live only in
SharePreset.cs, which the GUI uses for a one-click "(SHARE preset)" menu entry
alongside "(choose inputs)". Package folder, plugin id, .rdmp, README and
INSTALL renamed to match; "% of demography" row renamed "% of reference
population".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0169JCnaL3fhhZjseDx2XXT2
@mtinti mtinti changed the title RdmpCohortBuildHealthBoardBreakdown plugin package (9.2.3) RdmpCohortBuildBreakdownByGroups plugin package (9.2.3) Jul 14, 2026
mtinti and others added 5 commits July 15, 2026 09:28
…ds, doc accuracy

- csproj now uses a pinned HIC.RDMP.Plugin 9.2.3 PackageReference (with a CPM
  opt-out) so the checked-in source builds from the PR checkout, addressing the
  build-path finding; types moved into the plugin namespace.
- Join on the physical identifier column and refuse transformed identifier
  expressions; GroupLookup hard-stops duplicate keys/labels and reserved labels;
  group counts accumulate under case-sensitive collation; enabled-but-empty
  containers are skipped; DBMS type checked alongside server name; reference
  table fully qualified via discovery.
- Ctor reordered (file before optional grouping) for CLI ergonomics; docs state
  the one-to-one identifier-to-group precondition and correct the two
  overstated claims (source queries never re-run; explicit assertions plus
  reconciliation invariant).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0169JCnaL3fhhZjseDx2XXT2
…ion, doc accuracy

- IsTransformedIdentifier is a whitelist (syntax-helper runtime name must equal
  the physical column; catches chi - 1 etc); server co-location validated via
  RDMP's DataAccessPointCollection (server + DBMS + credentials); container
  skipping reuses CohortQueryBuilderResult.IsEnabled with an empty-compose
  guard; duplicate-key lookup rejection now DB-tested; enabled-but-empty
  container covered under non-strict validation.
- Docs corrected: CLI example matches the argument order (file before optional
  grouping), "choose inputs" no longer claims to prompt for the grouping
  column, build-from-source describes the NuGet reference, and the
  precondition is a single-valued identifier-to-group mapping (not one-to-one).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0169JCnaL3fhhZjseDx2XXT2
…uard, doc accuracy

- Transform guard splits off aliases before whitelisting ("UPPER(chi) AS chi"
  rejected, "[db]..[tbl].[chi] AS PatientId" accepted); PostgreSQL requires the
  reference table in the same database as the cache; co-location catch narrowed
  to InvalidOperationException; group keys trimmed before accumulating.
- Docs: INSTALL lists the transformed-identifier and lookup-uniqueness
  restrictions, the PostgreSQL constraint and the SQL Server-only execution
  coverage; empty-final-cohort percentage omission documented; "purely
  cache-only" wording removed from heading/nuspec/README; preset prompt claim
  corrected. Artifact rebuilt from clean committed source.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0169JCnaL3fhhZjseDx2XXT2
…ccuracy

- PostgreSQL guard normalizes both database names via the syntax helper's
  GetRuntimeName before an exact comparison (RDMP stores TableInfo.Database
  wrapped but cache databases unwrapped - the raw comparison refused valid
  same-database configurations); blank names rejected; regression-tested
  against the FAnsi PostgreSql helper (wrapped/plain, different, case, blank).
- Test fixture mutates the global strict-validation setting inside try/finally.
- Docs: lookup table read once up front; README notes percentage rows are
  omitted for an empty final cohort; "dialect-correct by construction" replaced
  with the helpers-used/SQL-Server-tested statement; "same database server"
  wording; preset/UI comments state the optional grouping is omitted, not
  prompted; group/unfiltered/reference-table terminology in xml docs.
- Artifact rebuilt from clean committed source.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0169JCnaL3fhhZjseDx2XXT2
Preset resolves only SHARE_Demography.Region and z_hb_lookup Region/HB_Name;
the optional grouping column remains a generic command input but is no longer
part of the preset. Rebuilt .rdmp from clean committed source.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0169JCnaL3fhhZjseDx2XXT2
@mtinti

mtinti commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Apologies for the confusion - this was always intended as a plugin, not a change to core RDMP, but I got confused about how best to submit it. For the moment it lives in a folder in this PR (self-contained: the built .rdmp plus its source, which builds against the released HIC.RDMP.Plugin 9.2.3 NuGet package). Happy to discuss the right home for it via Teams as suggested.

…n pg)

INSTALL/README now state execution is tested on SQL Server and PostgreSQL
(single-database requirement on PostgreSQL) and that Oracle is blocked
upstream by FAnsi's 30-character identifier cap on the cache bookkeeping
table name.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0169JCnaL3fhhZjseDx2XXT2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants