diff --git a/PRPs/prp-sde-stratum-value-lazy-conversion.md b/PRPs/prp-sde-stratum-value-lazy-conversion.md new file mode 100644 index 0000000000..a55860a690 --- /dev/null +++ b/PRPs/prp-sde-stratum-value-lazy-conversion.md @@ -0,0 +1,663 @@ +# PRP: Eliminate eager FHIR reconstruction in SDE/stratifier value accumulation + +## Metadata +- **Title**: Stop reflectively rebuilding whole FHIR resources in `StratumValueWrapper` to read their id +- **Status**: Parts 1–4 implemented; Parts 1–2 measured, Parts 3–4 covered by tests only +- **Priority**: High (Performance regression + latent correctness defect) +- **Estimated Effort**: 1 day (Parts 1–2, done), 2–3 days (Part 3, done), 1–2 days (Part 4, done) +- **Target Branch**: `ld-20260901-sde-lazy-conversion`, from `main` (`3a82da9c`) +- **Reported From**: `cqis-spark` HEDIS 2025 measure evaluation, 400-member cohort + +## Problem Statement + +`SdeDef.accumulate()` converts **every** SDE value from its engine-native `ClassInstance` into a +fully materialized HAPI FHIR object graph — reflectively, field by field — and then uses only the +resource's **id** as a grouping key. The graph is discarded immediately afterward. + +On a measure with many resource-valued SDEs this dominates the entire evaluation. + +### Measured impact + +Two HEDIS 2025 certified measures were used as a controlled pair. `AAB-Details` and +`AAB-Reporting` are structurally identical — one group, four populations, one stratifier each — +and differ **only** in supplemental data: + +| | `AAB-Reporting` | `AAB-Details` | +|---|---|---| +| `supplementalData` | 0 | **38** (≈28 return FHIR resource collections; 9 return `ExplanationOfBenefit`) | +| groups / populations / stratifiers | 1 / 4 / 1 | 1 / 4 / 1 | + +Subtracting Reporting from Details on the same build, same 400-member cohort, same host isolates +the SDE cost exactly: + +| | CR 4.8.0 / CQL 4.4.0 | CR 4.12.0 / CQL 5.3.0 | +|---|---|---| +| CQL evaluation, Reporting | 122 714 ms | 203 205 ms | +| CQL evaluation, Details | 145 738 ms | 1 177 396 ms | +| **Δ (SDE accumulation)** | **23 024 ms** | **974 191 ms** | +| Report build, Reporting | 3 982 ms | 4 108 ms | +| Report build, Details | 5 874 ms | 74 585 ms | +| **Δ (SDE rendering)** | **1 892 ms** | **70 477 ms** | +| **Total SDE cost per member** | **62 ms** | **2 612 ms** | + +**42x.** Wall-clock for the run went from 24 s to 6 minutes. + +The regression also destroys parallelism. Per-member cost becomes a function of how many resources +that member's SDEs return, which varies far more between members than CQL logic does, so data skew +becomes time skew. Across 11 Spark partitions, effective parallelism measured 7.7–9.2x on +`AAB-Reporting` but 3.65x on `AAB-Details`; on CR 4.11.1 a single partition consumed 311.5 s of a +312.8 s span. + +### Root cause + +Under CQL 4.x the engine handed CR real HAPI objects. `StratumValueWrapper`'s constructor was a +plain assignment (CR 4.8.0): + +```java +public StratumValueWrapper(Object value) { + this.value = value; +} +``` + +CQL 5 changed the value model — the class's own javadoc records it: *"CQL-5 stratifier/SDE results +arrive as engine-native values: FHIR resources and complex types as `ClassInstance`."* The +constructor now converts on every construction: + +```java +public StratumValueWrapper(Object value) { + this.value = normalizeEngineNativeValue(value); // → full reflective FHIR reconstruction +} +``` + +The call chain, matching the production stack trace exactly: + +``` +SdeDef.accumulate ← 38 SDEs × 400 members × N resources each + → new StratumValueWrapper(value) + → normalizeEngineNativeValue + → ClassInstanceHelper.convertToFhirR4IfNeeded + → CqlFhirParametersConverter.toFhirValue ← recurses over every child element + → BaseRuntimeDeclaredChildDefinition.setFieldValue + → java.lang.reflect.Field.set +``` + +And the entire result is consumed by this, in `StratumValueWrapper#getKey`: + +```java +} else if (value instanceof IBaseResource resource) { + key = resource.getIdElement().toVersionless().getValue(); // "MedicationDispense/123" +} +``` + +An `ExplanationOfBenefit` — dozens of nested backbone elements — is reconstructed through +reflection so that its id can be read. The `ClassInstance` already carried that id. + +### Secondary defect: the reconstruction is also a crash source + +`CqlFhirParametersConverter#toFhirValue` derives the target HAPI class from the **CQL value's own +type name** (`modelResolver.resolveType(typeName)`) rather than from the HAPI child definition it +is about to populate, then patches the guess with heuristics. Two known failures of the same +approach: + +``` +IllegalArgumentException: Could not resolve inner FHIR type: AdjudicationComponent +``` +— nested backbone class-name guessing; addressed in `3a82da9c` by the `parentName` string +replacement at `CqlFhirParametersConverter.kt:400-412`. + +``` +IllegalArgumentException: Can not set org.hl7.fhir.r4.model.Enumeration field +org.hl7.fhir.r4.model.MedicationDispense.status to org.hl7.fhir.r4.model.CodeType +``` +— a bound-code field. `toFhirValue` produced a `CodeType`; `MedicationDispense.status` is declared +`Enumeration`. The `instance is IBaseEnumeration` branch at line 426 +handles enumerations correctly, but is never reached because `resolveType` returned `CodeType`. +This reproduces on CR 4.11.1 / CQL 5.2.0 with `AAB-Details` and fails 134 of 400 members. + +Every bound-code field on every resource type an SDE can return is a latent instance of this. +Patching them one at a time will not converge. + +## Solution Overview + +**Do not convert FHIR resources during accumulation at all.** Derive the grouping key directly from +the `ClassInstance`, and let conversion happen later — only for the values that are actually +rendered. + +Three facts make this nearly free to implement, because the pieces already exist: + +1. **`ClassInstanceHelper.getId(ClassInstance)` reads the id straight off the instance**, with no + conversion: `type.localPart` + `id.value`, returned as `"Type/id"`. + + > **Corrected during implementation.** This section claimed `getId` was *byte-identical* to + > `resource.getIdElement().toVersionless().getValue()`. It is not. Converting a `ClassInstance` + > copies `id.value` and nothing else, so the converted resource reports the **bare** id — the + > current key for `ExplanationOfBenefit/eob-flat` is `eob-flat`, not `ExplanationOfBenefit/eob-flat`. + > Keying on `getId` would have changed every rendered stratum value and every DSTU3 SDE + > observation code, and would have split one resource across two strata depending on whether it + > happened to be converted. Part 1 therefore uses a new `ClassInstanceHelper.getIdPart`, which + > returns the bare id part, and `getId` is now defined in terms of it. `getId` remains correct for + > its own callers, which build references. + +2. **`ClassInstanceHelper.isFhirResource(FhirVersionEnum, ClassInstance)` already exists** as the + predicate distinguishing resources (huge, id-keyed) from complex datatypes (small, value-keyed). + A version-agnostic overload was added alongside it for callers holding no FHIR version of their + own — see the note at the end of Part 1. + +3. **`R4MeasureReportBuilder#buildSDE` already handles the unconverted shape** — and this branch is + currently dead code, because the wrapper converts before the builder ever sees the value: + + ```java + } else if (key.getValue() instanceof ClassInstance classInstance + && isFhirResource(FhirVersionEnum.R4, classInstance)) { + var resource = (Resource) convertToFhirR4(classInstance); + bc.addCriteriaExtensionToSupplementalData(resource, sde.id(), sde.description()); + } + ``` + +**Key insight**: `accumulate()` produces a `Map` — a frequency count over +**distinct** values. Deferring conversion to the report builder collapses it from *once per +occurrence per member* to *once per distinct value in the final map*. On a population report over +400 members, most SDE resources are unique per member, so the win is not primarily dedup — it is +that population reports never need the graph at all, only the id reference. + +## Implementation Details + +### Part 1 — Defer resource conversion in `StratumValueWrapper` + +**File**: `cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/StratumValueWrapper.java` + +Convert eagerly for everything **except** FHIR resources. Complex datatypes (`Coding`, +`CodeableConcept`, `Identifier`) must still convert — `getKey()` reads their contents, and they are +small. CQL `SimpleValue` unwrapping must also stay: the existing javadoc explains that a CQL String +renders as `'male'` rather than `male`, and skipping it produces wrong stratum keys. + +```java +private static Object normalizeEngineNativeValue(Object rawValue) { + // A FHIR *resource* is keyed by its id, which ClassInstanceHelper.getId reads directly off the + // ClassInstance. Converting it to HAPI first means reflectively rebuilding the entire element + // graph — for an ExplanationOfBenefit, dozens of nested backbone elements — and discarding it. + // Leave it engine-native; the report builders convert the values they actually render. + if (rawValue instanceof ClassInstance classInstance + && ClassInstanceHelper.isFhirResource(FhirVersionEnum.R4, classInstance)) { + return rawValue; + } + + // Complex datatypes (Coding, CodeableConcept, Identifier) are small and getKey() reads their + // contents, so they still convert here. + var converted = ClassInstanceHelper.convertToFhirR4IfNeeded(rawValue); + if (converted != rawValue) { + return converted; + } + // ... existing SimpleValue unwrapping, unchanged ... +} +``` + +Then add a `ClassInstance` branch to the three renderers, ahead of the `IBaseResource` branch they +mirror: + +```java +// getKey() +} else if (value instanceof ClassInstance ci + && ClassInstanceHelper.isFhirResource(FhirVersionEnum.R4, ci)) { + key = ClassInstanceHelper.getId(ci); +} else if (value instanceof IBaseResource resource) { + key = resource.getIdElement().toVersionless().getValue(); +} +``` + +The same branch is needed in `getDescription()` and in the private `getValueAsString(Object)`, both +of which currently end at `value instanceof IBaseResource → resource.getIdElement()...`. Without +them a raw `ClassInstance` falls through to `value.toString()`, which yields a wrong key and breaks +dedup. + +`getId` returns `null` when the `ClassInstance` has no `id` element. `getKey()` already throws +`InvalidRequestException` on a null key, so fall back to the existing `value.toString()` path rather +than propagating null. + +**Note the pre-existing R4 hard-coding**: `normalizeEngineNativeValue` already calls +`convertToFhirR4IfNeeded` unconditionally from a version-agnostic `common` class. This PRP does not +fix that, but `isFhirResource` takes a `FhirVersionEnum`, so the new branch should thread the +version through rather than entrench the assumption further. Resolving it properly is separate work. + +> **As implemented.** Threading a version through was not available: `StratumValueWrapper` is +> constructed from `SdeDef.accumulate` and four sites in `MeasureMultiSubjectEvaluator`, none of +> which holds a `FhirVersionEnum`, and the question being asked does not need one. A `ClassInstance` +> names its type but not the version that type came from, and telling a resource from a complex +> datatype has the same answer in every version, so `ClassInstanceHelper` gained +> `isFhirResource(ClassInstance)`, which tests the name against the resource types of DSTU3, R4, R4B +> and R5 together. The version-qualified overload remains for questions where the version does +> matter — conversion — and is what the R4 report builders still call. The two renderer branches use +> `getIdPart`, not `getId`, per the correction in the Solution Overview. + +### Part 2 — Memoise `getKey()` + +`hashCode()` calls `getKey()`. `equals()` calls it **twice** (on both operands). `accumulate()`'s +`Collectors.groupingBy(Function.identity(), Collectors.counting())` calls `hashCode()` per element +and `equals()` on every hash collision. `getKey()` allocates a `CqlExpressionValue` and walks an +eight-branch `instanceof` chain each time. + +The wrapped value is effectively immutable after construction, so cache it: + +```java +private String cachedKey; + +public String getKey() { + if (cachedKey == null) { + cachedKey = computeKey(); + } + return cachedKey; +} +``` + +Secondary to Part 1 in magnitude, but it is a few lines and removes a repeated cost on the same hot +path. + +### Part 3 — Resolve target types from the HAPI child definition in `toFhirValue` + +**File**: `cqf-fhir-cql/src/main/java/org/opencds/cqf/fhir/cql/engine/parameters/CqlFhirParametersConverter.kt` + +Parts 1–2 make the conversion rare; they do not make it correct. The report builder still calls +`convertToFhirR4(classInstance)` for rendered values, so the `MedicationDispense.status` crash moves +rather than disappears — it just affects far fewer values. + +The underlying flaw is that `toFhirValue` guesses the HAPI class from the CQL type name and then +repairs the guess: + +```kotlin +clazz = modelResolver.resolveType(typeName) +if (!parentName.isNullOrBlank() && !clazz.isEnum && clazz.name.contains("$") && ...) { + val correctClassName = clazz.name.replace(clazz.enclosingClass.simpleName, parentName) + clazz = Class.forName(correctClassName) // heuristic repair +} +``` + +The authoritative answer is available at the recursion site. The loop already holds the `child` +definition: + +```kotlin +for (child in definition.getChildren()) { + val elementValue = (valueToConvert as ClassInstance)[child.elementName] + ... + child.mutator.addValue(instance, toFhirValue(elementValue, parentNameForChildren)) +} +``` + +**Change the recursion to pass the child definition rather than a `parentName` string**, and derive +the target class from it (`BaseRuntimeChildDefinition#getChildByName(...).getImplementingClass()`, +or `getChildElementDefinitionByDatatype` where the child is a choice). Fall back to +`modelResolver.resolveType(typeName)` only at the top-level entry point, where there is no parent +child definition. + +This subsumes both known failures: +- Nested backbone classes come from the child definition, so the `parentName` string-replacement + heuristic from `3a82da9c` can be **removed**, not merely retained. +- A bound-code child reports its implementing class as `Enumeration`, so the existing + `instance is IBaseEnumeration` branch is reached and `MedicationDispense.status` populates + correctly. + +Part 3 is independently valuable and can ship separately from Parts 1–2 in either order. + +> **As implemented**, with three findings worth carrying forward. +> +> **The crash is engine-version-dependent.** `BoundCodeSdeTest` — a measure whose SDE returns +> `MedicationDispense` — fails on the version catalog's `cql = "5.2.0"` with exactly the +> `Enumeration ... to CodeType` message above, and **passes unchanged** on the +> `5.3.0-fix-model-resolver-overrides` snapshot, which makes `resolveType` return the right class +> upstream. Part 3 was validated against 5.2.0 so the test means something at the committed pin. The +> class of defect is what Part 3 removes: the converter no longer guesses, so it does not depend on +> the guess being good. +> +> **Instantiate through HAPI, not through reflection.** Deriving the class was necessary but not +> sufficient. A bound code's implementing class is `Enumeration`, whose no-arg constructor leaves it +> without an `EnumFactory`, so `setValueAsString` still fails. The fix is +> `elementDefinition.newInstance(childDefinition.instanceConstructorArguments)` — HAPI's own parser +> path, where the constructor arguments *are* the `EnumFactory`. +> +> **Two element kinds must stay on the type-name path.** `contained` is a `CONTAINED_RESOURCE_LIST` +> whose implementing class is the `IBaseResource` interface, and `Narrative.div` is an `XhtmlNode`, +> which is not an `IBase` at all — neither can be instantiated from its child definition. The +> implementation accepts only `PRIMITIVE_DATATYPE`, `ID_DATATYPE`, `COMPOSITE_DATATYPE`, +> `RESOURCE_BLOCK` and `RESOURCE`, and falls back for the rest, which handles them no worse than +> before. Also note `getValidChildNames()` is safe to call on every child but `getChildByName` is +> not: it throws `AssertionError` on `modifierExtension`. Only the single-valid-name path calls it; +> choices go through `getChildElementDefinitionByDatatype`. + +### Part 4 — Rebuild the custom sets around id-keying + +**Files**: +- `cqf-fhir-cr/.../measure/common/HashSetForFhirResourcesAndCqlTypes.java` +- `cqf-fhir-cr/.../measure/common/HashSetForCqlExpressionValues.java` +- `cqf-fhir-cr/.../measure/common/FhirResourceAndCqlTypeUtils.java` + +Both custom sets predate the CQL 5 migration. They were written when expression results were HAPI +objects, and the CQL-5 integration did not revisit them. They are now doing something other than +what they were designed to do. + +**4a. The CQL-type detection never fires.** `FhirResourceAndCqlTypeUtils` recognizes exactly one CQL +type: + +```java +public static Value castToCqlTypeIfApplicable(Object obj) { + if (obj instanceof Date cqlDate) return cqlDate; // only Date + return null; +} +``` + +The naming downstream still reflects that origin — `areEqualCqlTypes(Value cqlDate1, Value +cqlDate2)`. CQL 5 expression results are `ClassInstance`, which is neither `IBaseResource` nor +`Date`, so in `add()` and `remove()` both casts return null and control falls through to +`super.add` / `super.remove`. Those are plain `HashSet` operations keyed on +`ClassInstance.hashCode()` — the same deep structural hash Part 1 exists to avoid. **The class is +bypassed precisely where the values now live.** + +**4b. `contains()` still fires, on a different relation.** `areObjectsEqual` tests `instanceof +Value`, and `ClassInstance` *is* a `Value`, so `contains` routes into `EqualEvaluator.equal`. The +result is two relations in one collection: + +| operation | relation | +|---|---| +| `add`, `remove` | `ClassInstance.equals` — Kotlin structural: `type` + `elements` map equality | +| `contains`, `retainAll` | `EqualEvaluator.equal` — CQL `=` semantics | + +These are not defined to agree. CQL `=` deliberately diverges from structural equality for several +types: `Decimal` compares via `compareTo` (so `1.0 = 1.00`) where map equality over `BigDecimal` is +scale-sensitive, and `Quantity`, `Interval` and uncertain `DateTime` each have their own rules. So +`contains(x)` can answer false for an `x` that `add` placed in the set. + +This is a **narrow** divergence, not a wholesale one — `structuredValueElementsEqual` skips +element pairs that are null on both sides, so two identical resources still compare equal. +Confirm it against `Decimal.equals` before treating it as a live defect. It matters because +`retainAll` drives stratum population intersection at `MeasureMultiSubjectEvaluator:802,809`, where +a false negative silently drops resources from a population. + +> **Confirmed, and the resource case is worse than described.** Measured directly: +> +> | | `equals` | `EqualEvaluator.equal` | same hash | +> |---|---|---|---| +> | `Decimal("1.0")` vs `Decimal("1.00")` | false | **true** | no | +> | two identical `ClassInstance` resources | true | true | yes | +> | same resource, differing `meta.versionId` | false | **null** (uncertain) | — | +> +> The `Decimal` divergence is real, and compounded by 4a: `castToCqlTypeIfApplicable` recognised +> only `Date`, so `Decimal` never reached the CQL relation on `add` at all. +> +> The third row is the one that matters most, and neither relation catches it: the same resource +> retrieved twice with different metadata compares unequal *structurally* **and** uncertain under +> CQL `=`, which `areEqualCqlTypes` reads as unequal. So a set of evaluated resources could hold one +> resource twice regardless of which relation ran. Only id-keying collapses it. Both rows are now +> regression tests in `HashSetForFhirResourcesAndCqlTypesTest`. + +**4c. Both sets are O(n) per operation.** `contains` is a linear scan (`containsInner` iterates the +collection) and `add` calls `contains`, so building an n-element set is **O(n²)** — and under CQL 5 +each comparison walks a resource graph, making it O(n² × graph). `HashSetForCqlExpressionValues` +documents this in its own javadoc: *"Bucket placement still uses the wrapper's default +`Object.hashCode()` … so `add` / `remove` / `contains` / `retainAll` fall through to linear-time +identity checks."* + +**The fix is the same primitive as Part 1.** Key elements on `(resourceType, logical id)` — for +`IBaseResource` and for FHIR-resource `ClassInstance` alike — falling back to identity for values +carrying no id. That yields three things at once: O(1) `add`/`contains`/`remove`, a **single** +relation across all four operations, and no deep hashing. + +Unlike the equivalent change in the CQL engine, this code can see FHIR types directly, so no +model-agnostic key extraction is needed — `ClassInstanceHelper.getId` already supplies the +`ClassInstance` half and `IBaseResource.getIdElement()` the other. + +`HashSetForCqlExpressionValues` most likely collapses into the same class once keyed; it is the same +structure plus a `CqlExpressionValue.raw()` unwrap step. Decide that during implementation rather +than committing to it here. + +> **The design premise has inverted.** The javadoc states these exist "strictly to compensate for the +> fact that FHIR resource classes and CQL types do not implement equals() and hashCode()". Under +> CQL 5 `ClassInstance` implements both — deeply. The job is no longer to *supply* an equality but to +> *suppress* the expensive one in favour of id-keying. That is why this is a rewrite rather than a +> repair, and why leaving the classes as-is is not a neutral choice: they are currently slower than a +> plain `HashSet` for `contains`, and inconsistent with it for `add`. + +Part 4 is independent of Parts 1–3 and can ship on its own. + +> **As implemented.** Elements are stored in a `LinkedHashMap` under a new package-private +> `IdentityKey`, which is what makes one relation answer every operation rather than each reaching +> for its own: +> +> | key | for | cost | +> |---|---|---| +> | `ResourceKey(String)` | a FHIR resource, HAPI or `ClassInstance`, as `Type/id` | O(1), a string compare instead of a graph walk | +> | `CqlValueKey(Value)` | any other CQL value, compared with CQL `=` | one shared bucket, so linear among themselves | +> | `PlainKey(Object)` | everything else, on its own `equals` | O(1) | +> +> `CqlValueKey` has a constant hash because CQL `=` is not hash-compatible — `1.0 = 1.00` is true +> across differing hashes, and cross-precision `DateTime` comparison is uncertain rather than false. +> There is no key that spreads these across buckets, so they keep the linear behaviour they had. This +> is not a compromise in practice: resources take the keyed path, and what is left is the `Date`s and +> `Decimal`s a stratifier returns. +> +> **`HashSetForCqlExpressionValues` did collapse** — into a `keyFor` override on the parent that +> unwraps the wrapper, 140 lines down to 57. That is the whole of the difference between them. +> +> **`retainAll` and `removeAll` are overridden**, and must be. The inherited implementations ask the +> *other* collection what it contains, and a plain `List` or `HashSet` answers by Java object +> identity — which is precisely how a population intersection drops resources. Both now key the +> other collection first, so the comparison runs in this set's relation whatever it is handed. +> +> **4a is resolved by deletion, not extension.** With keying, `castToResourceIfApplicable` and +> `castToCqlTypeIfApplicable` have no callers left; the `Date`-only detection is gone rather than +> widened to `ClassInstance`. `areObjectsEqual` and the keys now share one `resourceIdentity` +> function, so `HashMapForFhirResourcesAndCqlTypes` and the ad-hoc `areObjectsEqual` call sites move +> with them. (That map remains O(n) per operation — out of scope here, and a candidate for the same +> treatment.) +> +> **Two deliberate deviations, both behaviour changes rather than speedups:** +> +> - **Id-less resources do not fall back to identity**, as this section proposed. The existing +> `addFhirResourceWithNullIdTwiceAddsOnlyOne` pins the opposite: two id-less HAPI resources of a +> type are today one element. That is preserved (key `Type/`), while an id-less `ClassInstance` +> falls through to CQL equality, which is the relation *that* form already had. Each keeps its own +> current behaviour; changing either is a separate decision. +> - **Keying on the logical id normalises.** `Patient/1` and `Patient/1/_history/2` are now one +> element where full-`IdElement` comparison called them two, and a HAPI resource and the +> `ClassInstance` of the same resource now unify. Both follow from "key on `(resourceType, +> logical id)`" as written, both match what `StratumValueWrapper` does after Part 1, and both are +> semantic changes to state plainly rather than fold into a performance claim. +> +> **The risk to watch**: in `HashSetForCqlExpressionValues`, wrappers around *equal non-resource* +> raws now deduplicate where identity-based `add` kept both — two `ObservationAccumulator`s carrying +> identical entries, say. The measure-observation suites pass, but this is the change most likely to +> surprise if something downstream depended on multiplicity. + +## Design Decisions + +### Decision 1: Why not just cache the conversion per `ClassInstance`? + +A conversion cache keyed on identity would help only where the same instance recurs, and SDE +results are largely distinct per member. It would also retain the memory cost — holding full HAPI +graphs for every accumulated value — which is the second half of the problem, since allocation +pressure is what collapses parallelism under Spark. Not converting is strictly better than +converting once. + +### Decision 2: Why not convert lazily inside `StratumValueWrapper` (a lazy getter)? + +Tempting, but it hides an expensive call behind `getValue()`, which reads as a field accessor. The +report builders are the only consumers that need the HAPI form, they already know they need it, and +`R4MeasureReportBuilder` already has the branch. Keeping conversion at the call site that requires +it keeps the cost visible. + +### Decision 3: Why keep converting complex datatypes eagerly? + +`getKey()` and `getDescription()` read the *contents* of `Coding` / `CodeableConcept` / `Identifier` +via `IAdapterFactory`, so they need the HAPI form regardless. They are also a handful of primitive +children, not a resource graph. Making them lazy adds branching for no measurable gain. + +### Decision 4: Scope — SDEs and stratifiers together + +`StratumValueWrapper` is constructed from five sites: `SdeDef.accumulate` and four in +`MeasureMultiSubjectEvaluator` (lines 605, 633, 658, 675) that build stratum values. The fix is in +the wrapper, so both paths benefit. Stratifiers usually return codes rather than resources, so the +measured win is SDE-side, but resource-valued stratifiers exist and would regress identically. + +## Performance Analysis + +> **Corrected after Parts 1–2 were implemented and measured.** The original version of this section +> predicted that the fix would also eliminate the 2 435 ms/member `cql_eval` delta. That was wrong, +> and the error is worth recording because it was a reasoning error, not a measurement error: +> `SdeDef.accumulate` is reached from `R4MeasureProcessor.evaluateMeasure`, **not** from +> `evaluateMeasureWithCqlEngine`, so all of its cost was in report building from the outset. Parts +> 1–2 could only ever move that half, and the `cql_eval` delta is a different problem — evaluating +> the 38 SDE **CQL expressions**, which is the CQL-engine defect covered by a separate PRP in the +> engine repo. + +Measured on the `AAB-Details` 400-member benchmark: + +| | before | after Parts 1–2 | | +|---|---|---|---| +| `report_build_ms` (whole run) | 74 585 | **14 249** | **−81%** | +| SDE rendering delta, per member | 176 ms | **25 ms** | target was < 100 ms | +| `cql_eval_ms` (whole run) | 1 177 396 | 1 095 340 | −7% — not this PRP's target | +| evaluations completed | 266 of 400 (CR 4.11.1) | **400 of 400** | crash eliminated | + +Parts 1–2 met their target. Profiling afterward put `SdeDef.accumulate` at **0.6% of execution +samples**, so this path is no longer material. + +> **The crash was not removed by Parts 1–2**, though the run above shows it gone. Parts 1–2 stop +> converting during accumulation, but a report still renders SDE resources, and `buildSDE` converts +> each one — the same conversion, later and less often. What removed it from that run is the +> `5.3.0-fix-model-resolver-overrides` snapshot the benchmark built against, which resolves the type +> upstream. Part 3 is what removes it at this layer, on any engine version. Worth keeping straight: +> the 400-of-400 result is evidence for the *snapshot*, not for Parts 1–2. + +Remaining wall-clock is dominated by two things outside Parts 1–3, both since diagnosed by JFR +profiling of the same run: + +- **83.6% of CPU** sits in a deep recursive `hashCode()` over CQL values, driven by the engine's + evaluated-resources set (`State.carryOverEvaluatedResourcesUpCallStack` 42.3%, + `ExpressionDefEvaluator.internalEvaluate` 40.4%). That is a CQL-engine fix. +- Part 4's custom sets contribute the clinical-reasoning share of the same pattern. + +**Part 4 expected effect**: `add`/`contains`/`remove` go from O(n) — with each comparison walking a +resource graph — to O(1). Set construction goes from O(n² × graph) to O(n). No standalone benchmark +figure is offered here because the cost is currently masked by the engine-side hashing; measure it +after the engine fix lands, or in isolation with a microbenchmark over a synthetic population set. + +Memory: reduced transient allocation during accumulation. Note that GC was measured at only +6 630 ms of pause across a 160 s recording, so allocation pressure is **not** what limits parallelism +under Spark — an earlier hypothesis in this document's history that the profile disproved. + +## Testing Strategy + +### Correctness +- **`NestedBackboneSdeTest` (existing, from `3a82da9c`) must stay green.** It is the regression + guard for the conversion path and covers depth-1 and depth-2 backbone elements. *Its helper needed + one change: the accumulated value is now a `ClassInstance`, so the test converts it the way the + report builders do. The conversion path it guards is unchanged.* +- **New**: an SDE returning `MedicationDispense` resources, asserting the bound-code `status` field + round-trips. This is red before Part 3 and green after — on CQL 5.2.0. See the Part 3 note: it is + green either way on the 5.3.0 snapshot. +- **New**: assert `StratumValueWrapper.getKey()` returns the identical string for a given resource + whether constructed from a `ClassInstance` or from a HAPI `IBaseResource`. This is the invariant + Part 1 depends on and the one that would silently corrupt SDE grouping if the key format ever + drifted between the two forms. *This is what caught the `getId`/`getIdPart` error in the Solution + Overview: written against `getId` it fails, and the failure is the real one.* +- **DSTU3**: `Dstu3MeasureReportBuilder` has **no** `ClassInstance` branch — it calls + `getValueAsString()` / `getKey()` directly. Part 1's additions to those two methods are what keep + it working. *Descoped: R4 is the target, so no DSTU3 test was added. Part 1's DSTU3 path is + nonetheless intact — `isFhirResource(ClassInstance)` recognises DSTU3 resource names, so a + resource-valued DSTU3 SDE still renders as its id, and untested is not the same as unhandled.* + +### Part 4 +- **`add` then `contains` agree for every value shape the sets hold** — HAPI resource, engine-native + resource, CQL `Date`, CQL `Decimal`, plain `String`. The `Decimal` case fails before Part 4. +- **Engine-native resources with the same id are one element**, including when they differ in + content. Fails before Part 4 under both old relations. +- **A HAPI resource and the `ClassInstance` of the same resource are one element.** +- **`retainAll` / `removeAll` against a plain `List`** — the `MeasureMultiSubjectEvaluator:802,809` + shape — intersect in the set's own relation. +- **`HashSetForCqlExpressionValuesTest`** (new file) covers the same ground through the wrapper: + dedup by wrapped resource, `contains` / `remove` accepting a raw value or a HAPI resource, + `retainAll` against both a wrapper set and a plain list of raws. + +### Performance +- Benchmark `AAB-Details` (38 SDEs) against `AAB-Reporting` (0 SDEs) over a fixed cohort. The + Details-minus-Reporting delta is the metric; it isolates SDE cost from everything else and both + measures ship in the HEDIS 2025 certified content. +- Assert the delta does not scale with resource count per member — that is the property being + restored. + +## Implementation Checklist + +> Marks below are from the diff on `ld-20260901-sde-lazy-conversion`, except the two Part 1 rows the +> benchmark alone supports. The full test suite and `spotlessCheck` pass on **both** CQL 5.2.0 and +> the 5.3.0 snapshot. + +- [x] Part 1: skip eager conversion for `ClassInstance` resources in `normalizeEngineNativeValue` +- [x] Part 1: add `ClassInstance` branches to `getKey()`, `getDescription()`, `getValueAsString()` +- [x] Part 1: handle `getId()` returning null (fall back, do not propagate) +- [x] Part 1: stop hard-coding R4 — done with a version-agnostic `isFhirResource(ClassInstance)` + rather than by threading a `FhirVersionEnum` the construction sites do not have +- [x] Part 1 (unplanned): key on `getIdPart`, not `getId` — see the Solution Overview correction +- [x] Part 2: memoise `getKey()` +- [x] Part 3: pass child definitions through `toFhirValue` recursion; derive target class from them +- [x] Part 3: remove the `parentName` string-replacement heuristic +- [x] Part 3 (unplanned): instantiate through HAPI so a bound code gets its `EnumFactory` +- [x] Part 4: key `HashSetForFhirResourcesAndCqlTypes` on `(resourceType, id)`; O(1) add/contains/remove +- [x] Part 4: extend type detection past `Date` — resolved by deleting `castToCqlTypeIfApplicable` + and `castToResourceIfApplicable`, which keying leaves without callers +- [x] Part 4: unify the relation so `add`/`remove` and `contains`/`retainAll` cannot disagree +- [x] Part 4: decide whether `HashSetForCqlExpressionValues` collapses into the keyed class — it does +- [x] Part 4: verify the `Decimal.equals` vs `EqualEvaluator.equal` divergence is real — it is +- [x] Part 4 (unplanned): override `retainAll` **and** `removeAll`; the inherited versions delegate + to the other collection's `contains` +- [x] Tests: `MedicationDispense.status` bound-code SDE +- [x] Tests: `ClassInstance` vs `IBaseResource` key equivalence +- [ ] Tests: DSTU3 resource-valued SDE — descoped, R4 is the target +- [x] Tests (Part 4): `add` then `contains` agree for every value shape the sets hold +- [x] Tests (Part 4): `retainAll` population intersection unchanged (`MeasureMultiSubjectEvaluator:802,809`) +- [ ] Benchmark: `AAB-Details` minus `AAB-Reporting` delta before/after — Parts 1–2 only; Parts 3–4 + are unmeasured, and the content does not live in this repo +- [ ] Confirm the Part 4 dedup risk: wrappers around equal non-resource raws now collapse + +## Success Criteria + +### Functional +- ✅ `NestedBackboneSdeTest` and the existing measure test suite pass. *One test helper changed: + see Testing Strategy.* +- ✅ `AAB-Details` completes over 400 members with **zero** per-member evaluation failures + (was 266 of 400). *Attributable to the CQL snapshot on that run; Part 3 secures it at this layer.* +- ✅ SDE observation output is byte-identical to current output where current output succeeds. *This + is the criterion that forced `getIdPart` over `getId`.* **Part 4 is the exception and does not + meet it**: keying on the logical id normalises versioned ids and unifies the HAPI and + engine-native forms of a resource, both deliberate. + +### Non-functional +- ✅ SDE cost per member on `AAB-Details` 25 ms (from 2 612 ms), against a target of 100 ms. +- ⏳ Effective parallelism on `AAB-Details` within 20% of `AAB-Reporting` on the same host — + unmeasured, and now expected to be gated by the engine-side hashing rather than by this code. + +### Code quality +- ✅ The `parentName` heuristic is deleted, not extended. +- ✅ No new version-specific hard-coding in the version-agnostic `common` package. *The pre-existing + `convertToFhirR4IfNeeded` call is untouched and still R4-only.* + +## Conclusion + +The 42x regression is one line in a constructor: a full reflective FHIR reconstruction performed to +read an id that the source object already carried. Every component needed to fix it — +`ClassInstanceHelper.getId`, `ClassInstanceHelper.isFhirResource`, and the report builder's +`ClassInstance` branch — is already in the codebase; the eager conversion is what renders them +unreachable. Parts 1–2 are small and low-risk. Part 3 is the larger change, and it converts a class +of recurring crashes into a resolved one by asking HAPI what type a field is instead of guessing. + +**All four parts are implemented on `ld-20260901-sde-lazy-conversion`.** What the work taught that +this document did not anticipate, in one place: + +1. **Converting a `ClassInstance` loses the resource type from the id.** That single fact decided + Part 1's key (`getIdPart`, not `getId`) and, later, made Part 4's id-keying a semantic change + rather than a pure speedup. It was found by writing the equivalence test this document asked for, + which is the argument for writing that kind of test first. +2. **The bound-code crash was already fixed upstream** in the CQL snapshot the benchmark used, so + the 400-of-400 result is not evidence for Parts 1–2. Part 3 still earns its place: it stops the + converter guessing rather than improving the guess. +3. **Both of Part 4's relations were wrong about the same resource**, not just inconsistent with each + other. Structural equality and CQL `=` both call one resource two when its metadata differs, so + id-keying is a correctness fix and not only an O(n²)-to-O(n) one. + +Parts 3 and 4 are covered by tests but unmeasured. The remaining wall-clock is in the CQL engine. diff --git a/cqf-fhir-cql/src/main/java/org/opencds/cqf/fhir/cql/ClassInstanceHelper.kt b/cqf-fhir-cql/src/main/java/org/opencds/cqf/fhir/cql/ClassInstanceHelper.kt index f41e06e596..64027c3fb0 100644 --- a/cqf-fhir-cql/src/main/java/org/opencds/cqf/fhir/cql/ClassInstanceHelper.kt +++ b/cqf-fhir-cql/src/main/java/org/opencds/cqf/fhir/cql/ClassInstanceHelper.kt @@ -16,15 +16,40 @@ object ClassInstanceHelper { val DSTU3_RESOURCE_TYPE_NAMES = org.hl7.fhir.dstu3.model.ResourceType.entries.map { obj -> obj.name } val R4_RESOURCE_TYPE_NAMES = org.hl7.fhir.r4.model.ResourceType.entries.map { obj -> obj.name } + val R4B_RESOURCE_TYPE_NAMES = + org.hl7.fhir.r4b.model.ResourceType.entries.map { obj -> obj.name } + val R5_RESOURCE_TYPE_NAMES = org.hl7.fhir.r5.model.ResourceType.entries.map { obj -> obj.name } + + /** Every name that is a resource type in some modelled FHIR version. See [isFhirResource]. */ + private val ALL_RESOURCE_TYPE_NAMES: Set = + (DSTU3_RESOURCE_TYPE_NAMES + + R4_RESOURCE_TYPE_NAMES + + R4B_RESOURCE_TYPE_NAMES + + R5_RESOURCE_TYPE_NAMES) + .toSet() @JvmStatic fun getId(classInstance: ClassInstance): kotlin.String? { + val idPart = getIdPart(classInstance) ?: return null + return "${classInstance.type.localPart}/$idPart" + } + + /** + * The bare `id.value` of a FHIR [ClassInstance], unqualified by resource type, or null when the + * instance carries no id. + * + * This is the id that a HAPI resource converted from the same instance reports from + * `getIdElement()`: the conversion copies `id.value` and nothing else, so the resource type + * that [getId] prepends is not part of it. Callers that need to agree with a converted resource + * want this; callers building a reference want [getId]. + */ + @JvmStatic + fun getIdPart(classInstance: ClassInstance): kotlin.String? { if (classInstance.type.namespaceURI == fhirModelNamespaceUri && classInstance.has("id")) { val resourceIdInstance = classInstance["id"] as ClassInstance? val resourceIdValue = resourceIdInstance?.get("value") if (resourceIdValue != null) { - val type = classInstance.type.localPart - return "$type/${plainStringValue(resourceIdValue)}" + return plainStringValue(resourceIdValue) } } return null @@ -75,4 +100,21 @@ object ClassInstanceHelper { } return false } + + /** + * Whether the instance is a FHIR resource in any FHIR version modelled here, for callers that + * hold no FHIR version of their own. A [ClassInstance] names its type but not the version that + * type came from, and the versions disagree about which names are resources ("Sequence" is a + * DSTU3 resource; R4 renamed it "MolecularSequence"), so this asks every version rather than + * assuming one. + * + * Sound only for questions whose answer does not depend on the version — telling a resource + * from a complex datatype so it can be keyed by [getIdPart], for instance. Converting to HAPI + * FHIR is not such a question: use the version-qualified overload there. + */ + @JvmStatic + fun isFhirResource(classInstance: ClassInstance): Boolean { + return classInstance.type.namespaceURI == fhirModelNamespaceUri && + ALL_RESOURCE_TYPE_NAMES.contains(classInstance.type.localPart) + } } diff --git a/cqf-fhir-cql/src/main/java/org/opencds/cqf/fhir/cql/engine/parameters/CqlFhirParametersConverter.kt b/cqf-fhir-cql/src/main/java/org/opencds/cqf/fhir/cql/engine/parameters/CqlFhirParametersConverter.kt index 5f1a572451..3253f0ac5f 100644 --- a/cqf-fhir-cql/src/main/java/org/opencds/cqf/fhir/cql/engine/parameters/CqlFhirParametersConverter.kt +++ b/cqf-fhir-cql/src/main/java/org/opencds/cqf/fhir/cql/engine/parameters/CqlFhirParametersConverter.kt @@ -1,6 +1,9 @@ package org.opencds.cqf.fhir.cql.engine.parameters +import ca.uhn.fhir.context.BaseRuntimeChildDefinition import ca.uhn.fhir.context.BaseRuntimeElementCompositeDefinition +import ca.uhn.fhir.context.BaseRuntimeElementDefinition +import ca.uhn.fhir.context.BaseRuntimeElementDefinition.ChildTypeEnum import ca.uhn.fhir.context.FhirContext import ca.uhn.fhir.fhirpath.IFhirPath import ca.uhn.fhir.rest.server.exceptions.InternalErrorException @@ -380,45 +383,37 @@ class CqlFhirParametersConverter( * Converts a CQL [Value] to a HAPI FHIR structure. * * @param valueToConvert The CQL value to convert. - * @param parentName The enclosing FHIR type for nested/inner HAPI FHIR classes representing - * backbone elements. + * @param childDefinition The HAPI child definition this value is being converted into, or null + * at the top level, where there is no enclosing element. HAPI is authoritative about what + * type an element holds - the inner class it declares for a backbone element, or the + * `Enumeration` and its `EnumFactory` behind a bound code - where the CQL value's own type + * name is only a guess at it. */ - fun toFhirValue(valueToConvert: Value, parentName: kotlin.String?): IBase { - var clazz: Class<*>? - val typeName: kotlin.String - when (valueToConvert) { - is NamedTypeValue -> { - typeName = valueToConvert.type.localPart - clazz = modelResolver.resolveType(typeName) - } - - else -> { - typeName = valueToConvert.typeAsString - clazz = null - } - } + fun toFhirValue(valueToConvert: Value, childDefinition: BaseRuntimeChildDefinition?): IBase { + val typeName: kotlin.String = + if (valueToConvert is NamedTypeValue) valueToConvert.type.localPart + else valueToConvert.typeAsString + + val elementDefinition = + childDefinition?.let { elementDefinitionFor(it, valueToConvert, typeName) } + val clazz: Class<*>? = + elementDefinition?.implementingClass + ?: if (valueToConvert is NamedTypeValue) modelResolver.resolveType(typeName) + else null requireNotNull(clazz) { "Could not resolve FHIR type: $typeName" } - if ( - !parentName.isNullOrBlank() && - !clazz.isEnum && - clazz.name.contains("$") && - (clazz.enclosingClass.simpleName != parentName) - ) { - val correctClassName = clazz.name.replace(clazz.enclosingClass.simpleName, parentName) - try { - clazz = Class.forName(correctClassName) - } catch (e: ClassNotFoundException) { - throw IllegalArgumentException("Could not resolve inner FHIR type: $typeName") - } - } val instance: IBase try { - if (clazz.isEnum) { - instance = modelResolver.createHapiInstance(typeName) as IBase - } else { - instance = clazz.getDeclaredConstructor().newInstance() as IBase - } + instance = + when { + // Instantiating through HAPI passes a bound code its EnumFactory, which + // reflecting on the class does not, and without which no code parses. + elementDefinition != null -> + elementDefinition.newInstance(childDefinition.instanceConstructorArguments) + as IBase + clazz.isEnum -> modelResolver.createHapiInstance(typeName) as IBase + else -> clazz.getDeclaredConstructor().newInstance() as IBase + } } catch (e: Exception) { throw IllegalArgumentException("Could not create instance of $typeName", e) } @@ -452,37 +447,62 @@ class CqlFhirParametersConverter( return instance } - val ibaseClazz = clazz as Class - var definition = - fhirContext.getElementDefinition(ibaseClazz) - as BaseRuntimeElementCompositeDefinition<*>? - if (definition == null) { - val resourceClazz = clazz as Class - definition = fhirContext.getResourceDefinition(resourceClazz) - } - - // `toFhirValue()` is called recursively for all subelements of the CQL class instance. If - // the current class is a nested/inner class, the same parent resource name (type name) - // should be used because in HAPI FHIR, all classes representing nested backbone elements - // are declared directly inside the named parent class. - val parentNameForChildren = if (clazz.enclosingClass == null) typeName else parentName + val definition = + elementDefinition as? BaseRuntimeElementCompositeDefinition<*> + ?: compositeDefinitionFor(clazz) for (child in definition.getChildren()) { - val elementValue = (valueToConvert as ClassInstance)[child.elementName] - if (elementValue == null) { - continue - } + val elementValue = (valueToConvert as ClassInstance)[child.elementName] ?: continue if (elementValue is List) { for (item in elementValue) { - child.mutator.addValue(instance, toFhirValue(item!!, parentNameForChildren)) + child.mutator.addValue(instance, toFhirValue(item!!, child)) } } else { - child.mutator.addValue(instance, toFhirValue(elementValue, parentNameForChildren)) + child.mutator.addValue(instance, toFhirValue(elementValue, child)) } } return instance } + /** + * The HAPI element definition for a value being converted into [childDefinition], or null when + * HAPI cannot settle it on its own and the CQL type name has to answer instead. + */ + private fun elementDefinitionFor( + childDefinition: BaseRuntimeChildDefinition, + valueToConvert: Value, + typeName: kotlin.String, + ): BaseRuntimeElementDefinition<*>? { + val validChildNames = childDefinition.validChildNames + val definition = + if (validChildNames.size == 1) { + childDefinition.getChildByName(validChildNames.first()) + } else { + // A choice element ([x]) holds one of several types, so the CQL value's own + // type picks between them; HAPI still supplies the class it will accept. + val candidate = + if (valueToConvert is NamedTypeValue) + runCatching { modelResolver.resolveType(typeName) }.getOrNull() + else null + if (candidate != null && IBase::class.java.isAssignableFrom(candidate)) { + @Suppress("UNCHECKED_CAST") + childDefinition.getChildElementDefinitionByDatatype( + candidate as Class + ) + } else null + } + return definition?.takeIf { SUPPORTED_CHILD_TYPES.contains(it.childType) } + } + + @Suppress("UNCHECKED_CAST") + private fun compositeDefinitionFor(clazz: Class<*>): BaseRuntimeElementCompositeDefinition<*> { + val elementDefinition = + fhirContext.getElementDefinition(clazz as Class) + as BaseRuntimeElementCompositeDefinition<*>? + return elementDefinition + ?: fhirContext.getResourceDefinition(clazz as Class) + } + private fun convertToCql(ppca: IParametersParameterComponentAdapter): Value? { if (ppca.hasValue()) { return this.fhirTypeConverter.toCqlType(ppca.getValue()) as Value? @@ -496,6 +516,21 @@ class CqlFhirParametersConverter( } companion object { + /** + * The element kinds this converter can instantiate from a HAPI child definition. + * `contained` (a CONTAINED_RESOURCE_LIST, whose implementing class is the IBaseResource + * interface) and `Narrative.div` (an XhtmlNode, which is not an IBase at all) are left to + * the CQL type name, which handles them no worse than it did before. + */ + private val SUPPORTED_CHILD_TYPES = + setOf( + ChildTypeEnum.PRIMITIVE_DATATYPE, + ChildTypeEnum.ID_DATATYPE, + ChildTypeEnum.COMPOSITE_DATATYPE, + ChildTypeEnum.RESOURCE_BLOCK, + ChildTypeEnum.RESOURCE, + ) + // This is basically a copy and paste from R4FhirTypeConverter, but it's not exposed. const val EMPTY_LIST_EXT_URL: kotlin.String = "http://hl7.org/fhir/StructureDefinition/cqf-isEmptyList" diff --git a/cqf-fhir-cql/src/test/java/org/opencds/cqf/fhir/cql/engine/parameters/CqlFhirParametersConverterTests.java b/cqf-fhir-cql/src/test/java/org/opencds/cqf/fhir/cql/engine/parameters/CqlFhirParametersConverterTests.java index a5e0f286fb..c758624565 100644 --- a/cqf-fhir-cql/src/test/java/org/opencds/cqf/fhir/cql/engine/parameters/CqlFhirParametersConverterTests.java +++ b/cqf-fhir-cql/src/test/java/org/opencds/cqf/fhir/cql/engine/parameters/CqlFhirParametersConverterTests.java @@ -77,8 +77,9 @@ void evaluationResultToParameters() { var testData = new EvaluationResult(); testData.set( new EvaluationExpressionRef("Patient"), - new ExpressionResult(new ClassInstance(new QName(fhirModelNamespaceUri, "Patient"), Map.of()), null)); - testData.set(new EvaluationExpressionRef("Numerator"), new ExpressionResult(new Boolean(true), null)); + new ExpressionResult( + new ClassInstance(new QName(fhirModelNamespaceUri, "Patient"), Map.of()), Map.of())); + testData.set(new EvaluationExpressionRef("Numerator"), new ExpressionResult(new Boolean(true), Map.of())); var actual = (Parameters) cqlFhirParametersConverter.toFhirParameters(testData); @@ -94,9 +95,11 @@ void evaluationResultToEmptyListParameters() { var testData = new EvaluationResult(); testData.set( new EvaluationExpressionRef("Patient"), - new ExpressionResult(new ClassInstance(new QName(fhirModelNamespaceUri, "Patient"), Map.of()), null)); + new ExpressionResult( + new ClassInstance(new QName(fhirModelNamespaceUri, "Patient"), Map.of()), Map.of())); testData.set( - new EvaluationExpressionRef("Encounters"), new ExpressionResult(List.Companion.getEMPTY_LIST(), null)); + new EvaluationExpressionRef("Encounters"), + new ExpressionResult(List.Companion.getEMPTY_LIST(), Map.of())); Parameters actual = (Parameters) cqlFhirParametersConverter.toFhirParameters(testData); @@ -116,7 +119,7 @@ void evaluationResultsWithListContainingNullValue() { var cqlList = new List(testList); var testData = new EvaluationResult(); - testData.set(new EvaluationExpressionRef("NullInList"), new ExpressionResult(cqlList, null)); + testData.set(new EvaluationExpressionRef("NullInList"), new ExpressionResult(cqlList, Map.of())); var actual = (Parameters) cqlFhirParametersConverter.toFhirParameters(testData); @@ -132,8 +135,9 @@ void evaluationResultNullParameters() { var testData = new EvaluationResult(); testData.set( new EvaluationExpressionRef("Patient"), - new ExpressionResult(new ClassInstance(new QName(fhirModelNamespaceUri, "Patient"), Map.of()), null)); - testData.set(new EvaluationExpressionRef("Null"), new ExpressionResult(null, null)); + new ExpressionResult( + new ClassInstance(new QName(fhirModelNamespaceUri, "Patient"), Map.of()), Map.of())); + testData.set(new EvaluationExpressionRef("Null"), new ExpressionResult(null, Map.of())); var actual = (Parameters) cqlFhirParametersConverter.toFhirParameters(testData); diff --git a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/CqlExpressionValue.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/CqlExpressionValue.java index 42e4420e66..fbc2f5691a 100644 --- a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/CqlExpressionValue.java +++ b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/CqlExpressionValue.java @@ -38,14 +38,21 @@ private CqlExpressionValue(@Nullable String expressionName, @Nullable Object raw /** * Wraps an {@link ExpressionResult}. Accepts a null result and yields an empty wrapper. + *

