Collapse date joins (#1726)

* Collapse date joins

* Add changelog

* Address review comments

* Add whitespace

* fix test

Co-authored-by: Ken Stevens <ken.stevens@sympatico.ca>

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.

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<String, Join<ResourceTable, ResourceIndexedSearchParamDate>> myJoinMap;
+
 	PredicateBuilderDate(SearchBuilder theSearchBuilder) {
 		super(theSearchBuilder);
 	}
@@ -56,7 +60,18 @@ public class PredicateBuilderDate extends BasePredicateBuilder implements IPredi
 											List<? extends IQueryParameterType> theList,
 											SearchFilterParser.CompareOperation operation) {
 
-		Join<ResourceTable, ResourceIndexedSearchParamDate> join = createJoin(SearchBuilderJoinEnum.DATE, theParamName);
+		boolean newJoin = false;
+		if (myJoinMap == null) {
+			myJoinMap = new HashMap<>();
+		}
+		String key = theResourceName + " " + theParamName;
+
+		Join<ResourceTable, ResourceIndexedSearchParamDate> 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<?, ResourceIndexedSearchParamDate> 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<?, ResourceIndexedSearchParamDate> 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,

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<IIdType> 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<IIdType> 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<IIdType> 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);

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<String> getSqlStatements(DriverTypeEnum theDriverType);
 
 	String getSchemaExistsIndicatorTable();
 
     String getSchemaDescription();
 
-	SchemaInitializationProvider setSchemaDescription(String theSchemaDescription);
+	ISchemaInitializationProvider setSchemaDescription(String theSchemaDescription);
 }

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;