From 177131a83f1736bc9c64db94799eb9299a645962 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Tue, 1 Sep 2026 17:27:18 -0400 Subject: [PATCH 1/6] Add failing coverage for bound-code SDE conversion BoundCodeSdeTest evaluates a measure whose supplemental data returns whole MedicationDispense resources. HAPI declares MedicationDispense.status as Enumeration, but CqlFhirParametersConverter derives the target type from the CQL value's own type name and builds a CodeType, so the assignment fails: 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 Both tests fail on the version catalog's CQL 5.2.0. They pass on the 5.3.0-fix-model-resolver-overrides snapshot, which resolves the type upstream; the converter still guesses, so the class of failure remains. StratumValueWrapperTest gains the invariant that deferring conversion depends on: an engine-native ClassInstance resource must produce the same key, value-as-string and description as the same resource converted to HAPI FHIR. These pass before and after; they are the guard against SDE grouping splitting one resource across two strata. Co-Authored-By: Claude Opus 5 --- .../common/StratumValueWrapperTest.java | 67 ++++++++++++++++ .../fhir/cr/measure/r4/BoundCodeSdeTest.java | 79 +++++++++++++++++++ .../cqf/fhir/cr/measure/r4/SdeValues.java | 40 ++++++++++ .../BoundCodeSde/input/cql/BoundCodeSde.cql | 19 +++++ .../input/resources/library/BoundCodeSde.json | 17 ++++ .../input/resources/measure/BoundCodeSde.json | 66 ++++++++++++++++ .../dispense-completed.json | 22 ++++++ .../input/tests/patient/patient-dispense.json | 6 ++ 8 files changed, 316 insertions(+) create mode 100644 cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSdeTest.java create mode 100644 cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/SdeValues.java create mode 100644 cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/cql/BoundCodeSde.cql create mode 100644 cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/resources/library/BoundCodeSde.json create mode 100644 cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/resources/measure/BoundCodeSde.json create mode 100644 cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/tests/medicationdispense/dispense-completed.json create mode 100644 cqf-fhir-cr/src/test/resources/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSde/input/tests/patient/patient-dispense.json 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 0d5063c95..94a5bdbbb 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,69 @@ 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/r4/BoundCodeSdeTest.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/BoundCodeSdeTest.java new file mode 100644 index 000000000..bed58ef74 --- /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/SdeValues.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/SdeValues.java new file mode 100644 index 000000000..af42f1c2c --- /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 000000000..423fda6a0 --- /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 000000000..c22a793d6 --- /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 000000000..dedad064c --- /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 000000000..04af89355 --- /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 000000000..a14cc1c94 --- /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" +} From 4e56afd8645ef5c3f2c8fcc489146d3d365b9ccd Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Tue, 1 Sep 2026 17:33:08 -0400 Subject: [PATCH 2/6] Stop rebuilding FHIR resources to read their id during SDE accumulation SdeDef.accumulate() wraps every SDE value in a StratumValueWrapper, whose constructor converted engine-native ClassInstance values into fully materialized HAPI FHIR object graphs - reflectively, field by field - and then used only the resource's id as a grouping key. The graph was discarded immediately. On an ExplanationOfBenefit that is dozens of nested backbone elements rebuilt per occurrence per subject; on a 400-member HEDIS cohort with 38 resource-valued SDEs it dominated the whole evaluation. Leave resources engine-native and key them off the ClassInstance, which already carries the id. Complex datatypes (Coding, CodeableConcept, Identifier) still convert eagerly: the rendering below reads their contents, and they are a handful of primitives rather than a graph. getKey(), getDescription() and getValueAsString() gain the matching branch, so a raw ClassInstance no longer falls through to toString(). The key is the bare id part, ClassInstanceHelper.getIdPart, not the type-qualified getId: converting a ClassInstance copies id.value and nothing else, so a converted resource reports the bare id from getIdElement(). Using the qualified form would have changed every rendered stratum value and split a resource across two strata depending on whether it happened to be converted. StratumValueWrapperTest pins that equivalence. Also memoise getKey(). hashCode() calls it once per element and equals() twice per comparison, so Collectors.groupingBy walked the eight-branch rendering chain repeatedly for a value that cannot change after construction. ClassInstanceHelper.isFhirResource gains a version-agnostic overload for callers, like StratumValueWrapper, that hold no FHIR version of their own. Co-Authored-By: Claude Opus 5 --- .../cqf/fhir/cql/ClassInstanceHelper.kt | 46 ++++++++++++++++- .../measure/common/StratumValueWrapper.java | 51 ++++++++++++++++++- .../r4/utils/R4MeasureReportUtils.java | 9 +++- .../common/StratumValueWrapperTest.java | 4 +- .../cr/measure/r4/NestedBackboneSdeTest.java | 19 +------ 5 files changed, 104 insertions(+), 25 deletions(-) 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 f41e06e59..64027c3fb 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-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 0fdd6a0b5..cbcbeb71e 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 d373c4357..59a7997f4 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/StratumValueWrapperTest.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/StratumValueWrapperTest.java index 94a5bdbbb..59d787054 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 @@ -250,9 +250,7 @@ void keyMatchesConvertedResource() { var classInstance = encounterInstance(); var converted = ClassInstanceHelper.convertToFhirR4(classInstance); - assertEquals( - new StratumValueWrapper(converted).getKey(), - new StratumValueWrapper(classInstance).getKey()); + assertEquals(new StratumValueWrapper(converted).getKey(), new StratumValueWrapper(classInstance).getKey()); } @Test 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 8714275bf..7ec1383ee 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); } } From a95a05efa8f0b0b93e1fc8a1d375dca72babd2af Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Tue, 1 Sep 2026 17:39:49 -0400 Subject: [PATCH 3/6] Resolve FHIR types from HAPI child definitions, not CQL type names toFhirValue picked the target HAPI class by resolving the CQL value's own type name and then repairing the guess with a string replacement over the class name. The guess failed in two ways: IllegalArgumentException: Could not resolve inner FHIR type: AdjudicationComponent 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 HAPI already knows the answer at the recursion site: the loop holds the child definition for the element it is about to populate. Pass that down instead of a parent-name string and take the type from it, falling back to the CQL type name only at the top level, where there is no enclosing element. This subsumes both failures. A backbone element's inner class comes from the child definition, so the parent-name heuristic is deleted rather than extended. A bound code reports Enumeration, and instantiating through HAPI hands it the EnumFactory that lets the existing IBaseEnumeration branch parse the value. Choice elements ([x]) still resolve through the CQL type name, since that is what picks between the types a choice accepts, but the class comes from HAPI's definition for that datatype. Contained resources and Narrative.div are left on the type-name path, which handles them no worse than before. BoundCodeSdeTest now passes on CQL 5.2.0. Co-Authored-By: Claude Opus 5 --- .../parameters/CqlFhirParametersConverter.kt | 143 +++++++++++------- 1 file changed, 89 insertions(+), 54 deletions(-) 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 5f1a57245..3253f0ac5 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" From 0d35a5825488ed8f354ca0a7d6fed2d57e8d6816 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Tue, 1 Sep 2026 18:39:52 -0400 Subject: [PATCH 4/6] Add PRP for SDE/stratum value lazy conversion The proposal this branch implements. Two of its claims did not survive implementation and are worth recording against it: - ClassInstanceHelper.getId is not byte-identical to the key the current code produces. It returns "Type/id"; converting a ClassInstance copies id.value and nothing else, so a converted resource reports the bare id. The implementation uses a new getIdPart to keep rendered output unchanged. - The MedicationDispense.status crash reproduces on the version catalog's CQL 5.2.0, but not on the 5.3.0-fix-model-resolver-overrides snapshot, which resolves the type upstream. Part 3 still stops the converter guessing. DSTU3 coverage was dropped from the testing strategy; R4 is the target. Co-Authored-By: Claude Opus 5 --- PRPs/prp-sde-stratum-value-lazy-conversion.md | 387 ++++++++++++++++++ 1 file changed, 387 insertions(+) create mode 100644 PRPs/prp-sde-stratum-value-lazy-conversion.md 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 000000000..669efdf93 --- /dev/null +++ b/PRPs/prp-sde-stratum-value-lazy-conversion.md @@ -0,0 +1,387 @@ +# 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**: Proposed +- **Priority**: High (Performance regression + latent correctness defect) +- **Estimated Effort**: 1 day (Parts 1–2), 2–3 days (Part 3) +- **Target Branch**: TBD — branches from `ld-20260901-anton-fix-fhir-model-overrides` (`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)` already returns exactly the required key format.** + It reads `type.localPart` + `id.value` off the `ClassInstance` and returns `"Type/id"` — + byte-identical to what `resource.getIdElement().toVersionless().getValue()` produces, with no + conversion. + +2. **`ClassInstanceHelper.isFhirResource(FhirVersionEnum, ClassInstance)` already exists** as the + predicate distinguishing resources (huge, id-keyed) from complex datatypes (small, value-keyed). + +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. + +### 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. + +## 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 + +Expected effect on the `AAB-Details` 400-member benchmark, per member: + +| | current | expected | +|---|---|---| +| SDE accumulation (`cql_eval` delta) | 2 435 ms | ≈ 0 — an id read replaces a graph rebuild | +| SDE rendering (`report_build` delta) | 176 ms | unchanged for individual reports; population reports emit references, not graphs | +| **total SDE cost** | **2 612 ms** | **target < 100 ms** (CR 4.8.0 measured 62 ms) | + +Projected run wall-clock: ~6 minutes → under 1 minute, the remainder being the separate ~2.7x CQL +hot-path regression between 4.8 and 4.11 that this PRP does **not** address (visible on +`AAB-Reporting`, which has no SDEs: 118 → 315 ms per member steady-state). That is a distinct +investigation. + +Memory: proportional reduction in transient allocation during accumulation, which is what should +restore parallelism under Spark from 3.65x toward the 7.7–9.2x that `AAB-Reporting` sustains. + +## 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. +- **New**: an SDE returning `MedicationDispense` resources, asserting the bound-code `status` field + round-trips. This is red before Part 3 and green after. +- **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 `getId`'s format ever + drifted from `getIdElement().toVersionless().getValue()`. +- **DSTU3**: `Dstu3MeasureReportBuilder` has **no** `ClassInstance` branch — it calls + `getValueAsString()` / `getKey()` directly. Part 1's additions to those two methods are what keep + it working; add a DSTU3 SDE test covering a resource-valued SDE explicitly. + +### 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 + +- [ ] Part 1: skip eager conversion for `ClassInstance` resources in `normalizeEngineNativeValue` +- [ ] Part 1: add `ClassInstance` branches to `getKey()`, `getDescription()`, `getValueAsString()` +- [ ] Part 1: handle `getId()` returning null (fall back, do not propagate) +- [ ] Part 1: thread `FhirVersionEnum` into `isFhirResource` rather than hard-coding R4 further +- [ ] Part 2: memoise `getKey()` +- [ ] Part 3: pass child definitions through `toFhirValue` recursion; derive target class from them +- [ ] Part 3: remove the `parentName` string-replacement heuristic +- [ ] Tests: `MedicationDispense.status` bound-code SDE +- [ ] Tests: `ClassInstance` vs `IBaseResource` key equivalence +- [ ] Tests: DSTU3 resource-valued SDE +- [ ] Benchmark: `AAB-Details` minus `AAB-Reporting` delta before/after + +## Success Criteria + +### Functional +- `NestedBackboneSdeTest` and the existing measure test suite pass unchanged. +- `AAB-Details` completes over 400 members with **zero** per-member evaluation failures on + CR 4.11.1+ content (currently 134 of 400 fail). +- SDE observation output is byte-identical to current output where current output succeeds. + +### Non-functional +- SDE cost per member on `AAB-Details` under 100 ms (from 2 612 ms). +- Effective parallelism on `AAB-Details` within 20% of `AAB-Reporting` on the same host. + +### Code quality +- The `parentName` heuristic is deleted, not extended. +- No new version-specific hard-coding in the version-agnostic `common` package. + +## 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. From 20087354d15bc6e1786a667f7190d1d4829f9e76 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Wed, 2 Sep 2026 09:58:44 -0400 Subject: [PATCH 5/6] Key the measure sets on resource identity instead of comparing pairwise HashSetForFhirResourcesAndCqlTypes and HashSetForCqlExpressionValues predate the CQL 5 migration and had stopped doing what they were written to do. Both compared elements pairwise, which left them with two relations at once: add and remove routed CQL values into HashSet's own equality while contains and retainAll routed them into CQL `=`. Under CQL 5 every expression result is a ClassInstance, so that split ran on every value, and a set could fail to contain something it had just accepted. Every operation was also a linear scan in which each comparison walked a resource graph, making set construction O(n^2) in deep structural comparisons. Store elements under an IdentityKey instead. A FHIR resource keys on (resource type, logical id) whether it arrives as a HAPI object or as a ClassInstance; other CQL values key on CQL `=` in one shared bucket, since CQL `=` is not hash-compatible and there is no key that spreads them; everything else keys on its own equals. One relation then answers add, contains, remove and retainAll by construction, and the resource case - the one that matters - becomes a string comparison. Measured while implementing, both now regression tests: Decimal("1.0") vs Decimal("1.00") equals false, CQL `=` true same resource, differing versionId equals false, CQL `=` null (uncertain) The second is the important one: both relations called one resource two, so a set of evaluated resources could hold it twice regardless of which ran. Only id-keying collapses that. retainAll and removeAll are overridden because the inherited implementations ask the other collection what it contains, and a plain List answers by Java object identity - which is how a population intersection silently drops resources. HashSetForCqlExpressionValues collapses into a keyFor override that unwraps the wrapper, 140 lines to 57. castToResourceIfApplicable and castToCqlTypeIfApplicable are deleted rather than extended past Date, since keying leaves them without callers. Two deliberate behaviour changes, not pure speedups: keying on the logical id makes Patient/1 and Patient/1/_history/2 one element, and unifies the HAPI and engine-native forms of a resource. Id-less resources keep the relation each form already had rather than falling back to identity. The PRP is updated alongside, for Part 4 and for the Parts 1-3 claims the implementation disproved: getId is not byte-identical to the key it replaced, and the bound-code crash was already fixed upstream in the CQL snapshot the benchmark ran against, so the 400-of-400 result is not evidence for Parts 1-2. Co-Authored-By: Claude Opus 5 --- PRPs/prp-sde-stratum-value-lazy-conversion.md | 368 +++++++++++++++--- .../common/FhirResourceAndCqlTypeUtils.java | 89 +++-- .../common/HashSetForCqlExpressionValues.java | 141 +------ .../HashSetForFhirResourcesAndCqlTypes.java | 196 ++++------ .../fhir/cr/measure/common/IdentityKey.java | 70 ++++ .../HashSetForCqlExpressionValuesTest.java | 114 ++++++ ...ashSetForFhirResourcesAndCqlTypesTest.java | 153 ++++++++ 7 files changed, 807 insertions(+), 324 deletions(-) create mode 100644 cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/IdentityKey.java create mode 100644 cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForCqlExpressionValuesTest.java diff --git a/PRPs/prp-sde-stratum-value-lazy-conversion.md b/PRPs/prp-sde-stratum-value-lazy-conversion.md index 669efdf93..a55860a69 100644 --- a/PRPs/prp-sde-stratum-value-lazy-conversion.md +++ b/PRPs/prp-sde-stratum-value-lazy-conversion.md @@ -2,10 +2,10 @@ ## Metadata - **Title**: Stop reflectively rebuilding whole FHIR resources in `StratumValueWrapper` to read their id -- **Status**: Proposed +- **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), 2–3 days (Part 3) -- **Target Branch**: TBD — branches from `ld-20260901-anton-fix-fhir-model-overrides` (`3a82da9c`) +- **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 @@ -125,13 +125,23 @@ rendered. Three facts make this nearly free to implement, because the pieces already exist: -1. **`ClassInstanceHelper.getId(ClassInstance)` already returns exactly the required key format.** - It reads `type.localPart` + `id.value` off the `ClassInstance` and returns `"Type/id"` — - byte-identical to what `resource.getIdElement().toVersionless().getValue()` produces, with no - conversion. +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: @@ -209,6 +219,16 @@ than propagating null. 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 @@ -277,6 +297,174 @@ This subsumes both known failures: 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`? @@ -309,36 +497,83 @@ measured win is SDE-side, but resource-valued stratifiers exist and would regres ## Performance Analysis -Expected effect on the `AAB-Details` 400-member benchmark, per member: - -| | current | expected | -|---|---|---| -| SDE accumulation (`cql_eval` delta) | 2 435 ms | ≈ 0 — an id read replaces a graph rebuild | -| SDE rendering (`report_build` delta) | 176 ms | unchanged for individual reports; population reports emit references, not graphs | -| **total SDE cost** | **2 612 ms** | **target < 100 ms** (CR 4.8.0 measured 62 ms) | - -Projected run wall-clock: ~6 minutes → under 1 minute, the remainder being the separate ~2.7x CQL -hot-path regression between 4.8 and 4.11 that this PRP does **not** address (visible on -`AAB-Reporting`, which has no SDEs: 118 → 315 ms per member steady-state). That is a distinct -investigation. - -Memory: proportional reduction in transient allocation during accumulation, which is what should -restore parallelism under Spark from 3.65x toward the 7.7–9.2x that `AAB-Reporting` sustains. +> **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. + 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. + 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 `getId`'s format ever - drifted from `getIdElement().toVersionless().getValue()`. + 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; add a DSTU3 SDE test covering a resource-valued SDE explicitly. + 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 @@ -349,33 +584,58 @@ restore parallelism under Spark from 3.65x toward the 7.7–9.2x that `AAB-Repor ## Implementation Checklist -- [ ] Part 1: skip eager conversion for `ClassInstance` resources in `normalizeEngineNativeValue` -- [ ] Part 1: add `ClassInstance` branches to `getKey()`, `getDescription()`, `getValueAsString()` -- [ ] Part 1: handle `getId()` returning null (fall back, do not propagate) -- [ ] Part 1: thread `FhirVersionEnum` into `isFhirResource` rather than hard-coding R4 further -- [ ] Part 2: memoise `getKey()` -- [ ] Part 3: pass child definitions through `toFhirValue` recursion; derive target class from them -- [ ] Part 3: remove the `parentName` string-replacement heuristic -- [ ] Tests: `MedicationDispense.status` bound-code SDE -- [ ] Tests: `ClassInstance` vs `IBaseResource` key equivalence -- [ ] Tests: DSTU3 resource-valued SDE -- [ ] Benchmark: `AAB-Details` minus `AAB-Reporting` delta before/after +> 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 unchanged. -- `AAB-Details` completes over 400 members with **zero** per-member evaluation failures on - CR 4.11.1+ content (currently 134 of 400 fail). -- SDE observation output is byte-identical to current output where current output succeeds. +- ✅ `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` under 100 ms (from 2 612 ms). -- Effective parallelism on `AAB-Details` within 20% of `AAB-Reporting` on the same host. +- ✅ 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 `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 @@ -385,3 +645,19 @@ read an id that the source object already carried. Every component needed to fix `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-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 556ea8929..bd033d63a 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/HashSetForCqlExpressionValues.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/HashSetForCqlExpressionValues.java index da01c08e0..74f41f66a 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 fbd76665c..674ef62f7 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 000000000..ee7608556 --- /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/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 000000000..0d5ce5049 --- /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 69ab08457..9043a3a0f 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); From 82c93a46ad19a505a9495bdcfe5b1b6185b0fac6 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Thu, 3 Sep 2026 11:27:56 -0400 Subject: [PATCH 6/6] Take the engine's evaluated resources as a map keyed by id The CQL engine now hands back ExpressionResult.evaluatedResources as a non-null Map keyed by resource id, where it was a Set. It was changed because hashing those resources into a set meant walking each one's element graph, which dominated evaluation. Adapt at the boundary rather than re-typing the pipeline: the Set shape runs through CqlExpressionValue, SdeDef, PopulationDef, StratifierDef, both report builders and the formatter, and converting all of it is a refactor rather than a compile fix. The keys are dropped and the values kept. The values land in HashSetForFhirResourcesAndCqlTypes, never a plain HashSet. That set keys on (resource type, logical id), so the resources stay cheap to hash and the change does not hand back the cost the engine's keying exists to remove. SdeDef.allEvaluatedResources moves off HashSet for the same reason: it accumulates across every subject. Two pieces of now-unreachable code go with it, both guarding against a null the Kotlin type no longer permits: a null check in CqlExpressionValue.of, and in FunctionEvaluationHandler an IllegalStateException followed by adding the same collection twice. Tests construct ExpressionResult directly, so they move to Map.of() for the empty case and to a new TestEvaluatedResources helper where resources are supplied - keyed by real resource id, since a fake that agrees with the engine only on the values would hide anything that later reads the keys. One test covered null evaluated resources, which the engine can no longer produce; it now covers the empty map instead. local.properties.example is corrected alongside: the CQL engine's Gradle root moved to its repo root, so the sample Src/java path no longer resolves. It also now records that the file's presence is what puts mavenLocal() on the repository list, which is not obvious from the build and costs time to find. Co-Authored-By: Claude Opus 5 --- .../CqlFhirParametersConverterTests.java | 18 +++++++----- .../cr/measure/common/CqlExpressionValue.java | 11 ++++++-- .../common/FunctionEvaluationHandler.java | 15 +++------- .../cqf/fhir/cr/measure/common/SdeDef.java | 5 ++-- ...positeEvaluationResultsPerMeasureTest.java | 21 +++++++------- .../common/CqlExpressionValueTest.java | 17 +++++++---- .../common/TestEvaluatedResources.java | 28 +++++++++++++++++++ .../r4/R4PopulationBasisValidatorTest.java | 3 +- local.properties.example | 9 ++++-- 9 files changed, 84 insertions(+), 43 deletions(-) create mode 100644 cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/TestEvaluatedResources.java 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 a5e0f286f..c75862456 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 42e4420e6..fbc2f5691 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/FunctionEvaluationHandler.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/FunctionEvaluationHandler.java index 6a9c58a11..bf1867376 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/SdeDef.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/measure/common/SdeDef.java index 5c4604312..c9a7e9eec 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/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 93e6891ca..44653698c 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 43e5a9d2a..3486010c5 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/TestEvaluatedResources.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/common/TestEvaluatedResources.java new file mode 100644 index 000000000..7e708d6fb --- /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/R4PopulationBasisValidatorTest.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/measure/r4/R4PopulationBasisValidatorTest.java index 872e64425..9c62bf263 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/local.properties.example b/local.properties.example index 58ad5bf7c..59fa5ed6c 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