-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update source parsing for query filter parsing #280
Conversation
Coverage report
Show files with reduced coverage 🔻
Test suite run success447 tests passing in 31 suites. Report generated by 🧪jest coverage report action from bead5ad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things are looking good so far except for some weirdness with existing QueryFilterParser code. Looks like some expressions that should be in the sources array in gaps.json
are being overwritten. Functionality looks okay with CMS161 and CMS177 but looks like there are issues with CMS142.
Other than that, the data-requirements outputs are the same for these measures and unit, integration, and regression tests pass 👍
src/gaps/QueryFilterParser.ts
Outdated
@@ -247,6 +274,17 @@ function parseDataType(retrieve: ELMRetrieve): string { | |||
return retrieve.dataType.replace(/^(\{http:\/\/hl7.org\/fhir\})?/, ''); | |||
} | |||
|
|||
/** | |||
* Pulls out the resource type of a result type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a bit more info to this? Does "Pulls out the resource type from a resultTypeSpecifier on an expression"? Or something like that? Like maybe include expression there somewhere
src/gaps/QueryFilterParser.ts
Outdated
} | ||
}); | ||
// parse relationship clauses | ||
query.relationship.forEach(relationship => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little confused about the addition of this forEach
...it is parsing through the relationship array rather than the source array so I think either we can change the name of this function to incorporate that somehow or perhaps put this in its own parseRelationship
function? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely need to parse the 'relationship`. But will need to parse it when it is something other than a retrieve, similar to above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments that may bring further discussion. This also does not deconflict the sources to their filters when building dataRequirements and will still result in the wrong filters being added to the wrong data types, ex: the Procedure.period
issue.
src/gaps/QueryFilterParser.ts
Outdated
} | ||
}); | ||
// parse relationship clauses | ||
query.relationship.forEach(relationship => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely need to parse the 'relationship`. But will need to parse it when it is something other than a retrieve, similar to above.
src/gaps/QueryFilterParser.ts
Outdated
* @returns FHIR ResourceType name. | ||
*/ | ||
function parseElementType(expression: ELMExpression): string { | ||
const elementType = (expression.resultTypeSpecifier as ListTypeSpecifier).elementType as NamedTypeSpecifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this isn't a ListTypeSpecifier of NamedTypeSpecifier or a NamedTypeSpecifier then perhaps we should gracefully not return anything. In the case it isn't or doesn't exist this could error out.
b0c964d
to
7556acc
Compare
Summary of changes made in newest commits:
Changes to the output: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good updates! It looks like the sources
array is now getting properly populated with everything and not getting overwritten!
The one thing I am confused about and I am not sure if it was intentional or not based on your summary of the changes is that the dataRequirements output.json
now no longer contains dateFilter
on one of the data requirements (seen for CMS177, CMS161, and CMS142).
Other than that, unit tests, integration tests, and regression still pass!
src/gaps/GapsReportBuilder.ts
Outdated
if (cf !== null) { | ||
dataRequirement.codeFilter?.push(cf); | ||
} | ||
} else if (df.type === 'during') { | ||
const dateFilter = generateDetailedDateFilter(df as DuringFilter); | ||
if (dataRequirement.dateFilter) { | ||
dataRequirement.dateFilter.push(dateFilter); | ||
} else { | ||
dataRequirement.dateFilter = [dateFilter]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this code was already there but I am a little confused by it. So for the first if
statement, are we only pushing cf
to dataRequirement.codeFilter
if dataRequirement.codeFilter
already exists? If not, then can we consolidate the code in the else if
to do the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like in DataRequirementHelpers.ts
this function gets called after calling generateDataRequirement
on the retrieve. The generateDataRequirement
function already creates the codeFilter
array if retrieve.valueSet
or retrieve.code
exists. So it seems that in most cases dataRequirement.codeFilter
will already exist by the time that we get here, unless the retrieve does not have a code or valueSet. In that case, it seems that the cf
here would not get added since codeFilter
does not exist. Perhaps this is the expected behavior? @hossenlopp does this reasoning seem right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this reasoning sounds good. It is possible that they may have not used a filter on the retrieve itself and may be filtering in the where
part of the query. So codeFilter
could definitely use similar logic to the dateFilter
to see if the list is there first, and create it if it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good and collects the source information correctly now. Just that one suggestion on the codeFilter
.
While it would be nice to have this code more unit tested, I think it would be better to revisit unit testing when we re-work dataRequirements.
src/gaps/GapsReportBuilder.ts
Outdated
if (cf !== null) { | ||
dataRequirement.codeFilter?.push(cf); | ||
} | ||
} else if (df.type === 'during') { | ||
const dateFilter = generateDetailedDateFilter(df as DuringFilter); | ||
if (dataRequirement.dateFilter) { | ||
dataRequirement.dateFilter.push(dateFilter); | ||
} else { | ||
dataRequirement.dateFilter = [dateFilter]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this reasoning sounds good. It is possible that they may have not used a filter on the retrieve itself and may be filtering in the where
part of the query. So codeFilter
could definitely use similar logic to the dateFilter
to see if the list is there first, and create it if it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
Summary
For measures authored using QI-Core, the
sources
array is unpopulated due to unhandled scenarios related to parsing query sources. UpdatesparseSources
to drill into expressions on a given query that are not immediate retrieves.New behavior
The
sources
arrays on the query info should now be populated appropriately. The approach to source parsing is as follows:resultTypeSpecifier
. For the QI-Core-authored measures, thesource
is of the following format (for an example of an Encounter datatype and “Union” type):Since there is no
datatype
on the expression, the “elementType” is parsed to retrieve the associated datatype.A potential side effect of the current query filter parsing implementation is that incorrect attribute paths are assigned to resource types during data requirements calculation. Note that this issue is not directly addressed in this pull request, as there are other issues involved, such as the “query stacking” approach that is implemented and how it handles the “Union” type that is seen in the ELM of QI-Core measures.
More information about the ELM structure is available at https://cql.hl7.org/04-logicalspecification.html.
Code changes
QueryFilterParser
parseSources
function to now extract sources from theresultTypeSpecifier
if present (in the case that the type is not a Retrieve)parseElementType
to parse theelementType
on the expression (this is an alternative to parsing thedatatype
)ELMTypes
resultTypeSpecifier
as an option field on an ELM expressionTesting guidance
It is important to test against both QI-Core and non-QI-Core measures.
Some potential steps that can be taken for testing:
npm run test
,npm run test:integration
, etc. to ensure that no side effects were introduced. Pay specific attention to the query filter parsing unit testsdebug
option enabled.debug/gaps.json
, pay specific attention to thequeryInfo
output on the retrieves. Previously thesources
array would be empty for some of the QI-Core measures (CMS161 is an example). Check that this is no longer the case, and that thesources
array is appropriately populated by looking back at the ELM output for the measure being tested.gaps.json
output has not changed for non-QI-Core measures, except in the case that therelationships
array sources are not encompassed in the sources. The data requirements output is not expected to change. This can be done by generating the output on this branch and on the main branch, and using the ‘Select for Compare’ feature in VSCode.Open questions to check for when testing
relationships
expressions alongside the source expressions? According to the logical specification, the relationship clauses “allow related sources to restrict the elements included from another source in a query scope…The elements of the related source are not included in the query scope.”resultTypeSpecifier
? The cql-execution library contains types forTypeSpecifier
s, but little (if any) handling is done in fqm-execution for parsing theTypeSpecifier
s.However, the original query info does not contain the Procedure and instead contains an Encounter:
When we set
queryInfo.sources = innerQueryInfo.sources
, we lose this Procedure source.