Ld 20260901 sde lazy conversion - #1095
Draft
lukedegruchy wants to merge 6 commits into
Draft
lukedegruchy wants to merge 6 commits into
lukedegruchy wants to merge 6 commits into
Conversation
BoundCodeSdeTest evaluates a measure whose supplemental data returns whole MedicationDispense resources. HAPI declares MedicationDispense.status as Enumeration<MedicationDispenseStatus>, 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
|
Formatting check succeeded! |
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 <noreply@anthropic.com>
|
The CQL engine now hands back ExpressionResult.evaluatedResources as a non-null Map<String, Value> keyed by resource id, where it was a Set<Value>. 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 <noreply@anthropic.com>
This branch has not been deployed
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
1 participant
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.
No description provided.