From b2b0ff22b4a8b3bccc3c322c96ed32448e85b51c Mon Sep 17 00:00:00 2001 From: James Agnew Date: Tue, 25 Feb 2020 17:17:13 -0500 Subject: [PATCH] Collapse date joins (#1726) * Collapse date joins * Add changelog * Address review comments * Add whitespace * fix test Co-authored-by: Ken Stevens --- .../4_3_0/1726-collapse-date-joins.yaml | 6 ++ .../dao/predicate/PredicateBuilderDate.java | 37 +++++-- .../r4/FhirResourceDaoR4SearchNoFtTest.java | 99 +++++++++++++++++++ .../api/ISchemaInitializationProvider.java | 4 +- .../taskdef/InitializeSchemaTaskTest.java | 5 + 5 files changed, 142 insertions(+), 9 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1726-collapse-date-joins.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1726-collapse-date-joins.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1726-collapse-date-joins.yaml new file mode 100644 index 00000000000..584b5dd2b63 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1726-collapse-date-joins.yaml @@ -0,0 +1,6 @@ +--- +type: perf +issue: 1726 +title: When performing date range searches in the JPA server, the server was generating extra + unneccessary joins in the generated SQL. This has been streamlined, which should result in + faster searches when performing date ranges. diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/predicate/PredicateBuilderDate.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/predicate/PredicateBuilderDate.java index 6ad65712e1d..37274a1d07a 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/predicate/PredicateBuilderDate.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/predicate/PredicateBuilderDate.java @@ -39,13 +39,17 @@ import javax.persistence.criteria.Join; import javax.persistence.criteria.Predicate; import java.util.ArrayList; import java.util.Date; +import java.util.HashMap; import java.util.List; +import java.util.Map; @Component @Scope("prototype") public class PredicateBuilderDate extends BasePredicateBuilder implements IPredicateBuilder { private static final Logger ourLog = LoggerFactory.getLogger(PredicateBuilderDate.class); + private Map> myJoinMap; + PredicateBuilderDate(SearchBuilder theSearchBuilder) { super(theSearchBuilder); } @@ -56,7 +60,18 @@ public class PredicateBuilderDate extends BasePredicateBuilder implements IPredi List theList, SearchFilterParser.CompareOperation operation) { - Join join = createJoin(SearchBuilderJoinEnum.DATE, theParamName); + boolean newJoin = false; + if (myJoinMap == null) { + myJoinMap = new HashMap<>(); + } + String key = theResourceName + " " + theParamName; + + Join join = myJoinMap.get(key); + if (join == null) { + join = createJoin(SearchBuilderJoinEnum.DATE, theParamName); + myJoinMap.put(key, join); + newJoin = true; + } if (theList.get(0).getMissing() != null) { Boolean missing = theList.get(0).getMissing(); @@ -77,7 +92,14 @@ public class PredicateBuilderDate extends BasePredicateBuilder implements IPredi } Predicate orPredicates = myBuilder.or(toArray(codePredicates)); - myQueryRoot.addPredicate(orPredicates); + + if (newJoin) { + Predicate identityAndValuePredicate = combineParamIndexPredicateWithParamNamePredicate(theResourceName, theParamName, join, orPredicates); + myQueryRoot.addPredicate(identityAndValuePredicate); + } else { + myQueryRoot.addPredicate(orPredicates); + } + return orPredicates; } @@ -86,12 +108,13 @@ public class PredicateBuilderDate extends BasePredicateBuilder implements IPredi String theParamName, CriteriaBuilder theBuilder, From theFrom) { - return createPredicateDate(theParam, + Predicate predicateDate = createPredicateDate(theParam, theResourceName, theParamName, theBuilder, theFrom, null); + return combineParamIndexPredicateWithParamNamePredicate(theResourceName, theParamName, theFrom, predicateDate); } private Predicate createPredicateDate(IQueryParameterType theParam, @@ -99,7 +122,7 @@ public class PredicateBuilderDate extends BasePredicateBuilder implements IPredi String theParamName, CriteriaBuilder theBuilder, From theFrom, - SearchFilterParser.CompareOperation operation) { + SearchFilterParser.CompareOperation theOperation) { Predicate p; if (theParam instanceof DateParam) { @@ -109,7 +132,7 @@ public class PredicateBuilderDate extends BasePredicateBuilder implements IPredi p = createPredicateDateFromRange(theBuilder, theFrom, range, - operation); + theOperation); } else { // TODO: handle missing date param? p = null; @@ -119,12 +142,12 @@ public class PredicateBuilderDate extends BasePredicateBuilder implements IPredi p = createPredicateDateFromRange(theBuilder, theFrom, range, - operation); + theOperation); } else { throw new IllegalArgumentException("Invalid token type: " + theParam.getClass()); } - return combineParamIndexPredicateWithParamNamePredicate(theResourceName, theParamName, theFrom, p); + return p; } private Predicate createPredicateDateFromRange(CriteriaBuilder theBuilder, diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java index 3ac22eca837..b9a3a1370c5 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java @@ -3258,6 +3258,105 @@ public class FhirResourceDaoR4SearchNoFtTest extends BaseJpaR4Test { } } + @Test + public void testSearchWithDateAndReusesExistingJoin() { + // Add a search parameter to Observation.issued, so that between that one + // and the existing one on Observation.effective, we have 2 date search parameters + // on the same resource + { + SearchParameter sp = new SearchParameter(); + sp.setStatus(Enumerations.PublicationStatus.ACTIVE); + sp.addBase("Observation"); + sp.setType(Enumerations.SearchParamType.DATE); + sp.setCode("issued"); + sp.setExpression("Observation.issued"); + mySearchParameterDao.create(sp); + mySearchParamRegistry.forceRefresh(); + } + + // Dates are reversed on these two observations + IIdType obsId1; + { + Observation obs = new Observation(); + obs.setIssuedElement(new InstantType("2020-06-06T12:00:00Z")); + obs.setEffective(new InstantType("2019-06-06T12:00:00Z")); + obsId1 = myObservationDao.create(obs).getId().toUnqualifiedVersionless(); + } + IIdType obsId2; + { + Observation obs = new Observation(); + obs.setIssuedElement(new InstantType("2019-06-06T12:00:00Z")); + obs.setEffective(new InstantType("2020-06-06T12:00:00Z")); + obsId2 = myObservationDao.create(obs).getId().toUnqualifiedVersionless(); + } + + // Add two with a period + IIdType obsId3; + { + Observation obs = new Observation(); + obs.setEffective(new Period().setStartElement(new DateTimeType("2000-06-06T12:00:00Z")).setEndElement(new DateTimeType("2001-06-06T12:00:00Z"))); + obsId3 = myObservationDao.create(obs).getId().toUnqualifiedVersionless(); + } + IIdType obsId4; + { + Observation obs = new Observation(); + obs.setEffective(new Period().setStartElement(new DateTimeType("2001-01-01T12:00:00Z")).setEndElement(new DateTimeType("2002-01-01T12:00:00Z"))); + obsId4 = myObservationDao.create(obs).getId().toUnqualifiedVersionless(); + } + + // Two AND instances of 1 SP + { + myCaptureQueriesListener.clear(); + SearchParameterMap params = new SearchParameterMap(); + params.setLoadSynchronous(true); + params.add("issued", new DateParam("ge2020-06-05")); + params.add("issued", new DateParam("lt2020-06-07")); + List patients = toUnqualifiedVersionlessIds(myObservationDao.search(params)); + assertThat(patients.toString(), patients, contains(obsId1)); + String searchQuery = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, true); + ourLog.info("Search query:\n{}", searchQuery); + assertEquals(searchQuery, 1, StringUtils.countMatches(searchQuery.toLowerCase(), "join")); + assertEquals(searchQuery, 1, StringUtils.countMatches(searchQuery.toLowerCase(), "hash_identity")); + assertEquals(searchQuery, 2, StringUtils.countMatches(searchQuery.toLowerCase(), "sp_value_low")); + } + + // Two AND instances of 1 SP and 1 instance of another + { + myCaptureQueriesListener.clear(); + SearchParameterMap params = new SearchParameterMap(); + params.setLoadSynchronous(true); + params.add("issued", new DateParam("ge2020-06-05")); + params.add("issued", new DateParam("lt2020-06-07")); + params.add("date", new DateParam("gt2019-06-05")); + params.add("date", new DateParam("lt2019-06-07")); + List patients = toUnqualifiedVersionlessIds(myObservationDao.search(params)); + assertThat(patients.toString(), patients, contains(obsId1)); + String searchQuery = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, true); + ourLog.info("Search query:\n{}", searchQuery); + assertEquals(searchQuery, 2, StringUtils.countMatches(searchQuery.toLowerCase(), "join")); + assertEquals(searchQuery, 2, StringUtils.countMatches(searchQuery.toLowerCase(), "hash_identity")); + assertEquals(searchQuery, 4, StringUtils.countMatches(searchQuery.toLowerCase(), "sp_value_low")); + } + + // Period search + { + myCaptureQueriesListener.clear(); + SearchParameterMap params = new SearchParameterMap(); + params.setLoadSynchronous(true); + params.add("date", new DateParam("lt2002-01-01T12:00:00Z")); + List patients = toUnqualifiedVersionlessIds(myObservationDao.search(params)); + assertThat(patients.toString(), patients, containsInAnyOrder(obsId3, obsId4)); + String searchQuery = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, true); + ourLog.info("Search query:\n{}", searchQuery); + assertEquals(searchQuery, 1, StringUtils.countMatches(searchQuery.toLowerCase(), "join")); + assertEquals(searchQuery, 1, StringUtils.countMatches(searchQuery.toLowerCase(), "hash_identity")); + assertEquals(searchQuery, 1, StringUtils.countMatches(searchQuery.toLowerCase(), "sp_value_low")); + } + + } + + + @Test public void testSearchWithFetchSizeDefaultMaximum() { myDaoConfig.setFetchSizeDefaultMaximum(5); diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/ISchemaInitializationProvider.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/ISchemaInitializationProvider.java index 85d2af206bc..1be30796e25 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/ISchemaInitializationProvider.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/ISchemaInitializationProvider.java @@ -21,16 +21,16 @@ package ca.uhn.fhir.jpa.migrate.tasks.api; */ import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; -import ca.uhn.fhir.jpa.migrate.tasks.SchemaInitializationProvider; import java.util.List; public interface ISchemaInitializationProvider { + List getSqlStatements(DriverTypeEnum theDriverType); String getSchemaExistsIndicatorTable(); String getSchemaDescription(); - SchemaInitializationProvider setSchemaDescription(String theSchemaDescription); + ISchemaInitializationProvider setSchemaDescription(String theSchemaDescription); } diff --git a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/InitializeSchemaTaskTest.java b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/InitializeSchemaTaskTest.java index 1247c332246..c65bcb06006 100644 --- a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/InitializeSchemaTaskTest.java +++ b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/InitializeSchemaTaskTest.java @@ -45,6 +45,11 @@ public class InitializeSchemaTaskTest extends BaseTest { return "TEST"; } + @Override + public ISchemaInitializationProvider setSchemaDescription(String theSchemaDescription) { + return this; + } + @Override public boolean equals(Object theO) { if (this == theO) return true;