From 1399a1a22db923bbf8f8715b0b058f332019f5d5 Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Thu, 14 Nov 2019 23:38:48 -0500 Subject: [PATCH] issue #418 - add order by clause to searchByIds to ensure sorted results 1. Updated the SearchPerformanceTest so that the observations have components with values...just to easily create many observations which helps verify the search behavior. This change could be removed if we want. 2. Deleted unused variants of searchForIds and searchByIds. I think these came from the original "basic" schema that was removed for https://github.com/IBM/FHIR/issues/93 but they were no longer in use and should be safe to delete. 3. Introduced ORDER BY CASE clause to ResourceDAOImpl.searchByIds. I found that DERBY is always returning the results in the same order as the resource ids provided in the "IN" clause, but in general we shouldn't rely on that. I tested the CASE statements on derby with up to 1000 results, but we need to run a similar test on Db2 to ensure it works there as well. Signed-off-by: Lee Surprenant --- .../fhir/persistence/jdbc/JDBCConstants.java | 3 + .../persistence/jdbc/dao/api/ResourceDAO.java | 30 +--- .../jdbc/dao/impl/ResourceDAOImpl.java | 136 +++++------------- .../util/SortedQuerySegmentAggregator.java | 17 ++- .../server/test/SearchPerformaceTest.java | 23 ++- .../com/ibm/fhir/server/test/SortingTest.java | 1 + 6 files changed, 70 insertions(+), 140 deletions(-) diff --git a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/JDBCConstants.java b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/JDBCConstants.java index 20b0338137c..82998d43626 100644 --- a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/JDBCConstants.java +++ b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/JDBCConstants.java @@ -41,6 +41,9 @@ public class JDBCConstants { public static final String ESCAPE_UNDERSCORE = ESCAPE_CHAR + "_"; public static final String ESCAPE_PERCENT = ESCAPE_CHAR + PERCENT_WILDCARD; public static final String ESCAPE_EXPR = " ESCAPE '" + ESCAPE_CHAR + "'"; + public static final String WHEN = " WHEN "; + public static final String THEN = " THEN "; + public static final String END = " END"; /** * Maps Parameter modifiers to SQL operators. diff --git a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/api/ResourceDAO.java b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/api/ResourceDAO.java index 1eba8cd9a23..dd7320ad6ad 100644 --- a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/api/ResourceDAO.java +++ b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/api/ResourceDAO.java @@ -118,19 +118,18 @@ List search(String sqlSelect) * @throws FHIRPersistenceDataAccessException * @throws FHIRPersistenceDBConnectException */ - List searchForIds(String sqlSelect) - throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException; + List searchForIds(SqlQueryData queryData) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException; /** * Searches for Resources that contain one of the passed ids. + * @param resourceType - The type of the FHIR Resource * @param resourceIds - A List of resource ids. * @return List - A List of resources matching the the passed list of ids. * @throws FHIRPersistenceDataAccessException * @throws FHIRPersistenceDBConnectException */ - List searchByIds(List resourceIds) - throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException; - + List searchByIds(String resourceType, List resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException; + /** * Executes a count query based on the data contained in the passed SqlQueryData, using it's encapsulated search string and bind variables. * @param queryData - Contains a search string and (optionally) bind variables. @@ -173,27 +172,6 @@ List searchByIds(List resourceIds) * @throws FHIRPersistenceDataAccessException */ Integer readResourceTypeId(String parameterName) throws FHIRPersistenceDBConnectException, FHIRPersistenceDataAccessException; - - /** - * This method supports the execution of a specialized query designed to return Resource ids, based on the contents - * of the passed select statement. - * Note that the first column to be selected MUST be the Resource.id column. - * @param sqlSelect - A select for Resource ids. - * @return - A List of resource ids that satisfy the passed SQL query. - * @throws FHIRPersistenceDataAccessException - * @throws FHIRPersistenceDBConnectException - */ - List searchForIds(SqlQueryData queryData) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException; - - /** - * Searches for Resources that contain one of the passed ids. - * @param resourceType - The type of the FHIR Resource. - * @param resourceIds - A List of resource ids. - * @return List - A List of resources matching the the passed list of ids. - * @throws FHIRPersistenceDataAccessException - * @throws FHIRPersistenceDBConnectException - */ - List searchByIds(String resourceType, List resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException; /** * Adds a resource type / resource id pair to a candidate collection for population into the ResourceTypesCache. diff --git a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/impl/ResourceDAOImpl.java b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/impl/ResourceDAOImpl.java index 4483aa32b4b..9e20cc2106e 100644 --- a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/impl/ResourceDAOImpl.java +++ b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/impl/ResourceDAOImpl.java @@ -6,6 +6,8 @@ package com.ibm.fhir.persistence.jdbc.dao.impl; +import static com.ibm.fhir.persistence.jdbc.JDBCConstants.*; + import java.sql.CallableStatement; import java.sql.Connection; import java.sql.PreparedStatement; @@ -90,6 +92,8 @@ public class ResourceDAOImpl extends FHIRDbDAOImpl implements ResourceDAO { "FROM %s_RESOURCES R, %s_LOGICAL_RESOURCES LR WHERE R.LOGICAL_RESOURCE_ID = LR.LOGICAL_RESOURCE_ID AND " + "R.RESOURCE_ID IN "; + private static final String SQL_ORDER_BY_IDS = "ORDER BY CASE R.RESOURCE_ID "; + private static final String DERBY_PAGINATION_PARMS = "OFFSET ? ROWS FETCH NEXT ? ROWS ONLY"; private static final String DB2_PAGINATION_PARMS = "LIMIT ? OFFSET ?"; @@ -418,64 +422,7 @@ public List searchForIds(SqlQueryData queryData) throws FHIRPersistenceDat return resourceIds; } - @Override - public List searchByIds(String resourceType, List resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException { - final String METHODNAME = "searchByIds"; - log.entering(CLASSNAME, METHODNAME); - - if (resourceIds.isEmpty()) { - return Collections.emptyList(); - } - - Connection connection = null; - PreparedStatement stmt = null; - ResultSet resultSet = null; - String errMsg; - StringBuilder idQuery = new StringBuilder(); - List resources = new ArrayList<>(); - String stmtString = null; - long dbCallStartTime; - double dbCallDuration; - - try { - stmtString = String.format(this.getSearchByIdsSql(resourceType)); - idQuery.append(stmtString); - idQuery.append("("); - for (int i = 0; i < resourceIds.size(); i++) { - if (i > 0) { - idQuery.append(","); - } - idQuery.append(resourceIds.get(i)); - } - idQuery.append(")"); - - connection = this.getConnection(); - stmt = connection.prepareStatement(idQuery.toString()); - dbCallStartTime = System.nanoTime(); - resultSet = stmt.executeQuery(); - dbCallDuration = (System.nanoTime()-dbCallStartTime)/1e6; - if (log.isLoggable(Level.FINE)) { - log.fine("DB search by ids complete. SQL=[" + idQuery + "] executionTime=" + dbCallDuration + "ms"); - } - resources = this.createDTOs(resultSet); - } catch(FHIRPersistenceException e) { - throw e; - } catch (Throwable e) { - FHIRPersistenceDataAccessException fx = new FHIRPersistenceDataAccessException("Failure retrieving FHIR Resources"); - errMsg = "Failure retrieving FHIR Resources. SQL=[" + idQuery + "]"; - throw severe(log, fx, errMsg, e); - } finally { - this.cleanup(resultSet, stmt, connection); - log.exiting(CLASSNAME, METHODNAME); - } - return resources; - } - - protected String getSearchByIdsSql(String resourceType) { - String stmtString; - stmtString = String.format(SQL_SEARCH_BY_IDS, resourceType, resourceType); - return stmtString; - } + /** * Adds a resource type/ resource id pair to a candidate collection for population into the ResourceTypesCache. @@ -734,78 +681,55 @@ public List search(String sqlSelect) throws FHIRPersistenceDataAccessE } @Override - public List searchForIds(String sqlSelect) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException { - final String METHODNAME = "searchForIds"; - log.entering(CLASSNAME, METHODNAME); - - List resourceIds = new ArrayList<>(); - Connection connection = null; - PreparedStatement stmt = null; - ResultSet resultSet = null; - String errMsg; - - try { - connection = this.getConnection(); - stmt = connection.prepareStatement(sqlSelect); - resultSet = stmt.executeQuery(); - while (resultSet.next()) { - resourceIds.add(resultSet.getLong(1)); - } - } catch(FHIRPersistenceException e) { - throw e; - } catch (Throwable e) { - // Log the SQL but don't expose it in the exception - FHIRPersistenceDataAccessException fx = new FHIRPersistenceDataAccessException("Failure retrieving FHIR Resource Ids."); - errMsg = "Failure retrieving FHIR Resource Ids. SQL=" + sqlSelect; - throw severe(log, fx, errMsg, e); - } finally { - this.cleanup(resultSet, stmt, connection); - log.exiting(CLASSNAME, METHODNAME); - } - return resourceIds; - } - - @Override - public List searchByIds(List resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException { + public List searchByIds(String resourceType, List resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException { final String METHODNAME = "searchByIds"; log.entering(CLASSNAME, METHODNAME); if (resourceIds.isEmpty()) { return Collections.emptyList(); } - + Connection connection = null; PreparedStatement stmt = null; ResultSet resultSet = null; String errMsg; StringBuilder idQuery = new StringBuilder(); List resources = new ArrayList<>(); - + String stmtString = null; + long dbCallStartTime; + double dbCallDuration; + try { - idQuery.append(SQL_SEARCH_BY_IDS); + stmtString = getSearchByIdsSql(resourceType); + idQuery.append(stmtString); idQuery.append("("); + // resourceIds should have a max length of 1000 (the max page size) + StringBuilder caseStmts = new StringBuilder(); for (int i = 0; i < resourceIds.size(); i++) { if (i > 0) { idQuery.append(","); } - idQuery.append("?"); + idQuery.append(resourceIds.get(i)); + + // build up the caseStmts here so we only need to iterate the list once + caseStmts.append(WHEN + resourceIds.get(i) + THEN + i); } - idQuery.append(")"); - + idQuery.append(") " + SQL_ORDER_BY_IDS + caseStmts + END); + connection = this.getConnection(); stmt = connection.prepareStatement(idQuery.toString()); - // Inject IDs into the prepared stmt. - for (int i = 0; i < resourceIds.size(); i++) { - stmt.setObject(i+1, resourceIds.get(i)); - } + dbCallStartTime = System.nanoTime(); resultSet = stmt.executeQuery(); + dbCallDuration = (System.nanoTime()-dbCallStartTime)/1e6; + if (log.isLoggable(Level.FINE)) { + log.fine("DB search by ids complete. SQL=[" + idQuery + "] executionTime=" + dbCallDuration + "ms"); + } resources = this.createDTOs(resultSet); } catch(FHIRPersistenceException e) { throw e; } catch (Throwable e) { - // Log the SQL but don't expose it in the exception - FHIRPersistenceDataAccessException fx = new FHIRPersistenceDataAccessException("Failure retrieving FHIR Resources."); - errMsg = "Failure retrieving FHIR Resources. SQL=" + idQuery; + FHIRPersistenceDataAccessException fx = new FHIRPersistenceDataAccessException("Failure retrieving FHIR Resources"); + errMsg = "Failure retrieving FHIR Resources. SQL=[" + idQuery + "]"; throw severe(log, fx, errMsg, e); } finally { this.cleanup(resultSet, stmt, connection); @@ -814,6 +738,10 @@ public List searchByIds(List resourceIds) throws FHIRPersistence return resources; } + protected String getSearchByIdsSql(String resourceType) { + return String.format(SQL_SEARCH_BY_IDS, resourceType, resourceType); + } + @Override public int searchCount(String sqlSelectCount) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException { final String METHODNAME = "searchCount"; diff --git a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/util/SortedQuerySegmentAggregator.java b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/util/SortedQuerySegmentAggregator.java index cdc03c70490..bed4442dc60 100644 --- a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/util/SortedQuerySegmentAggregator.java +++ b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/util/SortedQuerySegmentAggregator.java @@ -57,15 +57,14 @@ protected SortedQuerySegmentAggregator(Class resourceType, int offset, int pa *

