CDO-495: Merge update-cql-4-7-0 with latest main#1049
Closed
lukedegruchy wants to merge 84 commits into
Closed
Conversation
Introduce CqlExpressionValue, a wrapper around the raw Object returned by ExpressionResult.getValue(), to consolidate the null / Boolean / Iterable / scalar normalization that was previously duplicated across measure-evaluation call sites. Phase 1 adopts the wrapper at three boundaries: - MeasureEvaluator.evaluatePopulationCriteria / evaluateSupportingCriteria - FunctionEvaluationHandler.getResultIterable - PopulationBasisValidator's two-stage null checks The wrapper's internal raw field is intentionally typed as Object so a future migration to the upstream sealed CQL Value type touches only this class, not its callers. CqlExpressionValueException (plain RuntimeException) replaces a HAPI InternalErrorException previously thrown on the subject-context lookup path. Out of scope for Phase 1: collapsing CriteriaResult and StratumValueWrapper, and pushing the wrapper through the Def-class signatures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 2 of the wrapper migration. Adds valueAsSet() and nonNullValues() to CqlExpressionValue (matching the existing CriteriaResult contract, including HashSetForFhirResourcesAndCqlTypes identity semantics for valueAsSet) and switches the per-subject result maps on SdeDef, StratifierDef, and StratifierComponentDef from CriteriaResult to CqlExpressionValue. Updates the only external consumers (MeasureMultiSubjectEvaluator and Dstu3MeasureReportBuilder) to call raw() / valueAsSet() / nonNullValues() on the new wrapper. The CriteriaResult class is now redundant and is removed; its unused NULL_VALUE / EMPTY_RESULT sentinels go with it. The public StratifierDef.getAllCriteriaResultValues() method retains its name to avoid disturbing call sites; only its return type semantics (now backed by CqlExpressionValue) change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed-expression-results-wrapper-class
Extends CqlExpressionValue with isMap() / asMap() and replaces every remaining instanceof Map / Map.class::isInstance / unsafe-cast site that handles MEASUREOBSERVATION accumulators (Map<inputResource, outputValue>) so the lone unchecked cast lives in one tested place. Also fixes a latent bug in MeasureEvaluator.retainObservationResources InPopulation: the previous loop modified the same Set it was iterating over (working only by reference-aliasing accident). Replaced with the collect-then-removeAll pattern. Sites migrated: - MeasureEvaluator: retainObservationSubjectResourcesInPopulation, retainObservationResourcesInPopulation (+ bug fix), removeObservatorySubjectResource - MeasureMultiSubjectEvaluator: collectFunctionRowKeys, mapToListOfTableEntries, stratifierResultAsIntersectionSet, getPopulationResourceKeySet - MeasureReportDefScorer: getResultsForStratumByResourceIds - PopulationDef: countObservations, removeExcludedMeasureObservation Resource PopulationDef.subjectResources field type is unchanged; that's a larger architectural change reserved for a future phase. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A handful of small post-Phase-3a refinements that all match the wrapper-migration theme and get the remaining instanceof Map / raw Object passthroughs out of the way: - MeasureEvaluator.retainObservationResourcesInPopulation: switch from collect-then-removeAll(List) to Set.removeIf, silencing a static analyzer warning about Set.removeAll(List) being O(n*m) in the worst case and lining this method up with its sibling. - MeasureObservationHandler.removeObservationResourcesInPopulation: drop instanceof Map in favour of CqlExpressionValue.asMap(). - MeasureScoreCalculator.collectQuantities: replace the filter(instanceof Map).map(cast) chain with the wrapper's asMap() flatMap. Method signature unchanged. - StratifierUtils.extractClassesFromSingleOrListResult: change the parameter from Object to CqlExpressionValue and dispatch via isNull/isIterable/asIterable. The two callers in PopulationBasisValidator drop their .raw() passthrough. - StratifierDef: delete the private toSet helper and route through CqlExpressionValue.valueAsSet() directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tidies up the satellite call sites that did the same null / Iterable /
Map dispatch as the wrapper but in their own raw form. No behaviour
change; consistent vocabulary across the package.
- EvaluationResultFormatter.formatValue / printValue: wrapper-based
isIterable / asMap dispatch. The {empty} sentinel for empty Maps
is preserved.
- StratumValueWrapper: delete the private isEmptyCollection helper
(it was a near-duplicate of wrapper.isEmpty()) and route its three
callers through the wrapper. Drops the java.util.Collection and
java.util.Map imports along with it.
- R4SupportingEvidenceExtension.classifyValue / collectLeavesInto:
wrapper-based dispatch for the recursive evidence-flattening paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switches MeasureScoreCalculator.collectQuantities from Collection<Object> to Collection<CqlExpressionValue>. The body simplifies to a single stream chain over CqlExpressionValue::asMap, and the only production caller (MeasureReportDefScorer.calculate ContinuousVariableAggregateQuantity) wraps at the boundary using CqlExpressionValue.ofRaw. Tests gain a small wrap(...) helper. The boundary wrap is a transient shim: when PopulationDef.subject Resources eventually moves to typed storage, the wrap goes away (captured in a MIGRATION-NOTE at that boundary). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds MIGRATION-NOTE comments at the major call sites that will need to change when PopulationDef.subjectResources moves from Map<String, Set<Object>> to a typed container (Set<CqlExpressionValue> or a new PopulationResultSet). Each note captures: what changes at that site, the equality-semantics consideration where relevant, and test-focus areas for that flavour of measure. Sites covered: - PopulationDef: the field declaration (central design note) and addResource (single insertion point) - MeasureEvaluator: three observation methods that walk the Set - MeasureMultiSubjectEvaluator: getPopulationResourceKeySet (three branches by population basis) and stratifierResultAsIntersectionSet (load-bearing for Sets.intersection equality) - MeasureObservationHandler: HashSetForFhirResourcesAndCqlTypes copy - R4MeasureReportBuilder / Dstu3MeasureReportBuilder: the FHIR-build consumers - HashSetForFhirResourcesAndCqlTypes: javadoc note on the two options for wrapper identity Searchable via grep "MIGRATION-NOTE (typed-subjectResources)". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Promotes PopulationDef.subjectResources from Map<String, Set<Object>> to Map<String, Set<CqlExpressionValue>>, eliminating the last raw-Object storage in the population-results pipeline. Equality strategy: a new sister type HashSetForCqlExpressionValues mirrors HashSetForFhirResourcesAndCqlTypes but unwraps each element via .raw() before applying FHIR-resource / CQL-type identity rules. The wrapper itself stays generic (no equals/hashCode override), and per-subject sets remain small enough that linear-time identity checks are acceptable. Migrated: - PopulationDef accessors (addResource, getResourcesForSubject, getAllSubjectResources, getSubjectResources, countObservations, removeExcludedMeasureObservationResource) all speak in CqlExpressionValue. addResource is the single wrap point. - MeasureEvaluator's three observation methods (retainObservation SubjectResourcesInPopulation, retainObservationResourcesInPopulation, removeObservatorySubjectResource) take typed parameters and drop per-item ofRaw() wraps and the legacy Set<Object> casts. - MeasureMultiSubjectEvaluator: getPopulationResourceKeySet, the resourceIds builder, and calculateCriteriaStratifierIntersection (now manually intersects via raw() rather than Sets.intersection across mismatched element types). - MeasureReportDefScorer: calculateContinuousVariableAggregateQuantity drops the boundary-wrap shim; getResultsForStratum and getResultsForStratumByResourceIds return wrapped collections. - MeasureObservationHandler: copy uses HashSetForCqlExpressionValues; exclusion lookup unwraps to raw via wrapper.raw() at one site. - R4MeasureReportBuilder + Dstu3MeasureReportBuilder: builder consumers unwrap via .raw() at the FHIR-resource-id boundary. - EvaluationResultFormatter.printSubjectResources: unwraps wrappers before formatting. Tests: PopulationDefTest's helper unwraps wrappers; the assertions that used to call getAllSubjectResources().contains(rawValue) route through the FHIR-identity helper. MeasureObservationHandlerTest switches from direct subjectResources.put() to the public addResource() API everywhere. All MIGRATION-NOTE breadcrumbs left for this phase are now removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After the typed-subjectResources migration, several spots dereferenced .raw() or called wrapper methods without first guarding against null wrappers (which the underlying HashSet permits in principle). This adds defensive null filters / null-checks at the iteration boundaries. Spots tightened: - MeasureEvaluator: removeIf lambdas in retainObservation SubjectResourcesInPopulation, retainObservationResourcesInPopulation, and removeObservatorySubjectResource skip null wrappers; the removeObservatorySubjectResource null-then-isMap check no longer NPEs on a null first element. - MeasureMultiSubjectEvaluator: stream chains over Set<CqlExpression Value> add Objects::nonNull filters before .map(CqlExpressionValue:: raw / asMap), and the criteria-stratifier intersection skips null wrappers / null raw values explicitly. - MeasureObservationHandler: removeMatchingKeysFromObservationMap skips null exclusion wrappers. - MeasureReportDefScorer: getResultsForStratumByResourceIds adds Objects::nonNull after the flatMap before consuming wrappers. - PopulationDef: removeExcludedMeasureObservationResource null-guards the forEach and removeIf elements. - R4MeasureReportBuilder: getPopulationResourceIds and the populationSet filter null-guard the wrapper before calling .raw(); the date-of- compliance pull splits the iterator chain so the wrapper itself can be null-checked separately. - Dstu3MeasureReportBuilder: stratifier groupingBy null-guards the wrapper looked up via Map.get before calling .raw(). - EvaluationResultFormatter: printSubjectResources filters null wrappers before mapping to .raw(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Integrates the latest main (incl. the multi-component-stratifier fixes #1042 and #1045) into the update-cql-4-7-0 CQL-5 refactor. Conflict resolution (MeasureMultiSubjectEvaluator.java): - Re-applied #1042's collectAlignmentRowKeys generalization (function + iterable alignment) using the refactor's typed wrapper API (asFunctionResultAccumulator / isIterable) instead of raw instanceof Map. - Adopted #1045's StratumTableColumnKey column-key type. Completing the incomplete refactor's stratifier value/grouping path so the full cqf-fhir-cr suite passes (was "measures are broken again" on update-cql-4-7-0; ~18 pre-existing MeasureStratifierTest failures): - R4StratifierBuilder: render stratum value via getValueClass/getValueAsString instead of StratumValueWrapper.toString. - StratumValueWrapper: normalize CQL-5 ClassInstance values to FHIR R4. - CqlExpressionValue + MeasureMultiSubjectEvaluator: handle function results arriving as raw Map as well as FunctionResultAccumulator; handle MEASUREOBSERVATION ObservationAccumulator/Map inputs. - StratifierRowValue: normalize CQL-5 ClassInstance FHIR-resource keys. - PopulationBasisValidator: allow CQL runtime primitive value types. cqf-fhir-cr: 1777 tests, 0 failures, 0 errors (18 skipped). CQL v5 SNAPSHOT dependency unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Formatting check succeeded! |
Use org.apache.commons.lang3.StringUtils (the module convention) instead of com.apicatalog.jsonld.StringUtils, which is an internal utility of the JSON-LD library that was only resolvable transitively and broke the build. isNotBlank semantics are identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Same fix as MeasureMultiSubjectEvaluator: use org.apache.commons.lang3.StringUtils instead of com.apicatalog.jsonld.StringUtils (a JSON-LD-internal class that only resolved transitively and broke clean builds). All production StringUtils imports on the branch are now commons-lang3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cqf-fhir-utility (and downstream cqf-fhir-cql / cqf-fhir-cr) use org.apache.commons.lang3 and org.apache.commons.collections4 directly in production code but only received them transitively (via the HAPI/CQL api deps). A clean build off this branch could fail to resolve commons-collections4 on the compile classpath. Declare both as api deps of cqf-fhir-utility so all downstream modules get them reliably. Versions stay BOM-managed (commons-lang3 3.18.0, commons-collections4 4.5.0) — no classpath change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Same class of fix as commons-lang3/collections4: cqf-fhir-utility and downstream modules use com.google.common (Guava) and org.apache.commons.io directly in production but received them only transitively. Declare both as api deps of cqf-fhir-utility. Versions stay BOM-managed (guava 33.2.1-jre, commons-io 2.19.0) — no classpath change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cqf-fhir-cql uses jakarta.xml.bind.JAXB directly (BaseFhirModelInfoProvider) but received the API only transitively. Declare jakarta.xml.bind-api as an implementation dep. Version stays BOM-managed (4.0.1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Superseded by #1051. The upstreams moved ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Integrates the
update-cql-4-7-0measure/CQL-evaluation refactor with the latestmainon a fresh branch (CDO-495). This brings the in-progress CQL-5 refactor up to date with main's tactical fixes and reconciles the divergence between the two code paths.update-cql-4-7-0(78 commits of refactor) withmain(incl. the multi-component-stratifier fixes Fix multi-component stratifier alignment for list + scalar #1042 and Multi-component stratifier loses components that share a scalar value #1045).update-cql-4-7-0already fully contains PR Add and leverage a wrapper class for CQL ExpressionResult raw values. #1020, so no separate reconciliation was needed there.cql = 4.9.0-v5-399e5076a-SNAPSHOT) unchanged.Merge conflict resolution
The single content conflict was in
MeasureMultiSubjectEvaluator.java, where main's #1042/#1045 stratifier fixes intersected code the refactor had rewritten. Resolved by porting main's intent into the refactor's typed-wrapper API:collectFunctionRowKeys→ generalizedcollectAlignmentRowKeys(now aligns scalars against both function-result and iterable components), expressed viaasFunctionResultAccumulator()/isIterable()rather than rawinstanceof Map.StratumTableColumnKey(component, value)column-key type so equal values across components don't collide.The collected alignment row keys are constructed identically to the keys produced by
addFunctionResultRows/addIterableValueRows, so scalar expansion aligns exactly.Completing the refactor's stratifier path
update-cql-4-7-0was an intentionally-incomplete refactor (its tip is "measures are broken again"): ~18MeasureStratifierTestcases failed on it before this merge. The merge itself is regression-free (a baseline-vs-merged diff showed 0 pre-existing tests broken by the merge). To get the module green, this branch finishes the refactor's stratifier value/grouping path:R4StratifierBuilder: render stratum value viagetValueClass()/getValueAsString()instead ofStratumValueWrapper.toString().StratumValueWrapper: normalize CQL-5ClassInstancevalues to FHIR R4.CqlExpressionValue+MeasureMultiSubjectEvaluator: handle function results arriving as a rawMapas well as aFunctionResultAccumulator, and MEASUREOBSERVATIONObservationAccumulator/Map inputs.StratifierRowValue: normalize CQL-5ClassInstanceFHIR-resource keys so they intersect population keys.PopulationBasisValidator: allow CQL runtime primitive value types.Verification
cqf-fhir-cr: 1777 tests, 0 failures, 0 errors (18 skipped). spotlessCheck + checkstyleMain pass.Notes for reviewers (minor, non-blocking)
stratifierResultAsIntersectionSet(CRITERIA path) uses strictasFunctionResultAccumulator()while the VALUE path uses the new dual-shapefunctionResultEntries(). Correct under the current invariant (function-result Maps only arise from NON_SUBJECT_VALUE stratifiers), but the invariant is unasserted and the surrounding Javadoc predates it.PopulationBasisValidatoraccepts CQLruntime.Decimalwithout ajava.langdecimal counterpart in the allowed set — harmless, permissive on the CQL-5 side.Spec ticket: https://simpaticois.atlassian.net/browse/CDO-495
🤖 Prepared by smile-flow (claude-opus-4-8)