+ * The engine hands back its evaluated resources as a Map keyed by resource id. This pipeline + * carries them as a Set, so the keys are dropped here - but into a set that keys on the same + * identity, not a plain {@code HashSet}, which would hash each resource by walking its element + * graph and give back the cost the engine's own keying exists to avoid. */ public static CqlExpressionValue of(@Nullable String expressionName, @Nullable ExpressionResult result) { if (result == null) { return EMPTY; } - var resources = result.getEvaluatedResources(); return new CqlExpressionValue( - expressionName, result.getValue(), resources != null ? resources : Collections.emptySet()); + expressionName, + result.getValue(), + new HashSetForFhirResourcesAndCqlTypes<>( + result.getEvaluatedResources().values())); } /** diff --git a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/FhirResourceAndCqlTypeUtils.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/FhirResourceAndCqlTypeUtils.java index 556ea89296..bd033d63ac 100644 --- a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/FhirResourceAndCqlTypeUtils.java +++ b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/FhirResourceAndCqlTypeUtils.java @@ -5,17 +5,20 @@ import java.util.Objects; import org.hl7.fhir.instance.model.api.IBaseResource; import org.opencds.cqf.cql.engine.elm.executing.EqualEvaluator; -import org.opencds.cqf.cql.engine.runtime.Date; +import org.opencds.cqf.cql.engine.runtime.ClassInstance; import org.opencds.cqf.cql.engine.runtime.Value; +import org.opencds.cqf.fhir.cql.ClassInstanceHelper; /** * Utility class providing equality comparison methods for FHIR resources and CQL types. *