* A simple example query produced by this method: * 

-     * SELECT R.RESOURCE_ID,MIN(S1.STR_VALUE) FROM 
-     * Patient_RESOURCES R JOIN 
-     * Patient_LOGICAL_RESOURCES LR ON R.LOGICAL_RESOURCE_ID=LR.LOGICAL_RESOURCE_ID  JOIN 
-     * Patient_TOKEN_VALUES P1 ON P1.RESOURCE_ID=R.RESOURCE_ID  
-     * LEFT OUTER JOIN Patient_STR_VALUES S1 ON (S1.PARAMETER_NAME_ID=50 AND S1.RESOURCE_ID = R.RESOURCE_ID) WHERE 
-     * R.RESOURCE_ID = LR.CURRENT_RESOURCE_ID AND 
-     * R.IS_DELETED <> 'Y' AND 
-     * P1.RESOURCE_ID = R.RESOURCE_ID AND 
-     * (P1.PARAMETER_NAME_ID=196 AND ((P1.TOKEN_VALUE = false))) 
+     * SELECT R.RESOURCE_ID,MIN(S1.STR_VALUE) FROM Patient_RESOURCES R 
+     *   JOIN Patient_LOGICAL_RESOURCES LR ON R.LOGICAL_RESOURCE_ID=LR.LOGICAL_RESOURCE_ID
+     *   JOIN Patient_TOKEN_VALUES P1 ON P1.RESOURCE_ID=R.RESOURCE_ID  
+     *   LEFT OUTER JOIN Patient_STR_VALUES S1 ON (S1.PARAMETER_NAME_ID=50 AND S1.RESOURCE_ID = R.RESOURCE_ID)
+     *   WHERE R.RESOURCE_ID = LR.CURRENT_RESOURCE_ID AND 
+     *         R.IS_DELETED <> 'Y' AND 
+     *         P1.RESOURCE_ID = R.RESOURCE_ID AND 
+     *         (P1.PARAMETER_NAME_ID=196 AND ((P1.TOKEN_VALUE = false))) 
      * GROUP BY R.RESOURCE_ID  
      * ORDER BY MIN(S1.STR_VALUE) asc NULLS LAST 
      * OFFSET 0 ROWS FETCH NEXT 100 ROWS ONLY;
