fix: fix ne prefix was matching extra records #64
Conversation
| exists: { | ||
| field: startField, | ||
| }, | ||
| }, | ||
| { | ||
| exists: { | ||
| field: endField, | ||
| }, |
There was a problem hiding this comment.
Is it valid to have a start period but no end (or vice versa)?
There was a problem hiding this comment.
https://www.hl7.org/fhir/datatypes.html#period
FYI it looks like it is a usecase -- how much does it complicate things?
There was a problem hiding this comment.
yeah, I saw that start and end are optional.
The meaning of missing bounds in FHIR is rather vague and that's why I didn't immediately addressed that case:
If the start element is missing, the start of the period is not known(so what?). If the end element is missing, it means that the period is ongoing(does "ongoing" extends until now? or infinitely into the future?)
My interpretation would be that no start extends infinitely into the past and no end extends infinitely into the future. I'm not sure about how to validate it, but from the source code it looks like the IBM implementation agrees with that interpretation, so we'd likely go with that.
In any case, those cases can be part of a separate PR. They won't change the current query but rather expand it via composition.
Currently we have:
(start exists && end exists && prefixRangePeriodQuery)
The goal would be:
(start exists && end exists && prefixRangePeriodQuery)
|| (start not exists && end exists && prefixRangePeriodNoStartQuery)
|| (start exists && end not exists && prefixRangePeriodNoEndQuery)
The recent addition of support for Period type fields broke search for regular date fields when using the
neprefix.queries with
newhere returning additional results and integ tests caught the issue.The issue was a bad combination of the implicit failure of non-existent fields in range queries + must_not clauses
All integ test pass after this fix.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.