- * FHIR resources are compared by their resource type and logical ID (IdElement), - * while CQL types are compared using their {@code equal()} method. + * FHIR resources are compared by their resource type and logical ID, whether they arrive as HAPI + * objects or as the CQL engine's native {@link ClassInstance}. Everything else is compared with CQL + * {@code equal()} semantics, or {@code Objects.equals} for values that are neither. *

- * This class exists to compensate for the fact that FHIR resource classes and CQL types - * do not implement equals() and hashCode() in a way that reflects their logical identity. + * This relation is the one {@link IdentityKey} keys on, so a set or map built on those keys answers + * {@code add}, {@code contains}, {@code remove} and {@code retainAll} with the relation implemented + * here rather than with whatever {@code equals()} the value happens to carry. */ public class FhirResourceAndCqlTypeUtils { @@ -24,13 +27,47 @@ private FhirResourceAndCqlTypeUtils() { } public static boolean areObjectsEqual(Object obj, Object item) { - if (obj instanceof IBaseResource objResource && item instanceof IBaseResource itemResource) { - return areEqualResources(objResource, itemResource); - } else if (obj instanceof Value objCqlType && item instanceof Value itemCqlType) { + final String objIdentity = resourceIdentity(obj); + final String itemIdentity = resourceIdentity(item); + + // A resource is only ever equal to the same resource, never to a non-resource, so one + // identity being present settles the comparison on its own. + if (objIdentity != null || itemIdentity != null) { + return objIdentity != null && objIdentity.equals(itemIdentity); + } + + if (obj instanceof Value objCqlType && item instanceof Value itemCqlType) { return areEqualCqlTypes(objCqlType, itemCqlType); - } else { - return Objects.equals(item, obj); } + + return Objects.equals(item, obj); + } + + /** + * The identity a FHIR resource is compared by: its resource type and logical id, rendered as + * {@code Type/id}. Null for anything that is not a FHIR resource. + *

+ * A HAPI resource and the engine-native {@link ClassInstance} of the same resource yield the + * same identity. They are the same resource, and since SDE accumulation stopped converting + * resources eagerly the pipeline holds both forms. + *

+ * A HAPI resource with no id keeps the relation this class has always had - all id-less + * resources of a type are one. An id-less {@code ClassInstance} has no identity at all and + * falls back to CQL equality, which is the relation that form already had. + */ + @Nullable + public static String resourceIdentity(Object value) { + if (value instanceof IBaseResource resource) { + var idElement = resource.getIdElement(); + var idPart = idElement == null ? null : idElement.getIdPart(); + return resource.fhirType() + "/" + (idPart == null ? "" : idPart); + } + + if (value instanceof ClassInstance classInstance && ClassInstanceHelper.isFhirResource(classInstance)) { + return ClassInstanceHelper.getId(classInstance); + } + + return null; } public static boolean areEqualResources(IBaseResource resource1, IBaseResource resource2) { @@ -38,48 +75,28 @@ public static boolean areEqualResources(IBaseResource resource1, IBaseResource r return true; } - if (resource1.getIdElement() == null || resource2.getIdElement() == null) { + if (resource1 == null || resource2 == null) { return false; } - // In case we have IDs that are identical but different resource types, - // e.g. Patient/123 vs Observation/123 - if (resource1.getClass() != resource2.getClass()) { - return false; - } - - return Objects.equals(resource1.getIdElement(), resource2.getIdElement()); + return resourceIdentity(resource1).equals(resourceIdentity(resource2)); } - public static boolean areEqualCqlTypes(Value cqlDate1, Value cqlDate2) { - if (cqlDate1 == cqlDate2) { + public static boolean areEqualCqlTypes(Value cqlValue1, Value cqlValue2) { + if (cqlValue1 == cqlValue2) { return true; } - if (cqlDate1 == null || cqlDate2 == null) { + if (cqlValue1 == null || cqlValue2 == null) { return false; } // We're relying on all CqlTypes to implement equal() properly // Note this is equal(), not Object.equals() - var result = EqualEvaluator.equal(cqlDate1, cqlDate2); + var result = EqualEvaluator.equal(cqlValue1, cqlValue2); return result != null && result.getValue(); } - public static IBaseResource castToResourceIfApplicable(Object obj) { - if (obj instanceof IBaseResource resource) { - return resource; - } - return null; - } - - public static Value castToCqlTypeIfApplicable(Object obj) { - if (obj instanceof Date cqlDate) { - return cqlDate; - } - return null; - } - /** * Find a key in a map that matches the given key using FHIR/CQL equality semantics. *

diff --git a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/FunctionEvaluationHandler.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/FunctionEvaluationHandler.java index 6a9c58a113..bf1867376b 100644 --- a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/FunctionEvaluationHandler.java +++ b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/FunctionEvaluationHandler.java @@ -4,7 +4,6 @@ import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import java.util.ArrayList; import java.util.Collection; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; @@ -273,7 +272,7 @@ private static CqlEvaluationResult processMeasureObservation( // FhirResourceAndCqlTypeUtils.areObjectsEqual where needed; nothing in the downstream pipeline // does random-access lookup by input, so a List is sufficient and self-documenting. final List functionResults = new ArrayList<>(); - final Set evaluatedResources = new HashSet<>(); + final Set evaluatedResources = new HashSetForFhirResourcesAndCqlTypes<>(); final String exceptionMessageIfNotFunction = """ Measure: '%s', MeasureObservation population expression '%s' must be a CQL function @@ -293,7 +292,7 @@ private static CqlEvaluationResult processMeasureObservation( var quantity = convertCqlResultToQuantityDef(observationResult.getValue()); functionResults.add(new ObservationEntry(result, quantity)); - Optional.ofNullable(observationResult.getEvaluatedResources()).ifPresent(evaluatedResources::addAll); + evaluatedResources.addAll(observationResult.getEvaluatedResources().values()); } return buildEvaluationResult(expressionName, new ObservationAccumulator(functionResults), evaluatedResources); @@ -407,7 +406,7 @@ private static void processNonSubValueStratifier( // this will be used in MeasureEvaluator (Criteria population Id and Stratifier Expression) var expressionName = popDef.id() + "-" + stratifierExpression; final List functionResults = new ArrayList<>(); - final Set evaluatedResources = new HashSet<>(); + final Set evaluatedResources = new HashSetForFhirResourcesAndCqlTypes<>(); for (var result : resultsIter) { final ExpressionResult functionResult = evaluateNonSubValueStratifiersFunction( @@ -420,13 +419,7 @@ private static void processNonSubValueStratifier( // heterogeneous CQL value the function returned. Iteration order is the order // populationDef results were iterated. functionResults.add(new FunctionResultEntry(result, functionResult.getValue())); - var evaluated = functionResult.getEvaluatedResources(); - if (evaluated == null) { - throw new IllegalStateException("CQL function '" + stratifierExpression - + "' returned null evaluatedResources for measure: " + measureUrl); - } - evaluatedResources.addAll(evaluated); - evaluatedResources.addAll(functionResult.getEvaluatedResources()); + evaluatedResources.addAll(functionResult.getEvaluatedResources().values()); } // add to EvaluationResult addToEvaluationResult( diff --git a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForCqlExpressionValues.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForCqlExpressionValues.java index da01c08e08..74f41f66aa 100644 --- a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForCqlExpressionValues.java +++ b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForCqlExpressionValues.java @@ -1,149 +1,42 @@ package org.opencds.cqf.fhir.cr.measure.common; -import jakarta.annotation.Nonnull; import java.util.Collection; -import java.util.HashSet; -import java.util.Iterator; -import java.util.Objects; import java.util.stream.Collectors; import org.hl7.fhir.instance.model.api.IBaseResource; /** - * A {@link HashSet} of {@link CqlExpressionValue} that compares elements by the FHIR-resource and - * CQL-type identity rules of their underlying value (via {@link FhirResourceAndCqlTypeUtils}). + * A Set of {@link CqlExpressionValue} that identifies elements by what they wrap. *

* Sister type to {@link HashSetForFhirResourcesAndCqlTypes} for use when the population pipeline - * stores wrappers rather than raw {@link Object}s. Two wrappers around FHIR resources with the - * same resource type and logical ID are considered equal, even if the wrappers (or the underlying - * resource instances) are different object instances. Same applies to CQL types via - * {@link org.opencds.cqf.cql.engine.runtime.CqlType#equal}. + * stores wrappers rather than raw {@link Object}s: two wrappers around the same FHIR resource are + * one element, and {@code contains} / {@code remove} accept either a wrapper or a raw value, so a + * caller can ask "does this set hold resource X?" directly. *

- * Bucket placement still uses the wrapper's default {@code Object.hashCode()} (the wrapper - * doesn't implement {@code equals} / {@code hashCode}), so {@code add} / {@code remove} / - * {@code contains} / {@code retainAll} fall through to linear-time identity checks via - * {@link FhirResourceAndCqlTypeUtils#areObjectsEqual}. This is acceptable — per-subject - * population sets are small. + * Keying on the wrapped value is the whole of the difference from the parent, which is why this is + * a few lines rather than a parallel implementation. It used to be a parallel implementation, and + * the two drifted: the wrapper carries no {@code equals}, so {@code add} deduplicated by wrapper + * identity - that is, not at all - while {@code contains} compared what was wrapped. */ -@SuppressWarnings("squid:S3776") -public class HashSetForCqlExpressionValues extends HashSet { +public class HashSetForCqlExpressionValues extends HashSetForFhirResourcesAndCqlTypes { public HashSetForCqlExpressionValues() { super(); } public HashSetForCqlExpressionValues(Collection collection) { - super(); - for (CqlExpressionValue value : collection) { - this.add(value); - } + super(collection); } public HashSetForCqlExpressionValues(Iterable iterable) { - super(); - for (CqlExpressionValue value : iterable) { - this.add(value); - } - } - - /** - * Linear-search check that any wrapper in this set has an underlying value equal — by FHIR - * resource / CQL type identity — to {@code other}. Accepts either a {@link CqlExpressionValue} - * (the typical case) or a raw object (so callers can ask "does this set contain a wrapper - * around resource X?" directly). - */ - @Override - public boolean contains(Object other) { - return containsByIdentity(this, unwrap(other)); + super(iterable); } /** - * Adds {@code newElement} only if no existing wrapper in this set has an underlying value - * equal to {@code newElement.raw()} by FHIR identity. + * Keys on the wrapped value, so a wrapper and the raw value it wraps resolve to the same key. */ @Override - public boolean add(CqlExpressionValue newElement) { - if (newElement == null) { - return super.add(null); - } - Object newRaw = newElement.raw(); - if (newRaw == null - || (FhirResourceAndCqlTypeUtils.castToResourceIfApplicable(newRaw) == null - && FhirResourceAndCqlTypeUtils.castToCqlTypeIfApplicable(newRaw) == null)) { - return super.add(newElement); - } - for (CqlExpressionValue existing : this) { - if (existing != null && FhirResourceAndCqlTypeUtils.areObjectsEqual(existing.raw(), newRaw)) { - return false; - } - } - return super.add(newElement); - } - - /** - * Removes the wrapper whose underlying value matches {@code removalCandidate} by FHIR - * identity. {@code removalCandidate} may be a {@link CqlExpressionValue} or a raw resource. - */ - @Override - public boolean remove(Object removalCandidate) { - Object targetRaw = unwrap(removalCandidate); - if (targetRaw == null) { - return super.remove(removalCandidate); - } - if (FhirResourceAndCqlTypeUtils.castToResourceIfApplicable(targetRaw) == null - && FhirResourceAndCqlTypeUtils.castToCqlTypeIfApplicable(targetRaw) == null) { - return super.remove(removalCandidate); - } - for (CqlExpressionValue existing : this) { - if (existing != null && FhirResourceAndCqlTypeUtils.areObjectsEqual(existing.raw(), targetRaw)) { - return super.remove(existing); - } - } - return false; - } - - @Override - public boolean retainAll(@Nonnull Collection otherCollection) { - Objects.requireNonNull(otherCollection); - - if (otherCollection instanceof HashSetForCqlExpressionValues) { - return super.retainAll(otherCollection); - } - - boolean modified = false; - Iterator it = iterator(); - while (it.hasNext()) { - CqlExpressionValue next = it.next(); - if (!otherContains(otherCollection, next)) { - it.remove(); - modified = true; - } - } - return modified; - } - - private static boolean otherContains(Collection collection, CqlExpressionValue value) { - Object raw = value == null ? null : value.raw(); - for (Object other : collection) { - Object otherRaw = unwrap(other); - if (FhirResourceAndCqlTypeUtils.areObjectsEqual(raw, otherRaw)) { - return true; - } - } - return false; - } - - private static boolean containsByIdentity(Iterable elements, Object targetRaw) { - for (CqlExpressionValue existing : elements) { - Object existingRaw = existing == null ? null : existing.raw(); - if (FhirResourceAndCqlTypeUtils.areObjectsEqual(existingRaw, targetRaw)) { - return true; - } - } - return false; - } - - private static Object unwrap(Object o) { - return o instanceof CqlExpressionValue v ? v.raw() : o; + IdentityKey keyFor(Object element) { + return super.keyFor(element instanceof CqlExpressionValue value ? value.raw() : element); } @Override @@ -151,13 +44,13 @@ public String toString() { if (isEmpty()) { return "[]"; } - Object firstRaw = iterator().next() == null ? null : iterator().next().raw(); - if (firstRaw instanceof IBaseResource) { + var firstElement = iterator().next(); + if (firstElement != null && firstElement.raw() instanceof IBaseResource) { return stream() .map(CqlExpressionValue::raw) .filter(IBaseResource.class::isInstance) .map(IBaseResource.class::cast) - .map(r -> r.getIdElement().getValueAsString()) + .map(resource -> resource.getIdElement().getValueAsString()) .collect(Collectors.joining(",", "[", "]")); } return super.toString(); diff --git a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForFhirResourcesAndCqlTypes.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForFhirResourcesAndCqlTypes.java index fbd76665c1..674ef62f72 100644 --- a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForFhirResourcesAndCqlTypes.java +++ b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForFhirResourcesAndCqlTypes.java @@ -1,38 +1,53 @@ package org.opencds.cqf.fhir.cr.measure.common; import jakarta.annotation.Nonnull; +import java.util.AbstractSet; import java.util.Collection; import java.util.HashSet; import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.Map; import java.util.Objects; import java.util.stream.Collectors; import org.hl7.fhir.instance.model.api.IBaseResource; -import org.hl7.fhir.instance.model.api.IPrimitiveType; import org.opencds.cqf.cql.engine.runtime.Value; /** - * A HashSet implementation that uses FHIR resource identity rules when comparing resources or - * Cql.equal. - * This means that two resources with the same resource type and logical ID are considered - * equal, even if they are different object instances. + * A Set that identifies FHIR resources by resource type and logical ID, so two objects describing + * the same resource are one element even when they are different instances - or different + * representations, since a resource reaches this pipeline either as a HAPI object or as the CQL + * engine's native {@link org.opencds.cqf.cql.engine.runtime.ClassInstance}. Values that are not + * resources keep CQL {@code =} semantics, and everything else its own {@code equals}. *

- * This class exists strictly to compensate for the fact that FHIR resource classes and CQL types - * do not implement equals() and hashCode(). + * Elements are stored under an {@link IdentityKey}, which is what makes that one relation apply to + * every operation. The class this replaces compared elements pairwise, which had two consequences + * worth stating, since both are the reason for the rewrite rather than incidental to it: + *

    + *
  • {@code add} and {@code remove} routed CQL values into {@code HashSet}'s own equality while + * {@code contains} and {@code retainAll} routed them into CQL {@code =}. Under CQL 5 every + * expression result is a {@code ClassInstance}, which those two relations need not agree + * about, so a set could fail to contain a value it had just added.
    • + *
  • Every operation was a linear scan, and under CQL 5 each comparison in that scan walked a + * resource graph. Building an n-element set was O(n²) in deep structural comparisons.
    • + *
* *

For a wrapper-aware sister type used by {@code PopulationDef.subjectResources}, see * {@link HashSetForCqlExpressionValues}. + * * @param the type of elements in this set, which may or may not be a {@link IBaseResource} * or a {@link Value} */ -@SuppressWarnings("squid:S3776") -public class HashSetForFhirResourcesAndCqlTypes extends HashSet { +public class HashSetForFhirResourcesAndCqlTypes extends AbstractSet { + + /** Insertion-ordered so iteration is stable, as it was when this extended {@code HashSet}. */ + private final Map elementsByKey = new LinkedHashMap<>(); public HashSetForFhirResourcesAndCqlTypes() { super(); } public HashSetForFhirResourcesAndCqlTypes(Collection collection) { - super(collection); + this((Iterable) collection); } public HashSetForFhirResourcesAndCqlTypes(Iterable iterable) { @@ -48,144 +63,90 @@ public HashSetForFhirResourcesAndCqlTypes(T singleValue) { } /** - * This logic is triggered by retainAll() and removeAll(), whose behaviour we're trying - * to modify to use FHIR resource identity rules. - * - * @param other object to be checked for containment in this set - * @return true if this set contains the specified element + * The key {@code element} is stored under. Subclasses that hold wrappers rather than the values + * themselves override this to key on what they wrap. + *

+ * Package-private rather than protected: {@link IdentityKey} is an implementation detail of this + * package, so a subclass outside it could neither name the return type nor override this. */ - @Override - public boolean contains(Object other) { - return contains(this, other); + IdentityKey keyFor(Object element) { + return IdentityKey.of(element); } - /** - * If we don't override this logic, we'll get duplicate resources since the comparison to - * existing resources in the set will be based on object identity, not FHIR resource identity - * The default implementation calls to HashMap, which means it's not based on contains() - *

- * This is also called from super.allAll() - * - * @param newElement element to be added to this set - * @return true if this set did not already contain the specified element - */ @Override public boolean add(T newElement) { - final IBaseResource newElementResource = FhirResourceAndCqlTypeUtils.castToResourceIfApplicable(newElement); - - if (newElementResource != null) { - if (this.contains(newElementResource)) { - return false; - } else { - return super.add(newElement); - } - } - - final Value newElementCqlType = FhirResourceAndCqlTypeUtils.castToCqlTypeIfApplicable(newElement); - - if (newElementCqlType != null) { - if (this.contains(newElementCqlType)) { - return false; - } else { - return super.add(newElement); - } + var key = keyFor(newElement); + if (elementsByKey.containsKey(key)) { + return false; } + elementsByKey.put(key, newElement); + return true; + } - return super.add(newElement); + @Override + public boolean contains(Object other) { + return elementsByKey.containsKey(keyFor(other)); } - /** - * If we don't override this logic, we'll get duplicate resources since the comparison to - * existing resources in the set will be based on object identity, not FHIR resource identity - * The default implementation calls to HashMap, which means it's not based on contains() - *

- * This is also called from super.removeAll() - * - * @param removalCandidate object to be removed from this set, if present - * @return true if this set contained the specified element - */ @Override public boolean remove(Object removalCandidate) { - final IBaseResource removalCandidateResource = - FhirResourceAndCqlTypeUtils.castToResourceIfApplicable(removalCandidate); - - if (removalCandidateResource != null) { - for (T next : this) { - if (next instanceof IBaseResource nextResource - && FhirResourceAndCqlTypeUtils.areEqualResources(nextResource, removalCandidateResource)) { - return super.remove(nextResource); - } - } + var key = keyFor(removalCandidate); + if (!elementsByKey.containsKey(key)) { return false; } - - final Value removalCandidateCqlType = FhirResourceAndCqlTypeUtils.castToCqlTypeIfApplicable(removalCandidate); - - if (removalCandidateCqlType != null) { - for (T next : this) { - if (next instanceof Value nextCqlType - && FhirResourceAndCqlTypeUtils.areEqualCqlTypes(nextCqlType, removalCandidateCqlType)) { - return super.remove(nextCqlType); - } - } - return false; - } - - return super.remove(removalCandidate); + elementsByKey.remove(key); + return true; } /** - * If we don't override this logic, we'll get duplicate resources since the comparison to - * existing resources in the set will be based on object identity, not FHIR resource identity - * The default implementation calls to HashMap, which means it's not based on contains() + * Retains the elements whose identity appears in {@code otherCollection}. *

- * - * @param otherCollection collection containing elements to be retained in this set - * @return true if this set changed as a result of the call + * The inherited implementation would ask {@code otherCollection} what it contains, and a plain + * {@code List} or {@code HashSet} answers that by Java object identity - which is how a + * population intersection silently drops resources. Keying the other collection first means the + * comparison runs in this set's relation regardless of what the other collection is. */ @Override public boolean retainAll(@Nonnull Collection otherCollection) { Objects.requireNonNull(otherCollection); - // Both Collections are HashSetForFhirResources, so we can use the default implementation, - // which calls HashSetForFhirResources.contains(). - if (otherCollection instanceof HashSetForFhirResourcesAndCqlTypes) { - return super.retainAll(otherCollection); + var retainedKeys = new HashSet(); + for (Object other : otherCollection) { + retainedKeys.add(keyFor(other)); } + return elementsByKey.keySet().retainAll(retainedKeys); + } + + /** + * Removes the elements whose identity appears in {@code otherCollection}. Overridden for the + * same reason as {@link #retainAll(Collection)}: the inherited implementation delegates to the + * other collection's {@code contains} once this set is the smaller of the two. + */ + @Override + public boolean removeAll(@Nonnull Collection otherCollection) { + Objects.requireNonNull(otherCollection); - // Now we're dealing with another Collection, which calls its own contains() method when - // we invoke super.retainAll() boolean modified = false; - Iterator it = iterator(); - while (it.hasNext()) { - if (!contains(otherCollection, it.next())) { - it.remove(); - modified = true; - } + for (Object other : otherCollection) { + modified |= remove(other); } return modified; } - private static boolean contains(Collection collection, Object obj) { - final IBaseResource otherResource = FhirResourceAndCqlTypeUtils.castToResourceIfApplicable(obj); - final Value otherCqlType = FhirResourceAndCqlTypeUtils.castToCqlTypeIfApplicable(obj); - - // prevent infinite recursion - if (otherResource != null || otherCqlType != null || collection instanceof HashSetForFhirResourcesAndCqlTypes) { - return containsInner(collection, obj); - } - - return collection.contains(obj); + @Override + @Nonnull + public Iterator iterator() { + return elementsByKey.values().iterator(); } - private static boolean containsInner(Collection collection, Object obj) { - for (Object item : collection) { - if (FhirResourceAndCqlTypeUtils.areObjectsEqual(obj, item)) { - return true; - } - } + @Override + public int size() { + return elementsByKey.size(); + } - return false; + @Override + public void clear() { + elementsByKey.clear(); } @Override @@ -199,8 +160,7 @@ public String toString() { if (firstElement instanceof IBaseResource) { return stream() .map(IBaseResource.class::cast) - .map(IBaseResource::getIdElement) - .map(IPrimitiveType::getValueAsString) + .map(resource -> resource.getIdElement().getValueAsString()) .collect(Collectors.joining(",", "[", "]")); } diff --git a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/IdentityKey.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/IdentityKey.java new file mode 100644 index 0000000000..ee76085561 --- /dev/null +++ b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/IdentityKey.java @@ -0,0 +1,70 @@ +package org.opencds.cqf.fhir.cr.measure.common; + +import java.util.Objects; +import org.opencds.cqf.cql.engine.runtime.Value; + +/** + * The key a value is stored under in {@link HashSetForFhirResourcesAndCqlTypes}, carrying the + * identity {@link FhirResourceAndCqlTypeUtils} defines for it. + *

+ * Keying is what lets one relation answer every set operation. Comparing elements pairwise instead + * - which is what these collections used to do - lets {@code add} and {@code contains} disagree, + * because they reached for different notions of equality, and costs a linear scan per operation. + */ +sealed interface IdentityKey { + + static IdentityKey of(Object value) { + var resourceIdentity = FhirResourceAndCqlTypeUtils.resourceIdentity(value); + if (resourceIdentity != null) { + return new ResourceKey(resourceIdentity); + } + if (value instanceof Value cqlValue) { + return new CqlValueKey(cqlValue); + } + return new PlainKey(value); + } + + /** + * A FHIR resource, keyed by resource type and logical id. This is the case that matters: it + * turns a deep structural walk of a resource graph into a string comparison. + */ + record ResourceKey(String identity) implements IdentityKey {} + + /** + * A CQL value that is not a FHIR resource, compared with CQL {@code =}. + *

+ * CQL {@code =} is not hash-compatible - {@code 1.0 = 1.00} is true where the two have different + * structural hashes, and DateTime comparison across precisions can be uncertain rather than + * false - so there is no key to spread these across buckets. They all share one, which makes + * lookups among them linear, exactly as before. There are few: resources take the keyed path + * above, and these are the {@code Date}s and {@code Decimal}s a stratifier returns. + */ + record CqlValueKey(Value value) implements IdentityKey { + + @Override + public int hashCode() { + return 0; + } + + @Override + public boolean equals(Object other) { + return this == other + || (other instanceof CqlValueKey otherKey + && FhirResourceAndCqlTypeUtils.areEqualCqlTypes(value, otherKey.value)); + } + } + + /** Anything else - a String, a Map of criteria results - on its own {@code equals}/{@code hashCode}. */ + record PlainKey(Object value) implements IdentityKey { + + @Override + public int hashCode() { + return Objects.hashCode(value); + } + + @Override + public boolean equals(Object other) { + return this == other || (other instanceof PlainKey otherKey && Objects.equals(value, otherKey.value)); + } + } +} diff --git a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/SdeDef.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/SdeDef.java index 5c46043128..c9a7e9eecb 100644 --- a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/SdeDef.java +++ b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/SdeDef.java @@ -1,7 +1,6 @@ package org.opencds.cqf.fhir.cr.measure.common; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.function.Function; @@ -18,7 +17,9 @@ public class SdeDef { // Pre-accumulated state (populated by MeasureMultiSubjectEvaluator) private final Map accumulatedValues = new HashMap<>(); - private final Set allEvaluatedResources = new HashSet<>(); + // Keyed on resource identity: this accumulates the engine's evaluated resources across every + // subject, and a plain HashSet would hash each one by walking its element graph. + private final Set allEvaluatedResources = new HashSetForFhirResourcesAndCqlTypes<>(); public SdeDef(String id, ConceptDef code, String expression) { this(id, code, expression, null); diff --git a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/StratumValueWrapper.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/StratumValueWrapper.java index 0fdd6a0b55..cbcbeb71e8 100644 --- a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/StratumValueWrapper.java +++ b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/StratumValueWrapper.java @@ -9,6 +9,7 @@ import org.hl7.fhir.instance.model.api.IBaseCoding; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IPrimitiveType; +import org.opencds.cqf.cql.engine.runtime.ClassInstance; import org.opencds.cqf.cql.engine.runtime.Code; import org.opencds.cqf.fhir.cql.ClassInstanceHelper; import org.opencds.cqf.fhir.utility.adapter.IAdapterFactory; @@ -22,6 +23,13 @@ public class StratumValueWrapper { protected Object value; + /** + * {@link #getKey()} is called once per element by {@code hashCode()} and twice per comparison by + * {@code equals()}, so accumulating a frequency map walks the rendering chain below several times + * for every value. The wrapped value does not change after construction, so the key does not either. + */ + private String cachedKey; + public StratumValueWrapper(Object value) { this.value = normalizeEngineNativeValue(value); } @@ -36,7 +44,16 @@ public StratumValueWrapper(Object value) { * the FHIR-typed and primitive rendering branches apply. */ private static Object normalizeEngineNativeValue(Object rawValue) { - // FHIR-namespaced ClassInstance -> HAPI FHIR R4 typed object (resource, CodeableConcept, ...) + // A FHIR *resource* is rendered here by its id alone, and the ClassInstance already carries it. + // Converting one first means reflectively rebuilding its whole element graph - for an + // ExplanationOfBenefit, dozens of nested backbone elements - to read a single field off it and + // discard the rest, once per occurrence per subject. Leave resources engine-native; the report + // builders convert the ones they actually render. + if (rawValue instanceof ClassInstance classInstance && ClassInstanceHelper.isFhirResource(classInstance)) { + return rawValue; + } + // Complex datatypes (Coding, CodeableConcept, Identifier) still convert: the rendering below + // reads their contents through IAdapterFactory, and they are a handful of primitives, not a graph. var converted = ClassInstanceHelper.convertToFhirR4IfNeeded(rawValue); if (converted != rawValue) { return converted; @@ -110,6 +127,13 @@ public String toString() { private static final String EMPTY_STRATUM_VALUE = "empty"; public String getKey() { + if (cachedKey == null) { + cachedKey = computeKey(); + } + return cachedKey; + } + + private String computeKey() { var wrapper = CqlExpressionValue.ofRaw(null, value, null); // Handle null values - group them into a special "null" stratum if (wrapper.isNull()) { @@ -122,6 +146,7 @@ public String getKey() { } String key = null; + var engineNativeResourceId = engineNativeResourceId(value); if (value instanceof IBaseCoding) { // ASSUMPTION: We won't have different systems with the same code // within a given stratifier / sde @@ -140,6 +165,8 @@ public String getKey() { key = adapterFactoryFor((IBase) value) .createIdentifier((IBase) value) .getValue(); + } else if (engineNativeResourceId != null) { + key = engineNativeResourceId; } else if (value instanceof IBaseResource resource) { key = resource.getIdElement().toVersionless().getValue(); } else { @@ -165,6 +192,7 @@ public String getDescription() { if (wrapper.isEmpty()) { return EMPTY_STRATUM_VALUE; } + var engineNativeResourceId = engineNativeResourceId(value); if (value instanceof IBaseCoding) { ICodingAdapter coding = createCodingAdapter(value); return coding.hasDisplay() ? coding.getDisplay() : coding.getCode(); @@ -181,6 +209,8 @@ public String getDescription() { return adapterFactoryFor((IBase) value) .createIdentifier((IBase) value) .getValue(); + } else if (engineNativeResourceId != null) { + return engineNativeResourceId; } else if (value instanceof IBaseResource resource) { return resource.getIdElement().toVersionless().getValue(); } else { @@ -218,6 +248,7 @@ private String getValueAsString(Object valueInner) { if (wrapper.isEmpty()) { return EMPTY_STRATUM_VALUE; } + var engineNativeResourceId = engineNativeResourceId(valueInner); if (valueInner instanceof IBaseCoding) { return createCodingAdapter(valueInner).getCode(); } else if (isCodeableConcept(valueInner)) { @@ -232,6 +263,8 @@ private String getValueAsString(Object valueInner) { return adapterFactoryFor((IBase) valueInner) .createIdentifier((IBase) valueInner) .getValue(); + } else if (engineNativeResourceId != null) { + return engineNativeResourceId; } else if (valueInner instanceof IBaseResource resource) { return resource.getIdElement().toVersionless().getValue(); } else if (valueInner instanceof Iterable iterable) { @@ -244,6 +277,22 @@ private String getValueAsString(Object valueInner) { } } + /** + * The id of a FHIR resource still in the CQL engine's native form, or null for anything else. + *

+ * This is the bare id part, which is what a resource converted from the same {@link ClassInstance} + * reports from {@code getIdElement()} - the conversion copies {@code id.value} and nothing else, so + * a value rendered here must agree with it or the same resource would land in two strata depending + * on whether it happened to be converted. A resource with no id yields null and falls through to + * the caller's default rendering. + */ + private static String engineNativeResourceId(Object value) { + if (value instanceof ClassInstance classInstance && ClassInstanceHelper.isFhirResource(classInstance)) { + return ClassInstanceHelper.getIdPart(classInstance); + } + return null; + } + private static boolean isCodeableConcept(Object value) { return value instanceof IBase base && "CodeableConcept".equals(base.fhirType()); } diff --git a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/r4/utils/R4MeasureReportUtils.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/r4/utils/R4MeasureReportUtils.java index d373c4357a..59a7997f47 100644 --- a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/r4/utils/R4MeasureReportUtils.java +++ b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/r4/utils/R4MeasureReportUtils.java @@ -1,6 +1,9 @@ package org.opencds.cqf.fhir.cr.measure.r4.utils; +import static org.opencds.cqf.fhir.cql.ClassInstanceHelper.isFhirResource; + import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.context.FhirVersionEnum; import jakarta.annotation.Nullable; import java.util.Objects; import org.hl7.fhir.instance.model.api.IBaseEnumeration; @@ -70,7 +73,11 @@ public static String getStratumDefText(StratifierDef stratifierDef, StratumDef s var cqlFhirParametersConverter = Engines.getCqlFhirParametersConverter(FhirContext.forR4Cached()); Object value; - if (stratumValue.getValueClass().equals(ClassInstance.class)) { + // A resource-valued stratum is rendered by its id below, via getValueAsString(); converting + // it here would rebuild the whole object graph only to fall through every branch that reads + // a converted value. Only the complex datatypes those branches match are worth converting. + if (stratumValue.getValueClass().equals(ClassInstance.class) + && !isFhirResource(FhirVersionEnum.R4, (ClassInstance) stratumValue.getValue())) { value = cqlFhirParametersConverter.toFhirValue((ClassInstance) stratumValue.getValue()); } else { value = stratumValue.getValue(); diff --git a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/CompositeEvaluationResultsPerMeasureTest.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/CompositeEvaluationResultsPerMeasureTest.java index 93e6891ca8..44653698cb 100644 --- a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/CompositeEvaluationResultsPerMeasureTest.java +++ b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/CompositeEvaluationResultsPerMeasureTest.java @@ -9,7 +9,6 @@ import java.time.LocalDate; import java.time.LocalDateTime; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -41,7 +40,7 @@ void gettersContainExpectedData() { // Create a non-empty EvaluationResult without depending on ExpressionResult constructors EvaluationResult er = new EvaluationResult(); - er.set(new EvaluationExpressionRef("subject-123"), new ExpressionResult(null, null)); + er.set(new EvaluationExpressionRef("subject-123"), new ExpressionResult(null, Map.of())); CompositeEvaluationResultsPerMeasure.Builder builder = CompositeEvaluationResultsPerMeasure.builder(); builder.addResult(measureDef1, "subject-123", er, List.of()); @@ -119,7 +118,7 @@ void testToStringWithResultsOnly() { // Create an EvaluationResult with expression results EvaluationResult er = new EvaluationResult(); ExpressionResult expressionResult = new ExpressionResult( - new org.opencds.cqf.cql.engine.runtime.Boolean(true), new HashSet<>(List.of(patient))); + new org.opencds.cqf.cql.engine.runtime.Boolean(true), TestEvaluatedResources.of(patient)); er.set(new EvaluationExpressionRef("Initial Population"), expressionResult); CompositeEvaluationResultsPerMeasure.Builder builder = CompositeEvaluationResultsPerMeasure.builder(); @@ -180,11 +179,11 @@ void testToStringWithResultsAndErrors() { // Create EvaluationResult with multiple expression results EvaluationResult er = new EvaluationResult(); ExpressionResult popResult = new ExpressionResult( - new org.opencds.cqf.cql.engine.runtime.Integer(5), new HashSet<>(List.of(patient, encounter))); + new org.opencds.cqf.cql.engine.runtime.Integer(5), TestEvaluatedResources.of(patient, encounter)); er.set(new EvaluationExpressionRef("Initial Population"), popResult); ExpressionResult numResult = - new ExpressionResult(new org.opencds.cqf.cql.engine.runtime.String("test-string"), Set.of()); + new ExpressionResult(new org.opencds.cqf.cql.engine.runtime.String("test-string"), Map.of()); er.set(new EvaluationExpressionRef("Numerator"), numResult); CompositeEvaluationResultsPerMeasure.Builder builder = CompositeEvaluationResultsPerMeasure.builder(); @@ -229,12 +228,12 @@ void testToStringWithDateValues() { var localDate = modelResolver.toCqlValue(new DateType(LocalDate.of(2024, 1, 15).toString()), false); - ExpressionResult dateResult = new ExpressionResult(localDate, Set.of()); + ExpressionResult dateResult = new ExpressionResult(localDate, Map.of()); er.set(new EvaluationExpressionRef("Date Expression"), dateResult); var localDateTime = modelResolver.toCqlValue( new DateTimeType(LocalDateTime.of(2024, 1, 15, 14, 30, 45).toString()), false); - ExpressionResult dateTimeResult = new ExpressionResult(localDateTime, Set.of()); + ExpressionResult dateTimeResult = new ExpressionResult(localDateTime, Map.of()); er.set(new EvaluationExpressionRef("DateTime Expression"), dateTimeResult); CompositeEvaluationResultsPerMeasure.Builder builder = CompositeEvaluationResultsPerMeasure.builder(); @@ -267,7 +266,7 @@ void testToStringWithCollectionValues() { // Create EvaluationResult with collection value EvaluationResult er = new EvaluationResult(); var patientList = new org.opencds.cqf.cql.engine.runtime.List(List.of(patient1, patient2)); - ExpressionResult listResult = new ExpressionResult(patientList, Set.of()); + ExpressionResult listResult = new ExpressionResult(patientList, Map.of()); er.set(new EvaluationExpressionRef("Patient List"), listResult); CompositeEvaluationResultsPerMeasure.Builder builder = CompositeEvaluationResultsPerMeasure.builder(); @@ -294,7 +293,7 @@ void mergePreservesDebugResult() { EvaluationResult er = new EvaluationResult(); er.set( new EvaluationExpressionRef("expr1"), - new ExpressionResult(new org.opencds.cqf.cql.engine.runtime.Boolean(true), Set.of())); + new ExpressionResult(new org.opencds.cqf.cql.engine.runtime.Boolean(true), Map.of())); var debugResult = new DebugResult(); er.setDebugResult(debugResult); @@ -324,7 +323,7 @@ void mergePreservesTrace() { EvaluationResult er = new EvaluationResult(); er.set( new EvaluationExpressionRef("expr1"), - new ExpressionResult(new org.opencds.cqf.cql.engine.runtime.Boolean(true), Set.of())); + new ExpressionResult(new org.opencds.cqf.cql.engine.runtime.Boolean(true), Map.of())); var trace = new Trace(List.of()); er.setTrace(trace); @@ -348,7 +347,7 @@ void mergeWithNoDebugInfoLeavesFieldsNull() { EvaluationResult er = new EvaluationResult(); er.set( new EvaluationExpressionRef("expr1"), - new ExpressionResult(new org.opencds.cqf.cql.engine.runtime.Boolean(true), Set.of())); + new ExpressionResult(new org.opencds.cqf.cql.engine.runtime.Boolean(true), Map.of())); var builder = CompositeEvaluationResultsPerMeasure.builder(); builder.addResult(measureDef, "patient-1", er, List.of()); diff --git a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/CqlExpressionValueTest.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/CqlExpressionValueTest.java index 43e5a9d2a0..3486010c58 100644 --- a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/CqlExpressionValueTest.java +++ b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/CqlExpressionValueTest.java @@ -50,18 +50,23 @@ void of_nullExpressionResult_returnsEmpty() { void of_expressionResult_propagatesValueAndResources() { var patient = modelResolver.toCqlValue(new Patient().setId("p1"), false); assertNotNull(patient); - Set resources = new HashSet<>(List.of(patient)); + var resources = TestEvaluatedResources.of(patient); ExpressionResult result = new ExpressionResult(patient, resources); CqlExpressionValue wrapper = CqlExpressionValue.ofRaw(null, result, null); assertSame(patient, wrapper.raw()); - assertEquals(resources, wrapper.evaluatedResources()); + assertEquals(Set.copyOf(resources.values()), wrapper.evaluatedResources()); } + /** + * An ExpressionResult can no longer carry null evaluated resources - the engine's map is + * non-null and empty when an expression touched none - so the empty case is what there is to + * cover here. {@code ofRaw} still accepts null from this pipeline's own callers. + */ @Test - void of_expressionResultWithNullResources_substitutesEmptySet() { - ExpressionResult result = new ExpressionResult(new String("v"), null); + void of_expressionResultWithNoResources_yieldsEmptySet() { + ExpressionResult result = new ExpressionResult(new String("v"), Map.of()); CqlExpressionValue wrapper = CqlExpressionValue.ofRaw(null, result, null); @@ -203,7 +208,7 @@ void resolveForPopulation_nullValueReturnsEmpty() { void resolveForPopulation_falseReturnsEmpty() { EvaluationResult evaluationResult = new EvaluationResult(); var patient = modelResolver.toCqlValue(new Patient(), false); - evaluationResult.set(new EvaluationExpressionRef("Patient"), new ExpressionResult(patient, Set.of())); + evaluationResult.set(new EvaluationExpressionRef("Patient"), new ExpressionResult(patient, Map.of())); Iterable result = CqlExpressionValue.ofRaw(null, new Boolean(false), null) .resolveForPopulation("Patient", evaluationResult); @@ -215,7 +220,7 @@ void resolveForPopulation_falseReturnsEmpty() { void resolveForPopulation_trueLooksUpSubjectContextValue() { var patient = modelResolver.toCqlValue(new Patient().setId("p1"), false); var evaluationResult = new EvaluationResult(); - evaluationResult.set(new EvaluationExpressionRef("Patient"), new ExpressionResult(patient, Set.of())); + evaluationResult.set(new EvaluationExpressionRef("Patient"), new ExpressionResult(patient, Map.of())); Iterable result = CqlExpressionValue.ofRaw(null, new Boolean(true), null) .resolveForPopulation("Patient", evaluationResult); diff --git a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForCqlExpressionValuesTest.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForCqlExpressionValuesTest.java new file mode 100644 index 0000000000..0d5ce50497 --- /dev/null +++ b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForCqlExpressionValuesTest.java @@ -0,0 +1,114 @@ +package org.opencds.cqf.fhir.cr.measure.common; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.opencds.cqf.fhir.cr.measure.common.HashSetForFhirResourcesAndCqlTypesTest.encounterInstance; + +import java.util.List; +import org.hl7.fhir.r4.model.Encounter; +import org.junit.jupiter.api.Test; + +/** + * The wrapper-aware set keys on what it wraps, so a wrapper and the raw value inside it are the + * same element. Before that, the wrapper carried no {@code equals}, so {@code add} deduplicated by + * wrapper identity - not at all - while {@code contains} compared the wrapped values. + */ +class HashSetForCqlExpressionValuesTest { + + private static final String ENCOUNTER_ID = "encounter-1"; + private static final String EXPRESSION = "Qualifying Encounters"; + + @Test + void wrappersAroundTheSameResourceAreOneElement() { + var set = new HashSetForCqlExpressionValues(); + + assertTrue(set.add(wrap(encounterInstance(ENCOUNTER_ID)))); + assertFalse(set.add(wrap(encounterInstance(ENCOUNTER_ID)))); + assertEquals(1, set.size()); + } + + @Test + void wrappersAroundDifferentResourcesAreDistinct() { + var set = new HashSetForCqlExpressionValues(); + + assertTrue(set.add(wrap(encounterInstance(ENCOUNTER_ID)))); + assertTrue(set.add(wrap(encounterInstance("encounter-2")))); + assertEquals(2, set.size()); + } + + @Test + void containsAcceptsARawResource() { + var set = new HashSetForCqlExpressionValues(); + set.add(wrap(encounterInstance(ENCOUNTER_ID))); + + assertTrue(set.contains(encounterInstance(ENCOUNTER_ID))); + assertFalse(set.contains(encounterInstance("encounter-2"))); + } + + @Test + void containsAcceptsAHapiResourceForAnEngineNativeElement() { + var set = new HashSetForCqlExpressionValues(); + set.add(wrap(encounterInstance(ENCOUNTER_ID))); + + var hapiEncounter = new Encounter(); + hapiEncounter.setId(ENCOUNTER_ID); + + assertTrue(set.contains(hapiEncounter)); + } + + @Test + void removeAcceptsARawResource() { + var set = new HashSetForCqlExpressionValues(); + set.add(wrap(encounterInstance(ENCOUNTER_ID))); + set.add(wrap(encounterInstance("encounter-2"))); + + assertTrue(set.remove(encounterInstance(ENCOUNTER_ID))); + + assertEquals(1, set.size()); + assertTrue(set.contains(encounterInstance("encounter-2"))); + } + + /** + * {@code PopulationDef.retainAllResources} intersects one population's per-subject resources + * against another's. + */ + @Test + void retainAllIntersectsByWrappedResource() { + var set = new HashSetForCqlExpressionValues(); + set.add(wrap(encounterInstance(ENCOUNTER_ID))); + set.add(wrap(encounterInstance("encounter-2"))); + + set.retainAll(new HashSetForCqlExpressionValues(List.of(wrap(encounterInstance(ENCOUNTER_ID))))); + + assertEquals(1, set.size()); + assertTrue(set.contains(encounterInstance(ENCOUNTER_ID))); + } + + @Test + void retainAllAcceptsAPlainListOfRawResources() { + var set = new HashSetForCqlExpressionValues(); + set.add(wrap(encounterInstance(ENCOUNTER_ID))); + set.add(wrap(encounterInstance("encounter-2"))); + + set.retainAll(List.of(encounterInstance(ENCOUNTER_ID))); + + assertEquals(1, set.size()); + assertTrue(set.contains(encounterInstance(ENCOUNTER_ID))); + } + + @Test + void addThenContainsAgreeForANonResourceValue() { + var set = new HashSetForCqlExpressionValues(); + + assertTrue(set.add(wrap("a plain string"))); + assertTrue(set.contains(wrap("a plain string"))); + assertTrue(set.contains("a plain string")); + assertFalse(set.add(wrap("a plain string"))); + assertEquals(1, set.size()); + } + + private static CqlExpressionValue wrap(Object raw) { + return CqlExpressionValue.ofRaw(EXPRESSION, raw, null); + } +} diff --git a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForFhirResourcesAndCqlTypesTest.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForFhirResourcesAndCqlTypesTest.java index 69ab084578..9043a3a0f4 100644 --- a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForFhirResourcesAndCqlTypesTest.java +++ b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForFhirResourcesAndCqlTypesTest.java @@ -4,20 +4,27 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; +import ca.uhn.fhir.context.FhirVersionEnum; +import java.math.BigDecimal; import java.time.LocalDate; import java.time.Month; import java.util.List; import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.r4.model.Encounter; import org.hl7.fhir.r4.model.Observation; import org.hl7.fhir.r4.model.Patient; import org.junit.jupiter.api.Test; +import org.opencds.cqf.cql.engine.runtime.ClassInstance; import org.opencds.cqf.cql.engine.runtime.Date; +import org.opencds.cqf.cql.engine.runtime.Decimal; import org.opencds.cqf.cql.engine.runtime.Precision; +import org.opencds.cqf.fhir.utility.model.FhirModelResolverCache; class HashSetForFhirResourcesAndCqlTypesTest { public static final String PATIENT_ID_1 = "patient-1"; public static final String PATIENT_ID_2 = "patient-2"; + public static final String ENCOUNTER_ID = "encounter-1"; @Test void addFhirResourceWithSameIdIsNotAddedTwice() { @@ -311,6 +318,152 @@ void removeWithDifferentPrecisionDoesNotThrowNPE() { assertTrue(true, "Remove operation should complete without NPE"); } + // ==================== One relation across every operation ==================== + + /** + * The set holds elements under a single identity, so nothing it accepted can be absent from it. + *

+ * These are the value shapes the measure pipeline puts in these sets. Before the set was keyed, + * {@code add} and {@code contains} reached for different notions of equality and could disagree + * about the same element; the CQL {@code Decimal} case below is one that actually did. + */ + @Test + void addThenContainsAgreeForEveryValueShape() { + var values = List.of( + createPatientWithId(PATIENT_ID_1), + encounterInstance(ENCOUNTER_ID), + new Date(LocalDate.of(2024, Month.JANUARY, 1)), + new Decimal(new BigDecimal("1.0")), + "a plain string"); + + for (Object value : values) { + var set = new HashSetForFhirResourcesAndCqlTypes<>(); + assertTrue(set.add(value), () -> "add returned false for " + value); + assertTrue(set.contains(value), () -> "contains disagreed with add for " + value); + assertFalse(set.add(value), () -> "add accepted a duplicate of " + value); + assertEquals(1, set.size()); + } + } + + /** + * CQL {@code =} says {@code 1.0 = 1.00}; {@code Decimal.equals} is scale-sensitive and says the + * opposite. Whichever the set uses, it has to use it for both operations. + */ + @Test + void addAndContainsAgreeOnCqlDecimalScale() { + var set = new HashSetForFhirResourcesAndCqlTypes(); + var oneDecimalPlace = new Decimal(new BigDecimal("1.0")); + var twoDecimalPlaces = new Decimal(new BigDecimal("1.00")); + + assertTrue(set.add(oneDecimalPlace)); + assertTrue(set.contains(twoDecimalPlaces)); + assertFalse(set.add(twoDecimalPlaces)); + assertEquals(1, set.size()); + } + + // ==================== Engine-native (ClassInstance) resources ==================== + + /** + * The same resource retrieved twice is one element even when the two copies differ in content - + * a differing {@code meta.versionId} here. Structural comparison calls those two resources + * distinct, and CQL {@code =} calls the comparison uncertain, so before id-keying a set of + * evaluated resources could hold the same resource more than once. + */ + @Test + void engineNativeResourcesWithSameIdAreOneElement() { + var set = new HashSetForFhirResourcesAndCqlTypes(); + + assertTrue(set.add(encounterInstance(ENCOUNTER_ID))); + assertFalse(set.add(encounterInstanceWithVersion(ENCOUNTER_ID, "7"))); + assertEquals(1, set.size()); + } + + @Test + void engineNativeResourcesWithDifferentIdsAreDistinct() { + var set = new HashSetForFhirResourcesAndCqlTypes(); + + assertTrue(set.add(encounterInstance(ENCOUNTER_ID))); + assertTrue(set.add(encounterInstance("encounter-2"))); + assertEquals(2, set.size()); + } + + /** + * A resource reaches the pipeline either as a HAPI object or as an engine-native value, and it + * is the same resource in both forms. + */ + @Test + void engineNativeAndHapiFormsOfOneResourceAreOneElement() { + var set = new HashSetForFhirResourcesAndCqlTypes<>(); + var hapiEncounter = new Encounter(); + hapiEncounter.setId(ENCOUNTER_ID); + + assertTrue(set.add(hapiEncounter)); + assertTrue(set.contains(encounterInstance(ENCOUNTER_ID))); + assertFalse(set.add(encounterInstance(ENCOUNTER_ID))); + assertEquals(1, set.size()); + } + + /** + * The population/stratifier intersection at {@code MeasureMultiSubjectEvaluator} retains against + * a plain {@code List}, whose own {@code contains} compares by Java object identity. The + * comparison has to run in this set's relation regardless of what it is handed. + */ + @Test + void retainAllAgainstPlainListOfEngineNativeResources() { + var set = new HashSetForFhirResourcesAndCqlTypes(); + set.add(encounterInstance(ENCOUNTER_ID)); + set.add(encounterInstance("encounter-2")); + + set.retainAll(List.of(encounterInstance(ENCOUNTER_ID))); + + assertEquals(1, set.size()); + assertTrue(set.contains(encounterInstance(ENCOUNTER_ID))); + assertFalse(set.contains(encounterInstance("encounter-2"))); + } + + @Test + void removeAllAgainstPlainListOfEngineNativeResources() { + var set = new HashSetForFhirResourcesAndCqlTypes(); + set.add(encounterInstance(ENCOUNTER_ID)); + set.add(encounterInstance("encounter-2")); + + set.removeAll(List.of(encounterInstance("encounter-2"))); + + assertEquals(1, set.size()); + assertTrue(set.contains(encounterInstance(ENCOUNTER_ID))); + } + + @Test + void iterationOrderFollowsInsertion() { + var set = new HashSetForFhirResourcesAndCqlTypes(); + var patient1 = createPatientWithId(PATIENT_ID_1); + var patient2 = createPatientWithId(PATIENT_ID_2); + set.add(patient1); + set.add(patient2); + + assertEquals(List.of(patient1, patient2), List.copyOf(set)); + } + + static ClassInstance encounterInstance(String id) { + var encounter = new Encounter(); + encounter.setId(id); + encounter.setStatus(Encounter.EncounterStatus.FINISHED); + return toEngineNative(encounter); + } + + private static ClassInstance encounterInstanceWithVersion(String id, String versionId) { + var encounter = new Encounter(); + encounter.setId(id); + encounter.setStatus(Encounter.EncounterStatus.FINISHED); + encounter.getMeta().setVersionId(versionId); + return toEngineNative(encounter); + } + + private static ClassInstance toEngineNative(IBaseResource resource) { + return (ClassInstance) + FhirModelResolverCache.resolverForVersion(FhirVersionEnum.R4).toCqlValue(resource, false); + } + private static Patient createPatientWithId(String id) { var patient = new Patient(); patient.setId(id); diff --git a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/StratumValueWrapperTest.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/StratumValueWrapperTest.java index 0d5063c958..59d7870540 100644 --- a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/StratumValueWrapperTest.java +++ b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/StratumValueWrapperTest.java @@ -3,12 +3,16 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; +import ca.uhn.fhir.context.FhirVersionEnum; import java.util.ArrayList; import java.util.Collections; import java.util.List; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.opencds.cqf.cql.engine.runtime.ClassInstance; import org.opencds.cqf.cql.engine.runtime.Code; +import org.opencds.cqf.fhir.cql.ClassInstanceHelper; +import org.opencds.cqf.fhir.utility.model.FhirModelResolverCache; class StratumValueWrapperTest { @@ -219,6 +223,67 @@ void equalsWithDifferentValues() { } } + // ==================== Engine-native (ClassInstance) Tests ==================== + + /** + * Resource-valued SDE and stratifier results arrive from the CQL engine as {@link ClassInstance} + * and are deliberately left in that form: rebuilding the whole HAPI object graph to read one id + * off it is what made SDE accumulation dominate measure evaluation. These pin the invariant that + * makes that safe — an engine-native resource must render exactly as the same resource converted + * to HAPI FHIR, or SDE grouping silently splits into two strata for the same resource. + */ + @Nested + class EngineNativeResource { + + private static final FhirVersionEnum VERSION = FhirVersionEnum.R4; + + private ClassInstance encounterInstance() { + var encounter = new org.hl7.fhir.r4.model.Encounter(); + encounter.setId("encounter-1"); + encounter.setStatus(org.hl7.fhir.r4.model.Encounter.EncounterStatus.FINISHED); + return (ClassInstance) + FhirModelResolverCache.resolverForVersion(VERSION).toCqlValue(encounter, false); + } + + @Test + void keyMatchesConvertedResource() { + var classInstance = encounterInstance(); + var converted = ClassInstanceHelper.convertToFhirR4(classInstance); + + assertEquals(new StratumValueWrapper(converted).getKey(), new StratumValueWrapper(classInstance).getKey()); + } + + @Test + void valueAsStringMatchesConvertedResource() { + var classInstance = encounterInstance(); + var converted = ClassInstanceHelper.convertToFhirR4(classInstance); + + assertEquals( + new StratumValueWrapper(converted).getValueAsString(), + new StratumValueWrapper(classInstance).getValueAsString()); + } + + @Test + void descriptionMatchesConvertedResource() { + var classInstance = encounterInstance(); + var converted = ClassInstanceHelper.convertToFhirR4(classInstance); + + assertEquals( + new StratumValueWrapper(converted).getDescription(), + new StratumValueWrapper(classInstance).getDescription()); + } + + @Test + void equalInstancesOfTheSameResourceShareAStratum() { + assertEquals(new StratumValueWrapper(encounterInstance()), new StratumValueWrapper(encounterInstance())); + } + + @Test + void keyIsTheResourceId() { + assertEquals("encounter-1", new StratumValueWrapper(encounterInstance()).getKey()); + } + } + // ==================== DSTU3 Tests ==================== @Nested diff --git a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/TestEvaluatedResources.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/TestEvaluatedResources.java new file mode 100644 index 0000000000..7e708d6fbd --- /dev/null +++ b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/TestEvaluatedResources.java @@ -0,0 +1,28 @@ +package org.opencds.cqf.fhir.cr.measure.common; + +import java.util.LinkedHashMap; +import java.util.Map; +import org.opencds.cqf.cql.engine.execution.ExpressionResult; +import org.opencds.cqf.cql.engine.runtime.ClassInstance; +import org.opencds.cqf.cql.engine.runtime.Value; +import org.opencds.cqf.fhir.cql.ClassInstanceHelper; + +/** + * Builds the evaluated-resource map an {@link ExpressionResult} carries. + *

+ * The engine keys these by resource id, so this keys them the same way rather than inventing keys: + * a fake that agrees with the real thing only on the values would hide anything that later starts + * reading the keys. + */ +final class TestEvaluatedResources { + + private TestEvaluatedResources() {} + + static Map of(Value... resources) { + var resourcesById = new LinkedHashMap(); + for (Value resource : resources) { + resourcesById.put(ClassInstanceHelper.getId((ClassInstance) resource), resource); + } + return resourcesById; + } +} diff --git a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSdeTest.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSdeTest.java new file mode 100644 index 0000000000..bed58ef745 --- /dev/null +++ b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSdeTest.java @@ -0,0 +1,79 @@ +package org.opencds.cqf.fhir.cr.measure.r4; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.opencds.cqf.fhir.cr.measure.constant.MeasureConstants.EXT_SDE_REFERENCE_URL; + +import org.hl7.fhir.r4.model.MeasureReport.MeasureReportStatus; +import org.hl7.fhir.r4.model.MedicationDispense; +import org.hl7.fhir.r4.model.MedicationDispense.MedicationDispenseStatus; +import org.junit.jupiter.api.Test; + +/** + * Integration coverage for bound-code elements on a resource-valued supplemental data element, e.g. + * {@code MedicationDispense.status}. + *

+ * HAPI declares a bound code as {@code Enumeration}, but the CQL value carries only the code text. + * A converter that derives the target HAPI type from the CQL value's own type name builds a + * {@code CodeType} for it and then fails on assignment with + * {@code IllegalArgumentException: Can not set org.hl7.fhir.r4.model.Enumeration field + * org.hl7.fhir.r4.model.MedicationDispense.status to org.hl7.fhir.r4.model.CodeType}. Asking the HAPI + * child definition for the type instead yields the {@code Enumeration} along with its + * {@code EnumFactory}, which is what makes the code parse. + * + * @see org.opencds.cqf.fhir.cql.engine.parameters.CqlFhirParametersConverter + */ +@SuppressWarnings("squid:S2699") +class BoundCodeSdeTest { + + private static final Measure.Given GIVEN = Measure.given().repositoryFor("BoundCodeSde"); + + private static final String SDE_ID = "sde-medication-dispense"; + + @Test + void boundCodeConvertsForSupplementalData() { + var then = GIVEN.when() + .measureId("BoundCodeSde") + .subject("Patient/patient-dispense") + .periodStart("2024-01-01") + .periodEnd("2024-12-31") + .reportType("subject") + .evaluate() + .then(); + + then.hasStatus(MeasureReportStatus.COMPLETE) + .hasExtension(EXT_SDE_REFERENCE_URL, 1) + .extensionByValueReference("MedicationDispense/dispense-completed") + .extensionHasSDEId(SDE_ID) + .up() + .report(); + + var dispense = SdeValues.onlySupplementalDataResource(then.def().measureDef(), MedicationDispense.class); + assertEquals("dispense-completed", dispense.getIdElement().getIdPart()); + // The bound code must round-trip as a parsed enumeration, not merely as text. + assertEquals(MedicationDispenseStatus.COMPLETED, dispense.getStatus()); + assertEquals("completed", dispense.getStatusElement().getValueAsString()); + assertEquals( + "1049502", + dispense.getMedicationCodeableConcept().getCodingFirstRep().getCode()); + } + + /** + * Population reports run the same accumulation and rendering over every subject. + */ + @Test + void boundCodeConvertsForSupplementalDataInPopulationReport() { + GIVEN.when() + .measureId("BoundCodeSde") + .periodStart("2024-01-01") + .periodEnd("2024-12-31") + .reportType("population") + .evaluate() + .then() + .hasStatus(MeasureReportStatus.COMPLETE) + .hasExtension(EXT_SDE_REFERENCE_URL, 1) + .extensionByValueReference("MedicationDispense/dispense-completed") + .extensionHasSDEId(SDE_ID) + .up() + .report(); + } +} diff --git a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/NestedBackboneSdeTest.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/NestedBackboneSdeTest.java index 8714275bf2..7ec1383ee6 100644 --- a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/NestedBackboneSdeTest.java +++ b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/NestedBackboneSdeTest.java @@ -2,16 +2,13 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.opencds.cqf.fhir.cr.measure.constant.MeasureConstants.EXT_SDE_REFERENCE_URL; import java.math.BigDecimal; -import java.util.List; import org.hl7.fhir.r4.model.ExplanationOfBenefit; import org.hl7.fhir.r4.model.MeasureReport.MeasureReportStatus; import org.junit.jupiter.api.Test; import org.opencds.cqf.fhir.cr.measure.common.MeasureDef; -import org.opencds.cqf.fhir.cr.measure.common.SdeDef; import org.opencds.cqf.fhir.cr.measure.r4.Measure.Given; /** @@ -128,21 +125,7 @@ void nestedBackboneElementConvertsForSupplementalDataInPopulationReport() { .report(); } - /** - * Pulls the single converted supplemental-data value out of the MeasureDef. The MeasureReport only - * carries a reference to the resource (it is an evaluated resource, so it is not contained), so the - * converted object itself is only reachable through the def. - */ private static ExplanationOfBenefit supplementalDataResource(MeasureDef measureDef) { - List sdes = measureDef.sdes(); - assertEquals(1, sdes.size()); - var values = sdes.get(0).getAccumulatedValues().keySet().stream() - .map(wrapper -> wrapper.getValue()) - .toList(); - assertEquals(1, values.size()); - var value = values.get(0); - assertNotNull(value); - assertEquals(ExplanationOfBenefit.class, value.getClass()); - return (ExplanationOfBenefit) value; + return SdeValues.onlySupplementalDataResource(measureDef, ExplanationOfBenefit.class); } } diff --git a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/R4PopulationBasisValidatorTest.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/R4PopulationBasisValidatorTest.java index 872e64425a..9c62bf2638 100644 --- a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/R4PopulationBasisValidatorTest.java +++ b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/R4PopulationBasisValidatorTest.java @@ -11,7 +11,6 @@ import java.util.Arrays; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.stream.Stream; import org.hl7.fhir.Code; import org.hl7.fhir.r4.model.CodeableConcept; @@ -627,7 +626,7 @@ private static StratifierDef buildStratifierDef(MeasureStratifierType stratifier private static CqlEvaluationResult buildEvaluationResult(Map expressionResultMap) { final EvaluationResult evaluationResult = new EvaluationResult(); expressionResultMap.forEach((key, value) -> - evaluationResult.set(new EvaluationExpressionRef(key), new ExpressionResult(value, Set.of()))); + evaluationResult.set(new EvaluationExpressionRef(key), new ExpressionResult(value, Map.of()))); return new CqlEvaluationResult(evaluationResult); } } diff --git a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/SdeValues.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/SdeValues.java new file mode 100644 index 0000000000..af42f1c2cc --- /dev/null +++ b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/SdeValues.java @@ -0,0 +1,40 @@ +package org.opencds.cqf.fhir.cr.measure.r4; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +import org.opencds.cqf.cql.engine.runtime.ClassInstance; +import org.opencds.cqf.fhir.cql.ClassInstanceHelper; +import org.opencds.cqf.fhir.cr.measure.common.MeasureDef; + +/** + * Reads the converted supplemental-data values out of a {@link MeasureDef}. A MeasureReport only + * carries a reference to an SDE resource (it is an evaluated resource, so it is not contained), so + * the resource itself is only reachable through the def. + *

+ * Accumulation deliberately leaves resource-valued SDEs in their engine-native {@link ClassInstance} + * form, converting only what a report builder renders, so this converts the same way the builders do. + */ +final class SdeValues { + + private SdeValues() {} + + static T onlySupplementalDataResource(MeasureDef measureDef, Class type) { + var sdes = measureDef.sdes(); + assertEquals(1, sdes.size()); + var values = sdes.get(0).getAccumulatedValues().keySet().stream() + .map(wrapper -> toFhir(wrapper.getValue())) + .toList(); + assertEquals(1, values.size()); + var value = values.get(0); + assertNotNull(value); + assertEquals(type, value.getClass()); + return type.cast(value); + } + + private static Object toFhir(Object value) { + return value instanceof ClassInstance classInstance + ? ClassInstanceHelper.convertToFhirR4(classInstance) + : value; + } +} diff --git a/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/cql/BoundCodeSde.cql b/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/cql/BoundCodeSde.cql new file mode 100644 index 0000000000..423fda6a03 --- /dev/null +++ b/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/cql/BoundCodeSde.cql @@ -0,0 +1,19 @@ +library BoundCodeSde + +using FHIR version '4.0.1' + +include FHIRHelpers version '4.0.1' called FHIRHelpers + +context Patient + +define "Initial Population": + true + +/* + Returns whole MedicationDispense resources as supplemental data. MedicationDispense.status is a + bound code: HAPI declares it as Enumeration, while the CQL value carries + only the code text. A converter that picks the target HAPI type from the CQL type name rather than + from the HAPI child definition builds a CodeType for it, which the field will not accept. +*/ +define "SDE Medication Dispense": + [MedicationDispense] diff --git a/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/resources/library/BoundCodeSde.json b/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/resources/library/BoundCodeSde.json new file mode 100644 index 0000000000..c22a793d63 --- /dev/null +++ b/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/resources/library/BoundCodeSde.json @@ -0,0 +1,17 @@ +{ + "resourceType": "Library", + "id": "BoundCodeSde", + "url": "http://example.com/Library/BoundCodeSde", + "name": "BoundCodeSde", + "status": "active", + "type": { + "coding": [ { + "system": "http://terminology.hl7.org/CodeSystem/library-type", + "code": "logic-library" + } ] + }, + "content": [ { + "contentType": "text/cql", + "url": "../../cql/BoundCodeSde.cql" + } ] +} diff --git a/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/resources/measure/BoundCodeSde.json b/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/resources/measure/BoundCodeSde.json new file mode 100644 index 0000000000..dedad064c7 --- /dev/null +++ b/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/resources/measure/BoundCodeSde.json @@ -0,0 +1,66 @@ +{ + "id": "BoundCodeSde", + "resourceType": "Measure", + "name": "BoundCodeSde", + "url": "http://example.com/Measure/BoundCodeSde", + "status": "active", + "library": [ + "http://example.com/Library/BoundCodeSde" + ], + "extension": [ + { + "url": "http://hl7.org/fhir/us/cqfmeasures/StructureDefinition/cqfm-populationBasis", + "valueCode": "boolean" + } + ], + "scoring": { + "coding": [ + { + "system": "http://hl7.org/fhir/measure-scoring", + "code": "cohort" + } + ] + }, + "group": [ + { + "id": "group-1", + "population": [ + { + "id": "initial-population", + "code": { + "coding": [ + { + "system": "http://terminology.hl7.org/CodeSystem/measure-population", + "code": "initial-population", + "display": "Initial Population" + } + ] + }, + "criteria": { + "language": "text/cql-identifier", + "expression": "Initial Population" + } + } + ] + } + ], + "supplementalData": [ + { + "id": "sde-medication-dispense", + "usage": [ + { + "coding": [ + { + "system": "http://terminology.hl7.org/CodeSystem/measure-data-usage", + "code": "supplemental-data" + } + ] + } + ], + "criteria": { + "language": "text/cql.identifier", + "expression": "SDE Medication Dispense" + } + } + ] +} diff --git a/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/tests/medicationdispense/dispense-completed.json b/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/tests/medicationdispense/dispense-completed.json new file mode 100644 index 0000000000..04af893557 --- /dev/null +++ b/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/tests/medicationdispense/dispense-completed.json @@ -0,0 +1,22 @@ +{ + "resourceType": "MedicationDispense", + "id": "dispense-completed", + "status": "completed", + "medicationCodeableConcept": { + "coding": [ + { + "system": "http://www.nlm.nih.gov/research/umls/rxnorm", + "code": "1049502", + "display": "Acetaminophen 300 MG / Oxycodone Hydrochloride 5 MG Oral Tablet" + } + ] + }, + "subject": { + "reference": "Patient/patient-dispense" + }, + "quantity": { + "value": 30, + "unit": "tablet" + }, + "whenHandedOver": "2024-06-11T00:00:00-06:00" +} diff --git a/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/tests/patient/patient-dispense.json b/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/tests/patient/patient-dispense.json new file mode 100644 index 0000000000..a14cc1c94e --- /dev/null +++ b/cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/tests/patient/patient-dispense.json @@ -0,0 +1,6 @@ +{ + "resourceType": "Patient", + "id": "patient-dispense", + "gender": "female", + "birthDate": "1975-04-02" +} diff --git a/local.properties.example b/local.properties.example index 58ad5bf7c8..59fa5ed6cb 100644 --- a/local.properties.example +++ b/local.properties.example @@ -1,5 +1,10 @@ # Local development overrides (copy to local.properties, which is gitignored). # Uncomment and set paths to use local checkouts instead of Maven Central artifacts. +# +# The file's presence also adds mavenLocal() to the build's repositories, so an empty +# local.properties (no paths set) is what you want to resolve a SNAPSHOT you published with +# publishToMavenLocal. Without the file, mavenLocal() is not on the list and the artifact is +# invisible however the version catalog is pinned. -# CQL Engine (clinical_quality_language) - Gradle project root -#cql.engine.path=../clinical_quality_language/Src/java +# CQL Engine (clinical_quality_language) - Gradle project root, which is the repo root +#cql.engine.path=../clinical_quality_language