Skip to content

Commit 38d5647

Browse files
lmsurprelmsurpre
authored
Merge pull request #419 from IBM/issue-410
issue #418 - add order by clause to searchByIds to ensure sorted results
2 parents e19202a + 1399a1a commit 38d5647

6 files changed

Lines changed: 70 additions & 140 deletions

File tree

‎fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/JDBCConstants.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,9 @@ public class JDBCConstants {
4141
public static final String ESCAPE_UNDERSCORE = ESCAPE_CHAR + "_";
4242
public static final String ESCAPE_PERCENT = ESCAPE_CHAR + PERCENT_WILDCARD;
4343
public static final String ESCAPE_EXPR = " ESCAPE '" + ESCAPE_CHAR + "'";
44+
public static final String WHEN = " WHEN ";
45+
public static final String THEN = " THEN ";
46+
public static final String END = " END";
4447

4548
/**
4649
* Maps Parameter modifiers to SQL operators.

‎fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/api/ResourceDAO.java‎

Lines changed: 4 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -118,19 +118,18 @@ List<Resource> search(String sqlSelect)
118118
* @throws FHIRPersistenceDataAccessException
119119
* @throws FHIRPersistenceDBConnectException
120120
*/
121-
List<Long> searchForIds(String sqlSelect)
122-
throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException;
121+
List<Long> searchForIds(SqlQueryData queryData) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException;
123122

124123
/**
125124
* Searches for Resources that contain one of the passed ids.
125+
* @param resourceType - The type of the FHIR Resource
126126
* @param resourceIds - A List of resource ids.
127127
* @return List<Resource> - A List of resources matching the the passed list of ids.
128128
* @throws FHIRPersistenceDataAccessException
129129
* @throws FHIRPersistenceDBConnectException
130130
*/
131-
List<Resource> searchByIds(List<Long> resourceIds)
132-
throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException;
133-
131+
List<Resource> searchByIds(String resourceType, List<Long> resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException;
132+
134133
/**
135134
* Executes a count query based on the data contained in the passed SqlQueryData, using it's encapsulated search string and bind variables.
136135
* @param queryData - Contains a search string and (optionally) bind variables.
@@ -173,27 +172,6 @@ List<Resource> searchByIds(List<Long> resourceIds)
173172
* @throws FHIRPersistenceDataAccessException
174173
*/
175174
Integer readResourceTypeId(String parameterName) throws FHIRPersistenceDBConnectException, FHIRPersistenceDataAccessException;
176-
177-
/**
178-
* This method supports the execution of a specialized query designed to return Resource ids, based on the contents
179-
* of the passed select statement.
180-
* Note that the first column to be selected MUST be the Resource.id column.
181-
* @param sqlSelect - A select for Resource ids.
182-
* @return - A List of resource ids that satisfy the passed SQL query.
183-
* @throws FHIRPersistenceDataAccessException
184-
* @throws FHIRPersistenceDBConnectException
185-
*/
186-
List<Long> searchForIds(SqlQueryData queryData) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException;
187-
188-
/**
189-
* Searches for Resources that contain one of the passed ids.
190-
* @param resourceType - The type of the FHIR Resource.
191-
* @param resourceIds - A List of resource ids.
192-
* @return List<Resource> - A List of resources matching the the passed list of ids.
193-
* @throws FHIRPersistenceDataAccessException
194-
* @throws FHIRPersistenceDBConnectException
195-
*/
196-
List<Resource> searchByIds(String resourceType, List<Long> resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException;
197175

198176
/**
199177
* Adds a resource type / resource id pair to a candidate collection for population into the ResourceTypesCache.

‎fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/impl/ResourceDAOImpl.java‎

Lines changed: 32 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66

77
package com.ibm.fhir.persistence.jdbc.dao.impl;
88

9+
import static com.ibm.fhir.persistence.jdbc.JDBCConstants.*;
10+
911
import java.sql.CallableStatement;
1012
import java.sql.Connection;
1113
import java.sql.PreparedStatement;
@@ -90,6 +92,8 @@ public class ResourceDAOImpl extends FHIRDbDAOImpl implements ResourceDAO {
9092
"FROM %s_RESOURCES R, %s_LOGICAL_RESOURCES LR WHERE R.LOGICAL_RESOURCE_ID = LR.LOGICAL_RESOURCE_ID AND " +
9193
"R.RESOURCE_ID IN ";
9294

95+
private static final String SQL_ORDER_BY_IDS = "ORDER BY CASE R.RESOURCE_ID ";
96+
9397
private static final String DERBY_PAGINATION_PARMS = "OFFSET ? ROWS FETCH NEXT ? ROWS ONLY";
9498

9599
private static final String DB2_PAGINATION_PARMS = "LIMIT ? OFFSET ?";
@@ -418,64 +422,7 @@ public List<Long> searchForIds(SqlQueryData queryData) throws FHIRPersistenceDat
418422
return resourceIds;
419423
}
420424

421-
@Override
422-
public List<Resource> searchByIds(String resourceType, List<Long> resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException {
423-
final String METHODNAME = "searchByIds";
424-
log.entering(CLASSNAME, METHODNAME);
425-
426-
if (resourceIds.isEmpty()) {
427-
return Collections.emptyList();
428-
}
429-
430-
Connection connection = null;
431-
PreparedStatement stmt = null;
432-
ResultSet resultSet = null;
433-
String errMsg;
434-
StringBuilder idQuery = new StringBuilder();
435-
List<Resource> resources = new ArrayList<>();
436-
String stmtString = null;
437-
long dbCallStartTime;
438-
double dbCallDuration;
439-
440-
try {
441-
stmtString = String.format(this.getSearchByIdsSql(resourceType));
442-
idQuery.append(stmtString);
443-
idQuery.append("(");
444-
for (int i = 0; i < resourceIds.size(); i++) {
445-
if (i > 0) {
446-
idQuery.append(",");
447-
}
448-
idQuery.append(resourceIds.get(i));
449-
}
450-
idQuery.append(")");
451-
452-
connection = this.getConnection();
453-
stmt = connection.prepareStatement(idQuery.toString());
454-
dbCallStartTime = System.nanoTime();
455-
resultSet = stmt.executeQuery();
456-
dbCallDuration = (System.nanoTime()-dbCallStartTime)/1e6;
457-
if (log.isLoggable(Level.FINE)) {
458-
log.fine("DB search by ids complete. SQL=[" + idQuery + "] executionTime=" + dbCallDuration + "ms");
459-
}
460-
resources = this.createDTOs(resultSet);
461-
} catch(FHIRPersistenceException e) {
462-
throw e;
463-
} catch (Throwable e) {
464-
FHIRPersistenceDataAccessException fx = new FHIRPersistenceDataAccessException("Failure retrieving FHIR Resources");
465-
errMsg = "Failure retrieving FHIR Resources. SQL=[" + idQuery + "]";
466-
throw severe(log, fx, errMsg, e);
467-
} finally {
468-
this.cleanup(resultSet, stmt, connection);
469-
log.exiting(CLASSNAME, METHODNAME);
470-
}
471-
return resources;
472-
}
473-
474-
protected String getSearchByIdsSql(String resourceType) {
475-
String stmtString;
476-
stmtString = String.format(SQL_SEARCH_BY_IDS, resourceType, resourceType);
477-
return stmtString;
478-
}
425+
479426

480427
/**
481428
* Adds a resource type/ resource id pair to a candidate collection for population into the ResourceTypesCache.
@@ -734,78 +681,55 @@ public List<Resource> search(String sqlSelect) throws FHIRPersistenceDataAccessE
734681
}
735682

736683
@Override
737-
public List<Long> searchForIds(String sqlSelect) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException {
738-
final String METHODNAME = "searchForIds";
739-
log.entering(CLASSNAME, METHODNAME);
740-
741-
List<Long> resourceIds = new ArrayList<>();
742-
Connection connection = null;
743-
PreparedStatement stmt = null;
744-
ResultSet resultSet = null;
745-
String errMsg;
746-
747-
try {
748-
connection = this.getConnection();
749-
stmt = connection.prepareStatement(sqlSelect);
750-
resultSet = stmt.executeQuery();
751-
while (resultSet.next()) {
752-
resourceIds.add(resultSet.getLong(1));
753-
}
754-
} catch(FHIRPersistenceException e) {
755-
throw e;
756-
} catch (Throwable e) {
757-
// Log the SQL but don't expose it in the exception
758-
FHIRPersistenceDataAccessException fx = new FHIRPersistenceDataAccessException("Failure retrieving FHIR Resource Ids.");
759-
errMsg = "Failure retrieving FHIR Resource Ids. SQL=" + sqlSelect;
760-
throw severe(log, fx, errMsg, e);
761-
} finally {
762-
this.cleanup(resultSet, stmt, connection);
763-
log.exiting(CLASSNAME, METHODNAME);
764-
}
765-
return resourceIds;
766-
}
767-
768-
@Override
769-
public List<Resource> searchByIds(List<Long> resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException {
684+
public List<Resource> searchByIds(String resourceType, List<Long> resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException {
770685
final String METHODNAME = "searchByIds";
771686
log.entering(CLASSNAME, METHODNAME);
772687

773688
if (resourceIds.isEmpty()) {
774689
return Collections.emptyList();
775690
}
776-
691+
777692
Connection connection = null;
778693
PreparedStatement stmt = null;
779694
ResultSet resultSet = null;
780695
String errMsg;
781696
StringBuilder idQuery = new StringBuilder();
782697
List<Resource> resources = new ArrayList<>();
783-
698+
String stmtString = null;
699+
long dbCallStartTime;
700+
double dbCallDuration;
701+
784702
try {
785-
idQuery.append(SQL_SEARCH_BY_IDS);
703+
stmtString = getSearchByIdsSql(resourceType);
704+
idQuery.append(stmtString);
786705
idQuery.append("(");
706+
// resourceIds should have a max length of 1000 (the max page size)
707+
StringBuilder caseStmts = new StringBuilder();
787708
for (int i = 0; i < resourceIds.size(); i++) {
788709
if (i > 0) {
789710
idQuery.append(",");
790711
}
791-
idQuery.append("?");
712+
idQuery.append(resourceIds.get(i));
713+
714+
// build up the caseStmts here so we only need to iterate the list once
715+
caseStmts.append(WHEN + resourceIds.get(i) + THEN + i);
792716
}
793-
idQuery.append(")");
794-
717+
idQuery.append(") " + SQL_ORDER_BY_IDS + caseStmts + END);
718+
795719
connection = this.getConnection();
796720
stmt = connection.prepareStatement(idQuery.toString());
797-
// Inject IDs into the prepared stmt.
798-
for (int i = 0; i < resourceIds.size(); i++) {
799-
stmt.setObject(i+1, resourceIds.get(i));
800-
}
721+
dbCallStartTime = System.nanoTime();
801722
resultSet = stmt.executeQuery();
723+
dbCallDuration = (System.nanoTime()-dbCallStartTime)/1e6;
724+
if (log.isLoggable(Level.FINE)) {
725+
log.fine("DB search by ids complete. SQL=[" + idQuery + "] executionTime=" + dbCallDuration + "ms");
726+
}
802727
resources = this.createDTOs(resultSet);
803728
} catch(FHIRPersistenceException e) {
804729
throw e;
805730
} catch (Throwable e) {
806-
// Log the SQL but don't expose it in the exception
807-
FHIRPersistenceDataAccessException fx = new FHIRPersistenceDataAccessException("Failure retrieving FHIR Resources.");
808-
errMsg = "Failure retrieving FHIR Resources. SQL=" + idQuery;
731+
FHIRPersistenceDataAccessException fx = new FHIRPersistenceDataAccessException("Failure retrieving FHIR Resources");
732+
errMsg = "Failure retrieving FHIR Resources. SQL=[" + idQuery + "]";
809733
throw severe(log, fx, errMsg, e);
810734
} finally {
811735
this.cleanup(resultSet, stmt, connection);
@@ -814,6 +738,10 @@ public List<Resource> searchByIds(List<Long> resourceIds) throws FHIRPersistence
814738
return resources;
815739
}
816740

741+
protected String getSearchByIdsSql(String resourceType) {
742+
return String.format(SQL_SEARCH_BY_IDS, resourceType, resourceType);
743+
}
744+
817745
@Override
818746
public int searchCount(String sqlSelectCount) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException {
819747
final String METHODNAME = "searchCount";

‎fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/util/SortedQuerySegmentAggregator.java‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -57,15 +57,14 @@ protected SortedQuerySegmentAggregator(Class<?> resourceType, int offset, int pa
5757
* <p>
5858
* A simple example query produced by this method:
5959
* <pre>
60-
* SELECT R.RESOURCE_ID,MIN(S1.STR_VALUE) FROM
61-
* Patient_RESOURCES R JOIN
62-
* Patient_LOGICAL_RESOURCES LR ON R.LOGICAL_RESOURCE_ID=LR.LOGICAL_RESOURCE_ID JOIN
63-
* Patient_TOKEN_VALUES P1 ON P1.RESOURCE_ID=R.RESOURCE_ID
64-
* LEFT OUTER JOIN Patient_STR_VALUES S1 ON (S1.PARAMETER_NAME_ID=50 AND S1.RESOURCE_ID = R.RESOURCE_ID) WHERE
65-
* R.RESOURCE_ID = LR.CURRENT_RESOURCE_ID AND
66-
* R.IS_DELETED <> 'Y' AND
67-
* P1.RESOURCE_ID = R.RESOURCE_ID AND
68-
* (P1.PARAMETER_NAME_ID=196 AND ((P1.TOKEN_VALUE = false)))
60+
* SELECT R.RESOURCE_ID,MIN(S1.STR_VALUE) FROM Patient_RESOURCES R
61+
* JOIN Patient_LOGICAL_RESOURCES LR ON R.LOGICAL_RESOURCE_ID=LR.LOGICAL_RESOURCE_ID
62+
* JOIN Patient_TOKEN_VALUES P1 ON P1.RESOURCE_ID=R.RESOURCE_ID
63+
* LEFT OUTER JOIN Patient_STR_VALUES S1 ON (S1.PARAMETER_NAME_ID=50 AND S1.RESOURCE_ID = R.RESOURCE_ID)
64+
* WHERE R.RESOURCE_ID = LR.CURRENT_RESOURCE_ID AND
65+
* R.IS_DELETED <> 'Y' AND
66+
* P1.RESOURCE_ID = R.RESOURCE_ID AND
67+
* (P1.PARAMETER_NAME_ID=196 AND ((P1.TOKEN_VALUE = false)))
6968
* GROUP BY R.RESOURCE_ID
7069
* ORDER BY MIN(S1.STR_VALUE) asc NULLS LAST
7170
* OFFSET 0 ROWS FETCH NEXT 100 ROWS ONLY;

‎fhir-server-test/src/test/java/com/ibm/fhir/server/test/SearchPerformaceTest.java‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import static org.testng.Assert.assertTrue;
1616

1717
import java.lang.reflect.Method;
18+
import java.math.BigDecimal;
1819
import java.util.List;
1920

2021
import javax.ws.rs.client.Entity;
@@ -27,9 +28,14 @@
2728
import com.ibm.fhir.model.resource.Bundle;
2829
import com.ibm.fhir.model.resource.Observation;
2930
import com.ibm.fhir.model.resource.Patient;
31+
import com.ibm.fhir.model.resource.Observation.Component;
3032
import com.ibm.fhir.model.test.TestUtil;
3133
import com.ibm.fhir.model.type.Code;
34+
import com.ibm.fhir.model.type.CodeableConcept;
3235
import com.ibm.fhir.model.type.Coding;
36+
import com.ibm.fhir.model.type.Decimal;
37+
import com.ibm.fhir.model.type.Quantity;
38+
import com.ibm.fhir.model.type.Uri;
3339
import com.ibm.fhir.model.type.code.AdministrativeGender;
3440
import com.ibm.fhir.model.util.FHIRUtil;
3541

@@ -44,7 +50,7 @@ public class SearchPerformaceTest extends FHIRServerTestBase {
4450
// Controls how many observations and patients to create for the test.
4551
// Using invocationCount of testng can cause the testng report grows too big, so
4652
// this config is used to make sure all test users are created in one testng step.
47-
private final int numOfPatientObservationsToCreate = 100;
53+
private final int numOfPatientObservationsToCreate = 1000;
4854

4955
/**
5056
* Retrieve the server's conformance statement to determine the status of certain runtime options.
@@ -87,6 +93,21 @@ public void testCreatePatientAndObservation() throws Exception {
8793
// create observation for the patient
8894
Observation observation =
8995
TestUtil.buildPatientObservation(patientId, "Observation1.json");
96+
observation = observation.toBuilder()
97+
.code(CodeableConcept.builder()
98+
.coding(Coding.builder()
99+
.system(Uri.of("http://loinc.org"))
100+
.code(Code.of("55284-4"))
101+
.build())
102+
.build())
103+
.component(Component.builder()
104+
.code(CodeableConcept.builder().text(string("component1")).build())
105+
.value(Quantity.builder()
106+
.value(Decimal.of(BigDecimal.valueOf(Math.random())))
107+
.unit(string("mmHg"))
108+
.build())
109+
.build())
110+
.build();
90111
Entity<Observation> entity2 =
91112
Entity.entity(observation, FHIRMediaType.APPLICATION_FHIR_JSON);
92113
response =

‎fhir-server-test/src/test/java/com/ibm/fhir/server/test/SortingTest.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -674,6 +674,7 @@ private void assertTrueNaturalOrdering(List<BigDecimal> sortedList) {
674674
}
675675

676676
assertTrue(prior.compareTo(current) <= 0);
677+
prior = current;
677678
}
678679
done = true;
679680
}

0 commit comments

Comments
 (0)