diff --git a/fhir-server-test/src/test/java/com/ibm/fhir/server/test/SearchPerformaceTest.java b/fhir-server-test/src/test/java/com/ibm/fhir/server/test/SearchPerformaceTest.java
index a439e316d8a..66d60723b44 100644
--- a/fhir-server-test/src/test/java/com/ibm/fhir/server/test/SearchPerformaceTest.java
+++ b/fhir-server-test/src/test/java/com/ibm/fhir/server/test/SearchPerformaceTest.java
@@ -15,6 +15,7 @@
 import static org.testng.Assert.assertTrue;
 
 import java.lang.reflect.Method;
+import java.math.BigDecimal;
 import java.util.List;
 
 import javax.ws.rs.client.Entity;
@@ -27,9 +28,14 @@
 import com.ibm.fhir.model.resource.Bundle;
 import com.ibm.fhir.model.resource.Observation;
 import com.ibm.fhir.model.resource.Patient;
+import com.ibm.fhir.model.resource.Observation.Component;
 import com.ibm.fhir.model.test.TestUtil;
 import com.ibm.fhir.model.type.Code;
+import com.ibm.fhir.model.type.CodeableConcept;
 import com.ibm.fhir.model.type.Coding;
+import com.ibm.fhir.model.type.Decimal;
+import com.ibm.fhir.model.type.Quantity;
+import com.ibm.fhir.model.type.Uri;
 import com.ibm.fhir.model.type.code.AdministrativeGender;
 import com.ibm.fhir.model.util.FHIRUtil;
 
