search

FHIR / sql-on-fhir-v2

This project provides the source for the SQL on FHIR v2.0 Implementation Guide
https://build.fhir.org/ig/FHIR/sql-on-fhir-v2/
MIT License
100 stars 27 forks source link

Fix expectation in basic > where returns non-boolean for some cases #219

Closed johngrimes closed 7 months ago

johngrimes commented 8 months ago

Fixes #218 and #198.

mput commented 7 months ago

Hi @johngrimes This PR is related to the discussion: https://chat.fhir.org/#narrow/stream/179219-analytics-on-FHIR/topic/Constraints.20on.20.60ViewDefinition.2Ewhere.2Epath.60.20expression

I think we should add clarification to the related paragraphs in the spec. Possible change to the spec that describes the change from this PR:

ViewDefinition.where [no change]

A series of zero or more FHIRPath constraints to filter resources for the view. Every constraint must evaluate to true for the resource to be included in the view.

ViewDefinition.where.path [old]

A FHIRPath expression that defines a filter that must evaluate to true for a resource to be included in the output. The input context is the collection of resources of the type specified in the resource element. Constants defined in Reference({constant}) can be referenced as %[name]. The result of the expression must of type Boolean.

ViewDefinition.where.path [New - empty allowed]

A FHIRPath expression that defines a filter that must evaluate to true for a resource to be included in the output. The input context is the collection of resources of the type specified in the resource element. Constants defined in Reference({constant}) can be referenced as %[name]. The expression must evaluate to a single item of type Boolean or be empty. Implementations SHALL report an error for any other types of results.

The current behavior (before the PR) is described by this definition.

ViewDefinition.where.path [New - strict]

A FHIRPath expression that defines a filter that must evaluate to true for a resource to be included in the output. The input context is the collection of resources of the type specified in the resource element. Constants defined in Reference({constant}) can be referenced as %[name]. The expression must evaluate to a single item of type Boolean. Implementations SHALL report an error for any other types of results.

What do you think? I also prepared tests for corner cases like multiple values in the result, but didn't commit them before we agreed on the behavior. I also think it's a good idea to move all cases related to the "where" expression into a separate test.

johngrimes commented 7 months ago

Hi @mput, thanks for taking a look at this!

Yes, we may as well roll in the spec change to this PR.

My feeling is that we don't need to be quite a strict as you suggested, as we have already said that only a result of true will be included.

How about this:

A FHIRPath expression that defines a filter that must evaluate to true for a resource to be included in the output. The input context is the collection of resources of the type specified in the resource element. Constants defined in Reference({constant}) can be referenced as %[name]. The result of the expression must of type Boolean.

johngrimes commented 7 months ago

It looks like the change to the test expectation was already made on the master branch? 🤷