Skip to content

Commit

Permalink
Br fix datarequirements (#1256)
Browse files Browse the repository at this point in the history 
* #1203: Fixed function references missing from logic definitions in data-requirements

* #1202: Fixed extension access reporting as a code filter instead of a property access
#1202: Fixed property access not being reported correctly when it occurred after the initial query context for a retrieve
#1202: Fixed data requirements not being reported correctly when a query source was itself a nested query
#1202: Fixed duplicate data requirements being reported for property access not directly related to a retrieve
#1202: Fixed enableResultTypes not working in some cases
  • Loading branch information
@brynrhodes
brynrhodes authored Oct 25, 2023
1 parent 74deaeb commit c53d2ce
Show file tree
Hide file tree
Showing 23 changed files with 1,683 additions and 288 deletions.
105 changes: 6 additions & 99 deletions Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/Cql2ElmVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,6 @@

public class Cql2ElmVisitor extends CqlPreprocessorElmCommonVisitor {
static final Logger logger = LoggerFactory.getLogger(Cql2ElmVisitor.class);
private boolean locate = false;
private boolean resultTypes = false;
private boolean dateRangeOptimization = false;
private boolean detailedErrors = false;
private boolean methodInvocation = true;
private boolean includeDeprecatedElements = false;
private boolean fromKeywordRequired = false;

private final SystemMethodResolver systemMethodResolver;

public void setLibraryInfo(LibraryInfo libraryInfo) {
Expand Down Expand Up @@ -65,91 +57,6 @@ public Cql2ElmVisitor(LibraryBuilder libraryBuilder) {
this.systemMethodResolver = new SystemMethodResolver(this, libraryBuilder);
}

public void enableLocators() {
locate = true;
}

public void disableLocators() {
locate = false;
}

public void enableResultTypes() {
resultTypes = true;
}

public void disableResultTypes() {
resultTypes = false;
}

public void enableDateRangeOptimization() {
dateRangeOptimization = true;
}

public void disableDateRangeOptimization() {
dateRangeOptimization = false;
}

public boolean getDateRangeOptimization() {
return dateRangeOptimization;
}

public void enableDetailedErrors() {
detailedErrors = true;
}

public void disableDetailedErrors() {
detailedErrors = false;
}

public boolean isDetailedErrorsEnabled() {
return detailedErrors;
}

public void enableMethodInvocation() {
methodInvocation = true;
}

public void disableMethodInvocation() {
methodInvocation = false;
}

public boolean isFromKeywordRequired() {
return fromKeywordRequired;
}

public void enableFromKeywordRequired() {
fromKeywordRequired = true;
}

public void disableFromKeywordRequired() {
fromKeywordRequired = false;
}

public void setTranslatorOptions(CqlCompilerOptions options) {
if (options.getOptions().contains(CqlCompilerOptions.Options.EnableDateRangeOptimization)) {
this.enableDateRangeOptimization();
}
if (options.getOptions().contains(CqlCompilerOptions.Options.EnableAnnotations)) {
this.enableAnnotations();
}
if (options.getOptions().contains(CqlCompilerOptions.Options.EnableLocators)) {
this.enableLocators();
}
if (options.getOptions().contains(CqlCompilerOptions.Options.EnableResultTypes)) {
this.enableResultTypes();
}
if (options.getOptions().contains(CqlCompilerOptions.Options.EnableDetailedErrors)) {
this.enableDetailedErrors();
}
if (options.getOptions().contains(CqlCompilerOptions.Options.DisableMethodInvocation)) {
this.disableMethodInvocation();
}
if (options.getOptions().contains(CqlCompilerOptions.Options.RequireFromKeyword)) {
this.enableFromKeywordRequired();
}
libraryBuilder.setCompatibilityLevel(options.getCompatibilityLevel());
}

public List<Retrieve> getRetrieves() {
return retrieves;
}
Expand Down Expand Up @@ -335,7 +242,7 @@ public TupleElementDefinition visitTupleElementDefinition(cqlParser.TupleElement
.withName(parseString(ctx.referentialIdentifier()))
.withElementType(parseTypeSpecifier(ctx.typeSpecifier()));

if (includeDeprecatedElements) {
if (getIncludeDeprecatedElements()) {
result.setType(result.getElementType());
}

Expand Down Expand Up @@ -3123,7 +3030,7 @@ else if (libraryBuilder.isCompatibleWith("1.5")) {
@Override
public Object visitSourceClause(cqlParser.SourceClauseContext ctx) {
boolean hasFrom = "from".equals(ctx.getChild(0).getText());
if (!hasFrom && fromKeywordRequired) {
if (!hasFrom && isFromKeywordRequired()) {
throw new IllegalArgumentException("The from keyword is required for queries.");
}

Expand Down Expand Up @@ -3177,7 +3084,7 @@ public Object visitQuery(cqlParser.QueryContext ctx) {
}

Expression where = ctx.whereClause() != null ? (Expression) visit(ctx.whereClause()) : null;
if (dateRangeOptimization && where != null) {
if (getDateRangeOptimization() && where != null) {
for (AliasedQuerySource aqs : sources) {
where = optimizeDateRangeInQuery(where, aqs);
}
Expand Down Expand Up @@ -3960,7 +3867,7 @@ public Expression resolveFunctionOrQualifiedFunction(String identifier, cqlParse

// NOTE: FHIRPath method invocation
// If the target is an expression, resolve as a method invocation
if (target instanceof Expression && methodInvocation) {
if (target instanceof Expression && isMethodInvocationEnabled()) {
return systemMethodResolver.resolveMethod((Expression)target, identifier, paramListCtx, true);
}

Expand Down Expand Up @@ -4219,11 +4126,11 @@ private TrackBack getTrackBack(ParserRuleContext ctx) {
}

private void decorate(Element element, TrackBack tb) {
if (locate && tb != null) {
if (locatorsEnabled() && tb != null) {
element.setLocator(tb.toLocator());
}

if (resultTypes && element.getResultType() != null) {
if (resultTypesEnabled() && element.getResultType() != null) {
if (element.getResultType() instanceof NamedType) {
element.setResultTypeName(libraryBuilder.dataTypeToQName(element.getResultType()));
}
Expand Down
110 changes: 110 additions & 0 deletions ...c/main/java/org/cqframework/cql/cql2elm/preprocessor/CqlPreprocessorElmCommonVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ public class CqlPreprocessorElmCommonVisitor extends cqlBaseVisitor {
private int nextLocalId = 1;
private boolean locate = false;
private boolean resultTypes = false;
private boolean dateRangeOptimization = false;
private boolean methodInvocation = true;
private boolean fromKeywordRequired = false;

private final List<Expression> expressions = new ArrayList<>();
private boolean includeDeprecatedElements = false;

Expand Down Expand Up @@ -783,4 +787,110 @@ public static String normalizeWhitespace(String input) {
public static boolean isStartingWithDigit(String header, int index) {
return (index < header.length()) && Character.isDigit(header.charAt(index));
}


public void enableLocators() {
locate = true;
}

public boolean locatorsEnabled() {
return locate;
}

public void disableLocators() {
locate = false;
}

public void enableResultTypes() {
resultTypes = true;
}

public void disableResultTypes() {
resultTypes = false;
}

public boolean resultTypesEnabled() {
return resultTypes;
}

public void enableDateRangeOptimization() {
dateRangeOptimization = true;
}

public void disableDateRangeOptimization() {
dateRangeOptimization = false;
}

public boolean getDateRangeOptimization() {
return dateRangeOptimization;
}

public void enableDetailedErrors() {
detailedErrors = true;
}

public void disableDetailedErrors() {
detailedErrors = false;
}

public boolean isDetailedErrorsEnabled() {
return detailedErrors;
}

public void enableMethodInvocation() {
methodInvocation = true;
}

public void disableMethodInvocation() {
methodInvocation = false;
}

public boolean isMethodInvocationEnabled() {
return methodInvocation;
}

public boolean isFromKeywordRequired() {
return fromKeywordRequired;
}

public void enableFromKeywordRequired() {
fromKeywordRequired = true;
}

public void disableFromKeywordRequired() {
fromKeywordRequired = false;
}

public boolean getIncludeDeprecatedElements() {
return includeDeprecatedElements;
}

public void setIncludeDeprecatedElements(boolean includeDeprecatedElements) {
this.includeDeprecatedElements = includeDeprecatedElements;
}

public void setTranslatorOptions(CqlCompilerOptions options) {
if (options.getOptions().contains(CqlCompilerOptions.Options.EnableDateRangeOptimization)) {
this.enableDateRangeOptimization();
}
if (options.getOptions().contains(CqlCompilerOptions.Options.EnableAnnotations)) {
this.enableAnnotations();
}
if (options.getOptions().contains(CqlCompilerOptions.Options.EnableLocators)) {
this.enableLocators();
}
if (options.getOptions().contains(CqlCompilerOptions.Options.EnableResultTypes)) {
this.enableResultTypes();
}
if (options.getOptions().contains(CqlCompilerOptions.Options.EnableDetailedErrors)) {
this.enableDetailedErrors();
}
if (options.getOptions().contains(CqlCompilerOptions.Options.DisableMethodInvocation)) {
this.disableMethodInvocation();
}
if (options.getOptions().contains(CqlCompilerOptions.Options.RequireFromKeyword)) {
this.enableFromKeywordRequired();
}
libraryBuilder.setCompatibilityLevel(options.getCompatibilityLevel());
}
}
16 changes: 15 additions & 1 deletion Src/java/elm-fhir/src/main/java/org/cqframework/cql/elm/requirements/ElmDataRequirement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,11 @@ else if (querySource instanceof LetClause) {
private static ElmDataRequirement inferFrom(ElmDataRequirement requirement) {
Retrieve inferredRetrieve = ElmCloner.clone(requirement.getRetrieve());
ElmDataRequirement result = new ElmDataRequirement(requirement.libraryIdentifier, inferredRetrieve, requirement.getRetrieve());
result.propertySet = requirement.propertySet;
if (requirement.hasProperties()) {
for (Property p : requirement.getProperties()) {
result.addProperty(p);
}
}
return result;
}

Expand Down Expand Up @@ -127,13 +131,23 @@ public Iterable<Property> getProperties() {
return propertySet;
}

public boolean hasProperties() {
return propertySet != null;
}

public void addProperty(Property property) {
if (propertySet == null) {
propertySet = new LinkedHashSet<Property>();
}
propertySet.add(property);
}

public void removeProperty(Property property) {
if (propertySet != null) {
propertySet.remove(property);
}
}

public void reportProperty(ElmPropertyRequirement propertyRequirement) {
if (propertySet == null) {
propertySet = new LinkedHashSet<Property>();
Expand Down
36 changes: 34 additions & 2 deletions Src/java/elm-fhir/src/main/java/org/cqframework/cql/elm/requirements/ElmRequirements.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public Iterable<ElmRequirement> getRetrieves() {
For expressions, unique by qualified name
For data requirements, collapse according to the CQL specification: https://cql.hl7.org/05-languagesemantics.html#artifact-data-requirements
*/
public ElmRequirements collapse() {
public ElmRequirements collapse(ElmRequirementsContext context) {
ElmRequirements result = new ElmRequirements(this.libraryIdentifier, this.element);

// UsingDefs
Expand Down Expand Up @@ -243,6 +243,7 @@ public ElmRequirements collapse() {
// Retrieves
// Sort retrieves by type/profile to reduce search space
LinkedHashMap<String, List<ElmRequirement>> retrievesByType = new LinkedHashMap<String, List<ElmRequirement>>();
List<ElmRequirement> unboundRequirements = new ArrayList<>();
for (ElmRequirement r : getRetrieves()) {
Retrieve retrieve = (Retrieve)r.getElement();
if (retrieve.getDataType() != null) {
Expand All @@ -257,9 +258,40 @@ public ElmRequirements collapse() {
}
typeRetrieves.add(r);
}
// TODO: What to do with data requirements that are captured to track unbound element references
else {
unboundRequirements.add(r);
}
}

// Distribute unbound property requirements
// If an ElmDataRequirement has a retrieve that does not have a dataType (i.e. it is not a direct data access layer retrieve
// but rather is the result of requirements inference), then distribute the property references it contains to
// all data layer-bound retrieves of the same type
// In other words, we can't unambiguously tie the property reference to any particular retrieve of that type,
// so apply it to all of them
for (ElmRequirement requirement : unboundRequirements) {
if (requirement instanceof ElmDataRequirement) {
ElmDataRequirement dataRequirement = (ElmDataRequirement)requirement;
if (dataRequirement.hasProperties()) {
String typeUri = context.getTypeResolver().getTypeUri(dataRequirement.getRetrieve().getResultType());
if (typeUri != null) {
List<ElmRequirement> typeRequirements = retrievesByType.get(typeUri);
if (typeRequirements != null) {
for (ElmRequirement typeRequirement : typeRequirements) {
if (typeRequirement instanceof ElmDataRequirement) {
ElmDataRequirement typeDataRequirement = (ElmDataRequirement)typeRequirement;
for (Property p : dataRequirement.getProperties()) {
typeDataRequirement.addProperty(p);
}
}
}
}
}
}
}
}


// Equivalent
// Has the same context, type/profile, code path and date path
// If two retrieves are "equivalent" they can be merged
Expand Down
Loading

0 comments on commit c53d2ce

Please sign in to comment.