From 71a3fa949c79e27c3f6a0911453713fc28ec8053 Mon Sep 17 00:00:00 2001 From: jmarchionatto <60409882+jmarchionatto@users.noreply.github.com> Date: Wed, 3 Aug 2022 11:51:34 -0400 Subject: [PATCH] Fix range calculation for number or quantity search parameters (#3870) by: juan.marchionatto --- .../uhn/fhir/util/NumericParamRangeUtil.java | 23 ++++ .../fhir/util/NumericParamRangeUtilTest.java | 104 ++++++++++++++++++ ...search-numeric-ranges-wrong-precision.yaml | 5 + .../search/ExtendedHSearchClauseBuilder.java | 11 +- ...esourceDaoR4SearchWithElasticSearchIT.java | 78 ++++++++----- 5 files changed, 190 insertions(+), 31 deletions(-) create mode 100644 hapi-fhir-base/src/main/java/ca/uhn/fhir/util/NumericParamRangeUtil.java create mode 100644 hapi-fhir-base/src/test/java/ca/uhn/fhir/util/NumericParamRangeUtilTest.java create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/3868-hsearch-numeric-ranges-wrong-precision.yaml diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/NumericParamRangeUtil.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/NumericParamRangeUtil.java new file mode 100644 index 00000000000..62ae3725c1c --- /dev/null +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/NumericParamRangeUtil.java @@ -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 getRange(BigDecimal theNumber) { + BigDecimal halfRange = BigDecimal.valueOf(.5).movePointLeft( theNumber.scale() ); + return Pair.of(theNumber.subtract(halfRange), theNumber.add(halfRange)); + } + + +} diff --git a/hapi-fhir-base/src/test/java/ca/uhn/fhir/util/NumericParamRangeUtilTest.java b/hapi-fhir-base/src/test/java/ca/uhn/fhir/util/NumericParamRangeUtilTest.java new file mode 100644 index 00000000000..a2f6ed4a901 --- /dev/null +++ b/hapi-fhir-base/src/test/java/ca/uhn/fhir/util/NumericParamRangeUtilTest.java @@ -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 theExpectedRange) { + BigDecimal bd = new BigDecimal(theNumber); + + Pair 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 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 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) + ); + + } + + + + + +} diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/3868-hsearch-numeric-ranges-wrong-precision.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/3868-hsearch-numeric-ranges-wrong-precision.yaml new file mode 100644 index 00000000000..9d05d69857c --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/3868-hsearch-numeric-ranges-wrong-precision.yaml @@ -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." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchClauseBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchClauseBuilder.java index 29e6f35e5f7..483334c534a 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchClauseBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchClauseBuilder.java @@ -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 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 { } } - } diff --git a/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java b/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java index 748790cc30e..20ca991910f 100644 --- a/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java +++ b/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java @@ -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