mirror of
https://github.com/hapifhir/hapi-fhir.git
synced 2025-03-09 14:33:32 +00:00
Fix range calculation for number or quantity search parameters (#3870)
by: juan.marchionatto <juan.marchionatto@smilecdr.com>
This commit is contained in:
jmarchionatto
GitHub
parent
eae1c0ffb8
commit
71a3fa949c
@ -0,0 +1,23 @@
|
||||
package ca.uhn.fhir.util;
|
||||
|
||||
import org.apache.commons.lang3.tuple.Pair;
|
||||
|
||||
import java.math.BigDecimal;
|
||||
|
||||
public class NumericParamRangeUtil {
|
||||
|
||||
private NumericParamRangeUtil() {}
|
||||
|
||||
/**
|
||||
* Provides a Pair containing the low and high boundaries for a range based on the received number
|
||||
* characteristics. Note that the scale is used to calculate significant figures
|
||||
* @param theNumber the number which range is returned
|
||||
* @return a Pair of BigDecimal(s) with the low and high range boundaries
|
||||
*/
|
||||
public static Pair<BigDecimal, BigDecimal> getRange(BigDecimal theNumber) {
|
||||
BigDecimal halfRange = BigDecimal.valueOf(.5).movePointLeft( theNumber.scale() );
|
||||
return Pair.of(theNumber.subtract(halfRange), theNumber.add(halfRange));
|
||||
}
|
||||
|
||||
|
||||
}
|
||||
@ -0,0 +1,104 @@
|
||||
package ca.uhn.fhir.util;
|
||||
|
||||
import org.apache.commons.lang3.tuple.Pair;
|
||||
import org.junit.jupiter.params.ParameterizedTest;
|
||||
import org.junit.jupiter.params.provider.Arguments;
|
||||
import org.junit.jupiter.params.provider.MethodSource;
|
||||
|
||||
import java.math.BigDecimal;
|
||||
import java.util.stream.Stream;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||
|
||||
public class NumericParamRangeUtilTest {
|
||||
|
||||
@ParameterizedTest
|
||||
@MethodSource("provideParameters")
|
||||
void testSignificantFigures(String theNumber, int theExpectedSigDigs) {
|
||||
BigDecimal bd = new BigDecimal(theNumber);
|
||||
System.out.printf("%1$10s %2$2s %3$2s %4$2s %n",
|
||||
theNumber, bd.scale(), bd.precision(), theExpectedSigDigs);
|
||||
assertEquals(theExpectedSigDigs, bd.precision());
|
||||
}
|
||||
|
||||
|
||||
@ParameterizedTest
|
||||
@MethodSource("provideRangeParameters")
|
||||
void testObtainedRange(String theNumber, Pair<Double, Double> theExpectedRange) {
|
||||
BigDecimal bd = new BigDecimal(theNumber);
|
||||
|
||||
Pair<BigDecimal, BigDecimal> range = NumericParamRangeUtil.getRange(bd);
|
||||
|
||||
System.out.printf("%1$10s %2$2s %3$2s :: %4$8f %5$8f :: %6$8f %7$8f%n",
|
||||
theNumber, bd.scale(), bd.precision(),
|
||||
theExpectedRange.getLeft(), theExpectedRange.getRight(),
|
||||
range.getLeft(), range.getRight() );
|
||||
|
||||
assertEquals(theExpectedRange.getLeft(), range.getLeft().doubleValue() );
|
||||
assertEquals(theExpectedRange.getRight(), range.getRight().doubleValue() );
|
||||
}
|
||||
|
||||
|
||||
private static Stream<Arguments> provideRangeParameters() {
|
||||
return Stream.of(
|
||||
// cases from Number FHIR spec: https://www.hl7.org/fhir/search.html#number
|
||||
|
||||
// [parameter]=100 Values that equal 100, to 3 significant figures precision, so this is actually searching for values in the range [99.5 ... 100.5)
|
||||
Arguments.of("100", Pair.of( 99.5d, 100.5d)), // 100 +/- 0.5 (0.5e(3-3) )
|
||||
|
||||
// [parameter]=100.00 Values that equal 100, to 5 significant figures precision, so this is actually searching for values in the range [99.995 ... 100.005)
|
||||
Arguments.of("100.00", Pair.of(99.995d, 100.005d)), // 100 +/- 0.005 (0.5e-2 = 0.5e(3-5) )
|
||||
|
||||
// [parameter]=1e2 Values that equal 100, to 1 significant figures precision, so this is actually searching for values in the range [95 ... 105)
|
||||
// We considered this definition to be WWRONG! It should be
|
||||
|
||||
// Wouldn’t it be actually in the range [50 - 150) ?
|
||||
// because there is only 1 significant digit so the range would be (1 +/- 0.5)e2
|
||||
|
||||
Arguments.of("1e2", Pair.of(50d, 150d)), // 100 +/- 50 (0.5e2) <===== ???
|
||||
|
||||
// cases from Quantity FHIR spec: https://www.hl7.org/fhir/search.html#quantity
|
||||
|
||||
// GET [base]/Observation?value-quantity=5.4|http://unitsofmeasure.org|mg
|
||||
// Search for all the observations with a value of 5.4(+/-0.05) mg where mg is understood as a UCUM unit (system/code)
|
||||
Arguments.of("5.4", Pair.of(5.35d, 5.45d)), // 5.4 +/- 0.05 (=0.5e-1 =0.5e(1-2) )
|
||||
|
||||
// GET [base]/Observation?value-quantity=5.40e-3|http://unitsofmeasure.org|g
|
||||
// Search for all the observations with a value of 0.0054(+/-0.000005) g where g is understood as a UCUM unit (system/code)
|
||||
Arguments.of("5.40e-3", Pair.of(0.005395d, 0.005405d)) // 0.0054 +/- 0.000005 (=0.5e5 =0.5e(-3-2) )
|
||||
|
||||
);
|
||||
|
||||
}
|
||||
|
||||
|
||||
private static Stream<Arguments> provideParameters() {
|
||||
return Stream.of(
|
||||
Arguments.of("1234", 4),
|
||||
Arguments.of("101.001", 6),
|
||||
Arguments.of("41003", 5),
|
||||
Arguments.of("500", 3),
|
||||
Arguments.of("13000", 5),
|
||||
Arguments.of("140e-001", 3),
|
||||
Arguments.of("500.", 3),
|
||||
Arguments.of("5.0e2", 2),
|
||||
Arguments.of("2.000", 4),
|
||||
Arguments.of("8.200000e3", 7),
|
||||
|
||||
// cases from the FHIR spec
|
||||
|
||||
// [parameter]=100 Values that equal 100, to 3 significant figures precision, so this is actually searching for values in the range [99.5 ... 100.5)
|
||||
Arguments.of("100", 3),
|
||||
// [parameter]=100.00 Values that equal 100, to 5 significant figures precision, so this is actually searching for values in the range [99.995 ... 100.005)
|
||||
Arguments.of("100.00", 5),
|
||||
// [parameter]=1e2 Values that equal 100, to 1 significant figures precision, so this is actually searching for values in the range [95 ... 105)
|
||||
Arguments.of("1e2", 1)
|
||||
);
|
||||
|
||||
}
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
}
|
||||
@ -0,0 +1,5 @@
|
||||
---
|
||||
type: fix
|
||||
issue: 3868
|
||||
title: "With Elastic/Lucene indexing enabled search for numbers or quantities was not always using the specified ranges,
|
||||
because the number of significant figures was not properly calculated. This is now fixed."
|
||||
@ -40,6 +40,7 @@ import ca.uhn.fhir.rest.param.TokenParam;
|
||||
import ca.uhn.fhir.rest.param.UriParam;
|
||||
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
|
||||
import ca.uhn.fhir.util.DateUtils;
|
||||
import ca.uhn.fhir.util.NumericParamRangeUtil;
|
||||
import ca.uhn.fhir.util.StringUtil;
|
||||
import org.apache.commons.collections4.CollectionUtils;
|
||||
import org.apache.commons.lang3.StringUtils;
|
||||
@ -52,6 +53,7 @@ import org.slf4j.Logger;
|
||||
import org.slf4j.LoggerFactory;
|
||||
|
||||
import javax.annotation.Nonnull;
|
||||
import java.math.BigDecimal;
|
||||
import java.time.Instant;
|
||||
import java.util.Arrays;
|
||||
import java.util.HashSet;
|
||||
@ -653,7 +655,8 @@ public class ExtendedHSearchClauseBuilder {
|
||||
for (NumberParam orTerm : orTerms) {
|
||||
double value = orTerm.getValue().doubleValue();
|
||||
double approxTolerance = value * QTY_APPROX_TOLERANCE_PERCENT;
|
||||
double defaultTolerance = value * QTY_TOLERANCE_PERCENT;
|
||||
Pair<BigDecimal, BigDecimal> range = NumericParamRangeUtil.getRange(orTerm.getValue());
|
||||
|
||||
|
||||
ParamPrefixEnum activePrefix = orTerm.getPrefix() == null ? ParamPrefixEnum.EQUAL : orTerm.getPrefix();
|
||||
switch (activePrefix) {
|
||||
@ -664,7 +667,7 @@ public class ExtendedHSearchClauseBuilder {
|
||||
|
||||
case EQUAL:
|
||||
numberPredicateStep.should(myPredicateFactory.range()
|
||||
.field(fieldPath).between(value - defaultTolerance, value + defaultTolerance));
|
||||
.field(fieldPath).between(range.getLeft().doubleValue(), range.getRight().doubleValue()));
|
||||
break;
|
||||
|
||||
case STARTS_AFTER:
|
||||
@ -686,7 +689,8 @@ public class ExtendedHSearchClauseBuilder {
|
||||
break;
|
||||
|
||||
case NOT_EQUAL:
|
||||
numberPredicateStep.mustNot(myPredicateFactory.match().field(fieldPath).matching(value));
|
||||
numberPredicateStep.mustNot(myPredicateFactory.range()
|
||||
.field(fieldPath).between(range.getLeft().doubleValue(), range.getRight().doubleValue()));
|
||||
break;
|
||||
}
|
||||
}
|
||||
@ -695,5 +699,4 @@ public class ExtendedHSearchClauseBuilder {
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
}
|
||||
|
||||
@ -2342,18 +2342,13 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl
|
||||
}
|
||||
|
||||
/**
|
||||
* The following tests are to validate the specification implementation, but they don't work because we save parameters as BigInteger
|
||||
* which invalidates the possibility to differentiate requested significant figures, which are needed to define precision ranges
|
||||
* Leaving them here in case some day we fix the implementations
|
||||
* We copy the JPA implementation here, which ignores precision requests and treats all numbers using default ranges
|
||||
* @see TestSpecCasesNotUsingSignificantFigures
|
||||
* The following tests are to validate the specification implementation
|
||||
*/
|
||||
@Disabled
|
||||
@Nested
|
||||
public class TestSpecCasesUsingSignificantFigures {
|
||||
|
||||
@Test
|
||||
void specCase1() {
|
||||
void NumberSpecCase1() {
|
||||
String raId1 = createRiskAssessmentWithPredictionProbability(99.4).getIdPart();
|
||||
String raId2 = createRiskAssessmentWithPredictionProbability(99.6).getIdPart();
|
||||
String raId3 = createRiskAssessmentWithPredictionProbability(100.4).getIdPart();
|
||||
@ -2363,7 +2358,7 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl
|
||||
}
|
||||
|
||||
@Test
|
||||
void specCase2() {
|
||||
void NumberSpecCase2() {
|
||||
String raId1 = createRiskAssessmentWithPredictionProbability(99.994).getIdPart();
|
||||
String raId2 = createRiskAssessmentWithPredictionProbability(99.996).getIdPart();
|
||||
String raId3 = createRiskAssessmentWithPredictionProbability(100.004).getIdPart();
|
||||
@ -2373,22 +2368,18 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl
|
||||
}
|
||||
|
||||
@Test
|
||||
void specCase3() {
|
||||
String raId1 = createRiskAssessmentWithPredictionProbability(94).getIdPart();
|
||||
String raId2 = createRiskAssessmentWithPredictionProbability(96).getIdPart();
|
||||
String raId3 = createRiskAssessmentWithPredictionProbability(104).getIdPart();
|
||||
String raId4 = createRiskAssessmentWithPredictionProbability(106).getIdPart();
|
||||
void NumberSpecCase3() {
|
||||
String raId1 = createRiskAssessmentWithPredictionProbability(40).getIdPart();
|
||||
String raId2 = createRiskAssessmentWithPredictionProbability(55).getIdPart();
|
||||
String raId3 = createRiskAssessmentWithPredictionProbability(140).getIdPart();
|
||||
String raId4 = createRiskAssessmentWithPredictionProbability(155).getIdPart();
|
||||
// [parameter]=1e2 Values that equal 100, to 1 significant figures precision, so this is actually searching for values in the range [95 ... 105)
|
||||
// Previous line from spec is wrong! Range for [parameter]=1e2, having 1 significant figures precision, should be [50 ... 150]
|
||||
assertFindIds("when le", Set.of(raId2, raId3), "/RiskAssessment?probability=1e2");
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
@Nested
|
||||
public class TestSpecCasesNotUsingSignificantFigures {
|
||||
|
||||
@Test
|
||||
void specCase4() {
|
||||
void NumberSpecCase4() {
|
||||
String raId1 = createRiskAssessmentWithPredictionProbability(99).getIdPart();
|
||||
String raId2 = createRiskAssessmentWithPredictionProbability(100).getIdPart();
|
||||
// [parameter]=lt100 Values that are less than exactly 100
|
||||
@ -2396,7 +2387,7 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl
|
||||
}
|
||||
|
||||
@Test
|
||||
void specCase5() {
|
||||
void NumberSpecCase5() {
|
||||
String raId1 = createRiskAssessmentWithPredictionProbability(99).getIdPart();
|
||||
String raId2 = createRiskAssessmentWithPredictionProbability(100).getIdPart();
|
||||
String raId3 = createRiskAssessmentWithPredictionProbability(101).getIdPart();
|
||||
@ -2405,7 +2396,7 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl
|
||||
}
|
||||
|
||||
@Test
|
||||
void specCase6() {
|
||||
void NumberSpecCase6() {
|
||||
String raId1 = createRiskAssessmentWithPredictionProbability(100).getIdPart();
|
||||
String raId2 = createRiskAssessmentWithPredictionProbability(101).getIdPart();
|
||||
// [parameter]=gt100 Values that are greater than exactly 100
|
||||
@ -2413,7 +2404,7 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl
|
||||
}
|
||||
|
||||
@Test
|
||||
void specCase7() {
|
||||
void NumberSpecCase7() {
|
||||
String raId1 = createRiskAssessmentWithPredictionProbability(99).getIdPart();
|
||||
String raId2 = createRiskAssessmentWithPredictionProbability(100).getIdPart();
|
||||
String raId3 = createRiskAssessmentWithPredictionProbability(101).getIdPart();
|
||||
@ -2422,15 +2413,48 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl
|
||||
}
|
||||
|
||||
@Test
|
||||
void specCase8() {
|
||||
void NumberSpecCase8() {
|
||||
String raId1 = createRiskAssessmentWithPredictionProbability(99.4).getIdPart();
|
||||
String raId2 = createRiskAssessmentWithPredictionProbability(99.6).getIdPart();
|
||||
String raId3 = createRiskAssessmentWithPredictionProbability(100.4).getIdPart();
|
||||
String raId4 = createRiskAssessmentWithPredictionProbability(100.6).getIdPart();
|
||||
String raId5 = createRiskAssessmentWithPredictionProbability(100).getIdPart();
|
||||
String raId3 = createRiskAssessmentWithPredictionProbability(100).getIdPart();
|
||||
String raId4 = createRiskAssessmentWithPredictionProbability(100.4).getIdPart();
|
||||
String raId5 = createRiskAssessmentWithPredictionProbability(100.6).getIdPart();
|
||||
// [parameter]=ne100 Values that are not equal to 100 (actually, in the range 99.5 to 100.5)
|
||||
assertFindIds("when le", Set.of(raId1, raId2, raId3, raId4), "/RiskAssessment?probability=ne100");
|
||||
assertFindIds("when le", Set.of(raId1, raId5), "/RiskAssessment?probability=ne100");
|
||||
}
|
||||
|
||||
@Test
|
||||
void QuantitySpecCase1() {
|
||||
String raId1 = createRiskAssessmentWithPredictionProbability(5.34).getIdPart();
|
||||
String raId2 = createRiskAssessmentWithPredictionProbability(5.36).getIdPart();
|
||||
String raId3 = createRiskAssessmentWithPredictionProbability(5.40).getIdPart();
|
||||
String raId4 = createRiskAssessmentWithPredictionProbability(5.44).getIdPart();
|
||||
String raId5 = createRiskAssessmentWithPredictionProbability(5.46).getIdPart();
|
||||
// GET [base]/Observation?value-quantity=5.4 :: 5.4(+/-0.05)
|
||||
assertFindIds("when le", Set.of(raId2, raId3, raId4), "/RiskAssessment?probability=5.4");
|
||||
}
|
||||
|
||||
@Test
|
||||
void QuantitySpecCase2() {
|
||||
String raId1 = createRiskAssessmentWithPredictionProbability(0.005394).getIdPart();
|
||||
String raId2 = createRiskAssessmentWithPredictionProbability(0.005395).getIdPart();
|
||||
String raId3 = createRiskAssessmentWithPredictionProbability(0.0054).getIdPart();
|
||||
String raId4 = createRiskAssessmentWithPredictionProbability(0.005404).getIdPart();
|
||||
String raId5 = createRiskAssessmentWithPredictionProbability(0.005406).getIdPart();
|
||||
// GET [base]/Observation?value-quantity=5.40e-3 :: 0.0054(+/-0.000005)
|
||||
assertFindIds("when le", Set.of(raId2, raId3, raId4), "/RiskAssessment?probability=5.40e-3");
|
||||
}
|
||||
|
||||
@Test
|
||||
void QuantitySpecCase6() {
|
||||
String raId1 = createRiskAssessmentWithPredictionProbability(4.85).getIdPart();
|
||||
String raId2 = createRiskAssessmentWithPredictionProbability(4.86).getIdPart();
|
||||
String raId3 = createRiskAssessmentWithPredictionProbability(5.94).getIdPart();
|
||||
String raId4 = createRiskAssessmentWithPredictionProbability(5.95).getIdPart();
|
||||
// GET [base]/Observation?value-quantity=ap5.4 :: 5.4(+/- 10%) :: [4.86 ... 5.94]
|
||||
assertFindIds("when le", Set.of(raId2, raId3), "/RiskAssessment?probability=ap5.4");
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user