@@ -44,7 +50,7 @@ public class SearchPerformaceTest extends FHIRServerTestBase {
     // Controls how many observations and patients to create for the test.
     // Using invocationCount of testng can cause the testng report grows too big, so
     // this config is used to make sure all test users are created in one testng step.
-    private final int numOfPatientObservationsToCreate = 100;
+    private final int numOfPatientObservationsToCreate = 1000;
 
     /**
      * Retrieve the server's conformance statement to determine the status of certain runtime options.
@@ -87,6 +93,21 @@ public void testCreatePatientAndObservation() throws Exception {
             // create observation for the patient
             Observation observation =
                     TestUtil.buildPatientObservation(patientId, "Observation1.json");
+            observation = observation.toBuilder()
+                    .code(CodeableConcept.builder()
+                            .coding(Coding.builder()
+                                    .system(Uri.of("http://loinc.org"))
+                                    .code(Code.of("55284-4"))
+                                    .build())
+                            .build())
+                    .component(Component.builder()
+                            .code(CodeableConcept.builder().text(string("component1")).build())
+                            .value(Quantity.builder()
+                                    .value(Decimal.of(BigDecimal.valueOf(Math.random())))
+                                    .unit(string("mmHg"))
+                                    .build())
+                            .build())
+                    .build();
             Entity entity2 =
                     Entity.entity(observation, FHIRMediaType.APPLICATION_FHIR_JSON);
             response =
diff --git a/fhir-server-test/src/test/java/com/ibm/fhir/server/test/SortingTest.java b/fhir-server-test/src/test/java/com/ibm/fhir/server/test/SortingTest.java
index 34f5ec9332d..b402b3047a8 100644
--- a/fhir-server-test/src/test/java/com/ibm/fhir/server/test/SortingTest.java
+++ b/fhir-server-test/src/test/java/com/ibm/fhir/server/test/SortingTest.java
@@ -674,6 +674,7 @@ private void assertTrueNaturalOrdering(List sortedList) {
                 }
 
                 assertTrue(prior.compareTo(current) <= 0);
+                prior = current;
             }
             done = true;
         }