From 63e0c90023a81594db7ec9f24918c9d9a8c0cd05 Mon Sep 17 00:00:00 2001 From: ianmarshall Date: Thu, 24 Dec 2020 13:54:37 -0500 Subject: [PATCH 1/4] Change order by clause in search query to support MySQL, MSSQL and MariaDB. --- .../fhir/jpa/search/builder/QueryStack.java | 24 +-- .../builder/sql/SearchQueryBuilder.java | 95 +++++++++- .../sql/SearchQueryBuilderMySqlTest.java | 179 ++++++++++++++++++ 3 files changed, 283 insertions(+), 15 deletions(-) create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java index 6970081e753..2d29853630a 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java @@ -159,7 +159,7 @@ public class QueryStack { Condition hashIdentityPredicate = sortPredicateBuilder.createHashIdentityPredicate(theResourceName, theParamName); mySqlBuilder.addPredicate(hashIdentityPredicate); - mySqlBuilder.addSort(sortPredicateBuilder.getColumnValueLow(), theAscending); + mySqlBuilder.addSortDate(sortPredicateBuilder.getColumnValueLow(), theAscending); } public void addSortOnLastUpdated(boolean theAscending) { @@ -170,7 +170,7 @@ public class QueryStack { } else { resourceTablePredicateBuilder = mySqlBuilder.addResourceTablePredicateBuilder(firstPredicateBuilder.getResourceIdColumn()); } - mySqlBuilder.addSort(resourceTablePredicateBuilder.getColumnLastUpdated(), theAscending); + mySqlBuilder.addSortDate(resourceTablePredicateBuilder.getColumnLastUpdated(), theAscending); } @@ -180,7 +180,7 @@ public class QueryStack { Condition hashIdentityPredicate = sortPredicateBuilder.createHashIdentityPredicate(theResourceName, theParamName); mySqlBuilder.addPredicate(hashIdentityPredicate); - mySqlBuilder.addSort(sortPredicateBuilder.getColumnValue(), theAscending); + mySqlBuilder.addSortNumeric(sortPredicateBuilder.getColumnValue(), theAscending); } public void addSortOnQuantity(String theResourceName, String theParamName, boolean theAscending) { @@ -189,18 +189,18 @@ public class QueryStack { Condition hashIdentityPredicate = sortPredicateBuilder.createHashIdentityPredicate(theResourceName, theParamName); mySqlBuilder.addPredicate(hashIdentityPredicate); - mySqlBuilder.addSort(sortPredicateBuilder.getColumnValue(), theAscending); + mySqlBuilder.addSortNumeric(sortPredicateBuilder.getColumnValue(), theAscending); } public void addSortOnResourceId(boolean theAscending) { BaseJoiningPredicateBuilder firstPredicateBuilder = mySqlBuilder.getOrCreateFirstPredicateBuilder(); ForcedIdPredicateBuilder sortPredicateBuilder = mySqlBuilder.addForcedIdPredicateBuilder(firstPredicateBuilder.getResourceIdColumn()); if (!theAscending) { - mySqlBuilder.addSort(sortPredicateBuilder.getColumnForcedId(), false, OrderObject.NullOrder.FIRST); + mySqlBuilder.addSortString(sortPredicateBuilder.getColumnForcedId(), false, OrderObject.NullOrder.FIRST); } else { - mySqlBuilder.addSort(sortPredicateBuilder.getColumnForcedId(), true); + mySqlBuilder.addSortString(sortPredicateBuilder.getColumnForcedId(), true); } - mySqlBuilder.addSort(firstPredicateBuilder.getResourceIdColumn(), theAscending); + mySqlBuilder.addSortNumeric(firstPredicateBuilder.getResourceIdColumn(), theAscending); } @@ -210,7 +210,7 @@ public class QueryStack { Condition pathPredicate = sortPredicateBuilder.createPredicateSourcePaths(theResourceName, theParamName); mySqlBuilder.addPredicate(pathPredicate); - mySqlBuilder.addSort(sortPredicateBuilder.getColumnTargetResourceId(), theAscending); + mySqlBuilder.addSortNumeric(sortPredicateBuilder.getColumnTargetResourceId(), theAscending); } @@ -220,7 +220,7 @@ public class QueryStack { Condition hashIdentityPredicate = sortPredicateBuilder.createHashIdentityPredicate(theResourceName, theParamName); mySqlBuilder.addPredicate(hashIdentityPredicate); - mySqlBuilder.addSort(sortPredicateBuilder.getColumnValueNormalized(), theAscending); + mySqlBuilder.addSortString(sortPredicateBuilder.getColumnValueNormalized(), theAscending); } public void addSortOnToken(String theResourceName, String theParamName, boolean theAscending) { @@ -229,8 +229,8 @@ public class QueryStack { Condition hashIdentityPredicate = sortPredicateBuilder.createHashIdentityPredicate(theResourceName, theParamName); mySqlBuilder.addPredicate(hashIdentityPredicate); - mySqlBuilder.addSort(sortPredicateBuilder.getColumnSystem(), theAscending); - mySqlBuilder.addSort(sortPredicateBuilder.getColumnValue(), theAscending); + mySqlBuilder.addSortString(sortPredicateBuilder.getColumnSystem(), theAscending); + mySqlBuilder.addSortString(sortPredicateBuilder.getColumnValue(), theAscending); } public void addSortOnUri(String theResourceName, String theParamName, boolean theAscending) { @@ -239,7 +239,7 @@ public class QueryStack { Condition hashIdentityPredicate = sortPredicateBuilder.createHashIdentityPredicate(theResourceName, theParamName); mySqlBuilder.addPredicate(hashIdentityPredicate); - mySqlBuilder.addSort(sortPredicateBuilder.getColumnValue(), theAscending); + mySqlBuilder.addSortString(sortPredicateBuilder.getColumnValue(), theAscending); } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java index b7c6ea2ab76..e36bd5055b6 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java @@ -67,6 +67,7 @@ import org.slf4j.LoggerFactory; import javax.annotation.Nonnull; import javax.annotation.Nullable; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.List; import java.util.UUID; @@ -94,6 +95,9 @@ public class SearchQueryBuilder { private ResourceTablePredicateBuilder myResourceTableRoot; private boolean myHaveAtLeastOnePredicate; private BaseJoiningPredicateBuilder myFirstPredicateBuilder; + private boolean dialectIsMsSql; + private boolean dialectIsMySql; + private boolean dialectIsMariaDb; /** * Constructor @@ -114,6 +118,16 @@ public class SearchQueryBuilder { mySqlBuilderFactory = theSqlBuilderFactory; myCountQuery = theCountQuery; myDialect = theDialect; + if (myDialect instanceof org.hibernate.dialect.MySQL57Dialect){ + dialectIsMySql = true; + } + if (myDialect instanceof org.hibernate.dialect.MariaDB53Dialect){ + dialectIsMariaDb = true; + } + if (myDialect instanceof org.hibernate.dialect.SQLServer2012Dialect){ + dialectIsMsSql = true; + } + mySpec = new DbSpec(); mySchema = mySpec.addDefaultSchema(); @@ -495,12 +509,87 @@ public class SearchQueryBuilder { return myHaveAtLeastOnePredicate; } - public void addSort(DbColumn theColumnValueNormalized, boolean theAscending) { + public void addSortString(DbColumn theColumnValueNormalized, boolean theAscending) { OrderObject.NullOrder nullOrder = OrderObject.NullOrder.LAST; - addSort(theColumnValueNormalized, theAscending, nullOrder); + addSortString(theColumnValueNormalized, theAscending, nullOrder); } - public void addSort(DbColumn theTheColumnValueNormalized, boolean theTheAscending, OrderObject.NullOrder theNullOrder) { + public void addSortNumeric(DbColumn theColumnValueNormalized, boolean theAscending) { + OrderObject.NullOrder nullOrder = OrderObject.NullOrder.LAST; + addSortNumeric(theColumnValueNormalized, theAscending, nullOrder); + } + + public void addSortDate(DbColumn theColumnValueNormalized, boolean theAscending) { + OrderObject.NullOrder nullOrder = OrderObject.NullOrder.LAST; + addSortDate(theColumnValueNormalized, theAscending, nullOrder); + } + + public void addSortString(DbColumn theTheColumnValueNormalized, boolean theTheAscending, OrderObject.NullOrder theNullOrder) { + if ((dialectIsMariaDb || dialectIsMySql || dialectIsMsSql)) { + // MariaDB, MySQL and MSSQL do not support "NULLS FIRST" and "NULLS LAST" syntax. + // Null values are always treated as less than non-null values. + // As such special handling is required here. + String direction = theTheAscending ? " ASC" : " DESC"; + String sortColumnName = theTheColumnValueNormalized.getTable().getAlias() + "." + theTheColumnValueNormalized.getName(); + if ((theTheAscending && theNullOrder == OrderObject.NullOrder.LAST) + || (!theTheAscending && theNullOrder == OrderObject.NullOrder.FIRST)) { + // Coalescing the value with a String consisting of 200 'z' characters will ensure that the rows appear + // in the correct order with nulls being treated as the highest String values. + char[] chars = new char[200]; + Arrays.fill(chars, 'z'); + String lastString = new String(chars); + sortColumnName = "COALESCE(" + sortColumnName + ", '" + lastString + "')"; + } + mySelect.addCustomOrderings(sortColumnName + direction); + } else { + addSort(theTheColumnValueNormalized, theTheAscending, theNullOrder); + } + } + + public void addSortNumeric(DbColumn theTheColumnValueNormalized, boolean theTheAscending, OrderObject.NullOrder theNullOrder) { + if ((dialectIsMariaDb || dialectIsMySql || dialectIsMsSql)) { + // MariaDB, MySQL and MSSQL do not support "NULLS FIRST" and "NULLS LAST" syntax. + // Null values are always treated as less than non-null values. + // As such special handling is required here. + String direction; + String sortColumnName = theTheColumnValueNormalized.getTable().getAlias() + "." + theTheColumnValueNormalized.getName(); + if ((theTheAscending && theNullOrder == OrderObject.NullOrder.LAST) + || (!theTheAscending && theNullOrder == OrderObject.NullOrder.FIRST)) { + // Negating the numeric column value and reversing the sort order will ensure that the rows appear + // in the correct order with nulls appearing first or last as needed. + direction = theTheAscending ? " DESC" : " ASC"; + sortColumnName = "-" + sortColumnName; + } else { + direction = theTheAscending ? " ASC" : " DESC"; + } + mySelect.addCustomOrderings(sortColumnName + direction); + } else { + addSort(theTheColumnValueNormalized, theTheAscending, theNullOrder); + } + } + + public void addSortDate(DbColumn theTheColumnValueNormalized, boolean theTheAscending, OrderObject.NullOrder theNullOrder) { + if ((dialectIsMariaDb || dialectIsMySql || dialectIsMsSql)) { + // MariaDB, MySQL and MSSQL do not support "NULLS FIRST" and "NULLS LAST" syntax. + // Null values are always treated as less than non-null values. + // As such special handling is required here. + String direction = theTheAscending ? " ASC" : " DESC"; + String sortColumnName = theTheColumnValueNormalized.getTable().getAlias() + "." + theTheColumnValueNormalized.getName(); + if ((theTheAscending && theNullOrder == OrderObject.NullOrder.LAST) + || (!theTheAscending && theNullOrder == OrderObject.NullOrder.FIRST)) { + // Coalescing the value with a date well into the future will ensure that the rows appear + // in the correct order with nulls being treated as the highest Date values. + sortColumnName = "COALESCE(" + sortColumnName + ", '9000-01-01')"; + } else { + direction = theTheAscending ? " ASC" : " DESC"; + } + mySelect.addCustomOrderings(sortColumnName + direction); + } else { + addSort(theTheColumnValueNormalized, theTheAscending, theNullOrder); + } + } + + private void addSort(DbColumn theTheColumnValueNormalized, boolean theTheAscending, OrderObject.NullOrder theNullOrder) { OrderObject.Dir direction = theTheAscending ? OrderObject.Dir.ASCENDING : OrderObject.Dir.DESCENDING; OrderObject orderObject = new OrderObject(direction, theTheColumnValueNormalized); orderObject.setNullOrder(theNullOrder); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java new file mode 100644 index 00000000000..e0d997f29fc --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java @@ -0,0 +1,179 @@ +package ca.uhn.fhir.jpa.search.builder.sql; + +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.interceptor.model.RequestPartitionId; +import ca.uhn.fhir.jpa.config.HibernateDialectProvider; +import ca.uhn.fhir.jpa.model.config.PartitionSettings; +import ca.uhn.fhir.jpa.model.entity.ModelConfig; +import ca.uhn.fhir.jpa.search.builder.predicate.BaseJoiningPredicateBuilder; +import ca.uhn.fhir.jpa.search.builder.predicate.DatePredicateBuilder; +import ca.uhn.fhir.jpa.search.builder.predicate.ResourceTablePredicateBuilder; +import ca.uhn.fhir.jpa.search.builder.predicate.StringPredicateBuilder; +import com.healthmarketscience.sqlbuilder.Condition; +import com.healthmarketscience.sqlbuilder.OrderObject; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.when; + +public class SearchQueryBuilderMySqlTest { + + @Mock + private SqlObjectFactory mySqlObjectFactory; + @Mock + private HibernateDialectProvider myHibernateDialectProvider; + + private final FhirContext myFhirContext = FhirContext.forR4(); + + @BeforeEach + public void beforeInitMocks() { + MockitoAnnotations.initMocks(this); + when(myHibernateDialectProvider.getDialect()).thenReturn(new org.hibernate.dialect.MySQL57Dialect()); + } + + private SearchQueryBuilder createSearchQueryBuilder() { + return new SearchQueryBuilder(myFhirContext, new ModelConfig(), new PartitionSettings(), RequestPartitionId.allPartitions(), "Patient", mySqlObjectFactory, myHibernateDialectProvider, false); + } + + @Test + public void testAddSortNumericNoNullOrder() { + GeneratedSql generatedSql = buildSqlWithNumericSort(true,null); + assertTrue(generatedSql.getSql().endsWith("ORDER BY -t1.SP_VALUE_LOW DESC limit ?")); + + generatedSql = buildSqlWithNumericSort(false,null); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_LOW DESC limit ?")); + + } + + private GeneratedSql buildSqlWithNumericSort(Boolean theAscending, OrderObject.NullOrder theNullOrder) { + SearchQueryBuilder searchQueryBuilder = createSearchQueryBuilder(); + when(mySqlObjectFactory.resourceTable(any())).thenReturn(new ResourceTablePredicateBuilder(searchQueryBuilder)); + when(mySqlObjectFactory.dateIndexTable(any())).thenReturn(new DatePredicateBuilder(searchQueryBuilder)); + + BaseJoiningPredicateBuilder firstPredicateBuilder = searchQueryBuilder.getOrCreateFirstPredicateBuilder(); + DatePredicateBuilder sortPredicateBuilder = searchQueryBuilder.addDatePredicateBuilder(firstPredicateBuilder.getResourceIdColumn()); + + Condition hashIdentityPredicate = sortPredicateBuilder.createHashIdentityPredicate("MolecularSequence", "variant-start"); + searchQueryBuilder.addPredicate(hashIdentityPredicate); + if (theNullOrder == null) { + searchQueryBuilder.addSortNumeric(sortPredicateBuilder.getColumnValueLow(), theAscending); + } else { + searchQueryBuilder.addSortNumeric(sortPredicateBuilder.getColumnValueLow(), theAscending, theNullOrder); + } + + return searchQueryBuilder.generate(0,500); + + } + + @Test + public void testAddSortNumericWithNullOrder() { + GeneratedSql generatedSql = buildSqlWithNumericSort(true, OrderObject.NullOrder.FIRST); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_LOW ASC limit ?")); + + generatedSql = buildSqlWithNumericSort(false, OrderObject.NullOrder.FIRST); + assertTrue(generatedSql.getSql().endsWith("ORDER BY -t1.SP_VALUE_LOW ASC limit ?")); + + generatedSql = buildSqlWithNumericSort(true, OrderObject.NullOrder.LAST); + assertTrue(generatedSql.getSql().endsWith("ORDER BY -t1.SP_VALUE_LOW DESC limit ?")); + + generatedSql = buildSqlWithNumericSort(false, OrderObject.NullOrder.LAST); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_LOW DESC limit ?")); + + } + + @Test + public void testAddSortStringNoNullOrder() { + GeneratedSql generatedSql = buildSqlWithStringSort(true,null); + assertTrue(generatedSql.getSql().endsWith("ORDER BY COALESCE(t1.SP_VALUE_NORMALIZED, \'zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz\') ASC limit ?")); + + generatedSql = buildSqlWithStringSort(false,null); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_NORMALIZED DESC limit ?")); + + } + + private GeneratedSql buildSqlWithStringSort(Boolean theAscending, OrderObject.NullOrder theNullOrder) { + SearchQueryBuilder searchQueryBuilder = createSearchQueryBuilder(); + when(mySqlObjectFactory.resourceTable(any())).thenReturn(new ResourceTablePredicateBuilder(searchQueryBuilder)); + when(mySqlObjectFactory.stringIndexTable(any())).thenReturn(new StringPredicateBuilder(searchQueryBuilder)); + + BaseJoiningPredicateBuilder firstPredicateBuilder = searchQueryBuilder.getOrCreateFirstPredicateBuilder(); + StringPredicateBuilder sortPredicateBuilder = searchQueryBuilder.addStringPredicateBuilder(firstPredicateBuilder.getResourceIdColumn()); + + Condition hashIdentityPredicate = sortPredicateBuilder.createHashIdentityPredicate("patient", "family"); + searchQueryBuilder.addPredicate(hashIdentityPredicate); + if (theNullOrder == null) { + searchQueryBuilder.addSortString(sortPredicateBuilder.getColumnValueNormalized(), theAscending); + } else { + searchQueryBuilder.addSortString(sortPredicateBuilder.getColumnValueNormalized(), theAscending, theNullOrder); + } + + return searchQueryBuilder.generate(0,500); + + } + + @Test + public void testAddSortStringWithNullOrder() { + GeneratedSql generatedSql = buildSqlWithStringSort(true, OrderObject.NullOrder.FIRST); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_NORMALIZED ASC limit ?")); + + generatedSql = buildSqlWithStringSort(false, OrderObject.NullOrder.FIRST); + assertTrue(generatedSql.getSql().endsWith("ORDER BY COALESCE(t1.SP_VALUE_NORMALIZED, 'zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz') DESC limit ?")); + + generatedSql = buildSqlWithStringSort(true, OrderObject.NullOrder.LAST); + assertTrue(generatedSql.getSql().endsWith("ORDER BY COALESCE(t1.SP_VALUE_NORMALIZED, 'zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz') ASC limit ?")); + + generatedSql = buildSqlWithStringSort(false, OrderObject.NullOrder.LAST); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_NORMALIZED DESC limit ?")); + + } + + @Test + public void testAddSortDateNoNullOrder() { + GeneratedSql generatedSql = buildSqlWithDateSort(true,null); + assertTrue(generatedSql.getSql().endsWith("ORDER BY COALESCE(t1.SP_VALUE_LOW, '9000-01-01') ASC limit ?")); + + generatedSql = buildSqlWithDateSort(false,null); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_LOW DESC limit ?")); + + } + + private GeneratedSql buildSqlWithDateSort(Boolean theAscending, OrderObject.NullOrder theNullOrder) { + SearchQueryBuilder searchQueryBuilder = createSearchQueryBuilder(); + when(mySqlObjectFactory.resourceTable(any())).thenReturn(new ResourceTablePredicateBuilder(searchQueryBuilder)); + when(mySqlObjectFactory.dateIndexTable(any())).thenReturn(new DatePredicateBuilder(searchQueryBuilder)); + + BaseJoiningPredicateBuilder firstPredicateBuilder = searchQueryBuilder.getOrCreateFirstPredicateBuilder(); + DatePredicateBuilder sortPredicateBuilder = searchQueryBuilder.addDatePredicateBuilder(firstPredicateBuilder.getResourceIdColumn()); + + Condition hashIdentityPredicate = sortPredicateBuilder.createHashIdentityPredicate("patient", "birthdate"); + searchQueryBuilder.addPredicate(hashIdentityPredicate); + if (theNullOrder == null) { + searchQueryBuilder.addSortDate(sortPredicateBuilder.getColumnValueLow(), theAscending); + } else { + searchQueryBuilder.addSortDate(sortPredicateBuilder.getColumnValueLow(), theAscending, theNullOrder); + } + + return searchQueryBuilder.generate(0,500); + + } + + @Test + public void testAddSortDateWithNullOrder() { + GeneratedSql generatedSql = buildSqlWithDateSort(true, OrderObject.NullOrder.FIRST); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_LOW ASC limit ?")); + + generatedSql = buildSqlWithDateSort(false, OrderObject.NullOrder.FIRST); + assertTrue(generatedSql.getSql().endsWith("ORDER BY COALESCE(t1.SP_VALUE_LOW, '9000-01-01') DESC limit ?")); + + generatedSql = buildSqlWithDateSort(true, OrderObject.NullOrder.LAST); + assertTrue(generatedSql.getSql().endsWith("ORDER BY COALESCE(t1.SP_VALUE_LOW, '9000-01-01') ASC limit ?")); + + generatedSql = buildSqlWithDateSort(false, OrderObject.NullOrder.LAST); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_LOW DESC limit ?")); + + } +} From eb59da3f0353c86388feab04fe94909f08023557 Mon Sep 17 00:00:00 2001 From: ianmarshall Date: Thu, 24 Dec 2020 15:23:50 -0500 Subject: [PATCH 2/4] Added changelog. --- .../2262-fix-nullable-column-sorting-in-certain-dbs.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_3_0/2262-fix-nullable-column-sorting-in-certain-dbs.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_3_0/2262-fix-nullable-column-sorting-in-certain-dbs.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_3_0/2262-fix-nullable-column-sorting-in-certain-dbs.yaml new file mode 100644 index 00000000000..1e1a852ac85 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_3_0/2262-fix-nullable-column-sorting-in-certain-dbs.yaml @@ -0,0 +1,5 @@ +--- +type: fix +issue: 2262 +title: "Sorting of search results was not working for MySQL, MSSQL and MariaDB due to recent changes made to handle sorting + of nullable columns. This has now been fixed." From bac47dbbf6b31a9d32db0f5371a7a5a294c77d78 Mon Sep 17 00:00:00 2001 From: ianmarshall Date: Mon, 4 Jan 2021 19:06:39 -0500 Subject: [PATCH 3/4] Changes per code review plus test fixes. --- .../builder/sql/SearchQueryBuilder.java | 53 ++++++++++--------- .../sql/SearchQueryBuilderMySqlTest.java | 19 ++++--- .../builder/sql/SearchQueryBuilderTest.java | 50 ++++++++++------- 3 files changed, 71 insertions(+), 51 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java index c49da7706cb..a7214933e24 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java @@ -98,7 +98,6 @@ public class SearchQueryBuilder { private BaseJoiningPredicateBuilder myFirstPredicateBuilder; private boolean dialectIsMsSql; private boolean dialectIsMySql; - private boolean dialectIsMariaDb; /** * Constructor @@ -119,13 +118,10 @@ public class SearchQueryBuilder { mySqlBuilderFactory = theSqlBuilderFactory; myCountQuery = theCountQuery; myDialect = theDialect; - if (myDialect instanceof org.hibernate.dialect.MySQL57Dialect){ + if (myDialect instanceof org.hibernate.dialect.MySQLDialect){ dialectIsMySql = true; } - if (myDialect instanceof org.hibernate.dialect.MariaDB53Dialect){ - dialectIsMariaDb = true; - } - if (myDialect instanceof org.hibernate.dialect.SQLServer2012Dialect){ + if (myDialect instanceof org.hibernate.dialect.SQLServerDialect){ dialectIsMsSql = true; } @@ -580,29 +576,31 @@ public class SearchQueryBuilder { } public void addSortString(DbColumn theTheColumnValueNormalized, boolean theTheAscending, OrderObject.NullOrder theNullOrder) { - if ((dialectIsMariaDb || dialectIsMySql || dialectIsMsSql)) { + if ((dialectIsMySql || dialectIsMsSql)) { // MariaDB, MySQL and MSSQL do not support "NULLS FIRST" and "NULLS LAST" syntax. - // Null values are always treated as less than non-null values. - // As such special handling is required here. String direction = theTheAscending ? " ASC" : " DESC"; String sortColumnName = theTheColumnValueNormalized.getTable().getAlias() + "." + theTheColumnValueNormalized.getName(); + final StringBuilder sortColumnNameBuilder = new StringBuilder(); + // The following block has been commented out for performance. + // Uncomment if NullOrder is needed for MariaDB, MySQL or MSSQL + /* + // Null values are always treated as less than non-null values. if ((theTheAscending && theNullOrder == OrderObject.NullOrder.LAST) || (!theTheAscending && theNullOrder == OrderObject.NullOrder.FIRST)) { - // Coalescing the value with a String consisting of 200 'z' characters will ensure that the rows appear - // in the correct order with nulls being treated as the highest String values. - char[] chars = new char[200]; - Arrays.fill(chars, 'z'); - String lastString = new String(chars); - sortColumnName = "COALESCE(" + sortColumnName + ", '" + lastString + "')"; + // In this case, precede the "order by" column with a case statement that returns + // 1 for null and 0 non-null so that nulls will be sorted as greater than non-nulls. + sortColumnNameBuilder.append( "CASE WHEN " ).append( sortColumnName ).append( " IS NULL THEN 1 ELSE 0 END" ).append(direction).append(", "); } - mySelect.addCustomOrderings(sortColumnName + direction); + */ + sortColumnNameBuilder.append(sortColumnName).append(direction); + mySelect.addCustomOrderings(sortColumnNameBuilder.toString()); } else { addSort(theTheColumnValueNormalized, theTheAscending, theNullOrder); } } public void addSortNumeric(DbColumn theTheColumnValueNormalized, boolean theTheAscending, OrderObject.NullOrder theNullOrder) { - if ((dialectIsMariaDb || dialectIsMySql || dialectIsMsSql)) { + if ((dialectIsMySql || dialectIsMsSql)) { // MariaDB, MySQL and MSSQL do not support "NULLS FIRST" and "NULLS LAST" syntax. // Null values are always treated as less than non-null values. // As such special handling is required here. @@ -624,21 +622,24 @@ public class SearchQueryBuilder { } public void addSortDate(DbColumn theTheColumnValueNormalized, boolean theTheAscending, OrderObject.NullOrder theNullOrder) { - if ((dialectIsMariaDb || dialectIsMySql || dialectIsMsSql)) { + if ((dialectIsMySql || dialectIsMsSql)) { // MariaDB, MySQL and MSSQL do not support "NULLS FIRST" and "NULLS LAST" syntax. - // Null values are always treated as less than non-null values. - // As such special handling is required here. String direction = theTheAscending ? " ASC" : " DESC"; String sortColumnName = theTheColumnValueNormalized.getTable().getAlias() + "." + theTheColumnValueNormalized.getName(); + final StringBuilder sortColumnNameBuilder = new StringBuilder(); + // The following block has been commented out for performance. + // Uncomment if NullOrder is needed for MariaDB, MySQL or MSSQL + /* + // Null values are always treated as less than non-null values. if ((theTheAscending && theNullOrder == OrderObject.NullOrder.LAST) || (!theTheAscending && theNullOrder == OrderObject.NullOrder.FIRST)) { - // Coalescing the value with a date well into the future will ensure that the rows appear - // in the correct order with nulls being treated as the highest Date values. - sortColumnName = "COALESCE(" + sortColumnName + ", '9000-01-01')"; - } else { - direction = theTheAscending ? " ASC" : " DESC"; + // In this case, precede the "order by" column with a case statement that returns + // 1 for null and 0 non-null so that nulls will be sorted as greater than non-nulls. + sortColumnNameBuilder.append( "CASE WHEN " ).append( sortColumnName ).append( " IS NULL THEN 1 ELSE 0 END" ).append(direction).append(", "); } - mySelect.addCustomOrderings(sortColumnName + direction); + */ + sortColumnNameBuilder.append(sortColumnName).append(direction); + mySelect.addCustomOrderings(sortColumnNameBuilder.toString()); } else { addSort(theTheColumnValueNormalized, theTheAscending, theNullOrder); } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java index e0d997f29fc..1f531ffcadd 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java @@ -12,6 +12,7 @@ import ca.uhn.fhir.jpa.search.builder.predicate.StringPredicateBuilder; import com.healthmarketscience.sqlbuilder.Condition; import com.healthmarketscience.sqlbuilder.OrderObject; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -88,7 +89,8 @@ public class SearchQueryBuilderMySqlTest { @Test public void testAddSortStringNoNullOrder() { GeneratedSql generatedSql = buildSqlWithStringSort(true,null); - assertTrue(generatedSql.getSql().endsWith("ORDER BY COALESCE(t1.SP_VALUE_NORMALIZED, \'zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz\') ASC limit ?")); +// assertTrue(generatedSql.getSql().endsWith("ORDER BY CASE WHEN t1.SP_VALUE_NORMALIZED IS NULL THEN 1 ELSE 0 END ASC, t1.SP_VALUE_NORMALIZED ASC limit ?")); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_NORMALIZED ASC limit ?")); generatedSql = buildSqlWithStringSort(false,null); assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_NORMALIZED DESC limit ?")); @@ -121,10 +123,12 @@ public class SearchQueryBuilderMySqlTest { assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_NORMALIZED ASC limit ?")); generatedSql = buildSqlWithStringSort(false, OrderObject.NullOrder.FIRST); - assertTrue(generatedSql.getSql().endsWith("ORDER BY COALESCE(t1.SP_VALUE_NORMALIZED, 'zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz') DESC limit ?")); +// assertTrue(generatedSql.getSql().endsWith("ORDER BY CASE WHEN t1.SP_VALUE_NORMALIZED IS NULL THEN 1 ELSE 0 END DESC, t1.SP_VALUE_NORMALIZED DESC limit ?")); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_NORMALIZED DESC limit ?")); generatedSql = buildSqlWithStringSort(true, OrderObject.NullOrder.LAST); - assertTrue(generatedSql.getSql().endsWith("ORDER BY COALESCE(t1.SP_VALUE_NORMALIZED, 'zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz') ASC limit ?")); +// assertTrue(generatedSql.getSql().endsWith("ORDER BY CASE WHEN t1.SP_VALUE_NORMALIZED IS NULL THEN 1 ELSE 0 END ASC, t1.SP_VALUE_NORMALIZED ASC limit ?")); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_NORMALIZED ASC limit ?")); generatedSql = buildSqlWithStringSort(false, OrderObject.NullOrder.LAST); assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_NORMALIZED DESC limit ?")); @@ -134,7 +138,8 @@ public class SearchQueryBuilderMySqlTest { @Test public void testAddSortDateNoNullOrder() { GeneratedSql generatedSql = buildSqlWithDateSort(true,null); - assertTrue(generatedSql.getSql().endsWith("ORDER BY COALESCE(t1.SP_VALUE_LOW, '9000-01-01') ASC limit ?")); +// assertTrue(generatedSql.getSql().endsWith("ORDER BY CASE WHEN t1.SP_VALUE_LOW IS NULL THEN 1 ELSE 0 END ASC, t1.SP_VALUE_LOW ASC limit ?")); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_LOW ASC limit ?")); generatedSql = buildSqlWithDateSort(false,null); assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_LOW DESC limit ?")); @@ -167,10 +172,12 @@ public class SearchQueryBuilderMySqlTest { assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_LOW ASC limit ?")); generatedSql = buildSqlWithDateSort(false, OrderObject.NullOrder.FIRST); - assertTrue(generatedSql.getSql().endsWith("ORDER BY COALESCE(t1.SP_VALUE_LOW, '9000-01-01') DESC limit ?")); +// assertTrue(generatedSql.getSql().endsWith("ORDER BY CASE WHEN t1.SP_VALUE_LOW IS NULL THEN 1 ELSE 0 END DESC, t1.SP_VALUE_LOW DESC limit ?")); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_LOW DESC limit ?")); generatedSql = buildSqlWithDateSort(true, OrderObject.NullOrder.LAST); - assertTrue(generatedSql.getSql().endsWith("ORDER BY COALESCE(t1.SP_VALUE_LOW, '9000-01-01') ASC limit ?")); +// assertTrue(generatedSql.getSql().endsWith("ORDER BY CASE WHEN t1.SP_VALUE_LOW IS NULL THEN 1 ELSE 0 END ASC, t1.SP_VALUE_LOW ASC limit ?")); + assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_LOW ASC limit ?")); generatedSql = buildSqlWithDateSort(false, OrderObject.NullOrder.LAST); assertTrue(generatedSql.getSql().endsWith("ORDER BY t1.SP_VALUE_LOW DESC limit ?")); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderTest.java index e6ba8aabf56..fe0340ed1b9 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderTest.java @@ -83,22 +83,25 @@ public class SearchQueryBuilderTest { dialectProvider.setDialectForUnitTest(new SQLServer2005Dialect()); SearchQueryBuilder builder = new SearchQueryBuilder(myFhirContext, myModelConfig, myPartitionSettings, myRequestPartitionId, "Patient", mySqlBuilderFactory, dialectProvider, false); builder.addResourceIdsPredicate(Lists.newArrayList(500L, 501L)); - builder.addSort(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); + builder.addSortDate(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); GeneratedSql generated; // No range generated = builder.generate(null, null); - assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC NULLS LAST", generated.getSql()); +// assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY CASE WHEN t0.RES_UPDATED IS NULL THEN 1 ELSE 0 END ASC, t0.RES_UPDATED ASC", generated.getSql()); + assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC", generated.getSql()); assertThat(generated.getBindVariables().toString(), generated.getBindVariables(), contains("Patient", 500L, 501L)); // Max only generated = builder.generate(null, 10); - assertEquals("SELECT TOP(?) t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC NULLS LAST", generated.getSql()); +// assertEquals("SELECT TOP(?) t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY CASE WHEN t0.RES_UPDATED IS NULL THEN 1 ELSE 0 END ASC, t0.RES_UPDATED ASC", generated.getSql()); + assertEquals("SELECT TOP(?) t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC", generated.getSql()); assertThat(generated.getBindVariables().toString(), generated.getBindVariables(), contains(10, "Patient", 500L, 501L)); // Range generated = builder.generate(10, 5); - assertEquals("WITH query AS (SELECT inner_query.*, ROW_NUMBER() OVER (ORDER BY CURRENT_TIMESTAMP) as __hibernate_row_nr__ FROM ( SELECT TOP(?) t0.RES_ID as page0_ FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC NULLS LAST ) inner_query ) SELECT page0_ FROM query WHERE __hibernate_row_nr__ >= ? AND __hibernate_row_nr__ < ?", generated.getSql()); +// assertEquals("WITH query AS (SELECT inner_query.*, ROW_NUMBER() OVER (ORDER BY CURRENT_TIMESTAMP) as __hibernate_row_nr__ FROM ( SELECT TOP(?) t0.RES_ID as page0_ FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY CASE WHEN t0.RES_UPDATED IS NULL THEN 1 ELSE 0 END ASC, t0.RES_UPDATED ASC ) inner_query ) SELECT page0_ FROM query WHERE __hibernate_row_nr__ >= ? AND __hibernate_row_nr__ < ?", generated.getSql()); + assertEquals("WITH query AS (SELECT inner_query.*, ROW_NUMBER() OVER (ORDER BY CURRENT_TIMESTAMP) as __hibernate_row_nr__ FROM ( SELECT TOP(?) t0.RES_ID as page0_ FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC ) inner_query ) SELECT page0_ FROM query WHERE __hibernate_row_nr__ >= ? AND __hibernate_row_nr__ < ?", generated.getSql()); assertThat(generated.getBindVariables().toString(), generated.getBindVariables(), contains(5, "Patient", 500L, 501L, 11, 16)); } @@ -137,22 +140,25 @@ public class SearchQueryBuilderTest { dialectProvider.setDialectForUnitTest(new SQLServer2012Dialect()); SearchQueryBuilder builder = new SearchQueryBuilder(myFhirContext, myModelConfig, myPartitionSettings, myRequestPartitionId, "Patient", mySqlBuilderFactory, dialectProvider, false); builder.addResourceIdsPredicate(Lists.newArrayList(500L, 501L)); - builder.addSort(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); + builder.addSortDate(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); GeneratedSql generated; // No range generated = builder.generate(null, null); - assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC NULLS LAST", generated.getSql()); +// assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY CASE WHEN t0.RES_UPDATED IS NULL THEN 1 ELSE 0 END ASC, t0.RES_UPDATED ASC", generated.getSql()); + assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC", generated.getSql()); assertThat(generated.getBindVariables().toString(), generated.getBindVariables(), contains("Patient", 500L, 501L)); // Max only generated = builder.generate(null, 10); - assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC NULLS LAST offset 0 rows fetch next ? rows only", generated.getSql()); +// assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY CASE WHEN t0.RES_UPDATED IS NULL THEN 1 ELSE 0 END ASC, t0.RES_UPDATED ASC offset 0 rows fetch next ? rows only", generated.getSql()); + assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC offset 0 rows fetch next ? rows only", generated.getSql()); assertThat(generated.getBindVariables().toString(), generated.getBindVariables(), contains("Patient", 500L, 501L, 10)); // Range generated = builder.generate(10, 5); - assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC NULLS LAST offset ? rows fetch next ? rows only", generated.getSql()); +// assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY CASE WHEN t0.RES_UPDATED IS NULL THEN 1 ELSE 0 END ASC, t0.RES_UPDATED ASC offset ? rows fetch next ? rows only", generated.getSql()); + assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC offset ? rows fetch next ? rows only", generated.getSql()); assertThat(generated.getBindVariables().toString(), generated.getBindVariables(), contains("Patient", 500L, 501L, 10, 5)); } @@ -190,7 +196,7 @@ public class SearchQueryBuilderTest { dialectProvider.setDialectForUnitTest(new PostgreSQL95Dialect()); SearchQueryBuilder builder = new SearchQueryBuilder(myFhirContext, myModelConfig, myPartitionSettings, myRequestPartitionId, "Patient", mySqlBuilderFactory, dialectProvider, false); builder.addResourceIdsPredicate(Lists.newArrayList(500L, 501L)); - builder.addSort(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); + builder.addSortDate(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); GeneratedSql generated; // No range @@ -243,7 +249,7 @@ public class SearchQueryBuilderTest { dialectProvider.setDialectForUnitTest(new Oracle12cDialect()); SearchQueryBuilder builder = new SearchQueryBuilder(myFhirContext, myModelConfig, myPartitionSettings, myRequestPartitionId, "Patient", mySqlBuilderFactory, dialectProvider, false); builder.addResourceIdsPredicate(Lists.newArrayList(500L, 501L)); - builder.addSort(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); + builder.addSortDate(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); GeneratedSql generated; // No range @@ -297,22 +303,25 @@ public class SearchQueryBuilderTest { dialectProvider.setDialectForUnitTest(new MySQL8Dialect()); SearchQueryBuilder builder = new SearchQueryBuilder(myFhirContext, myModelConfig, myPartitionSettings, myRequestPartitionId, "Patient", mySqlBuilderFactory, dialectProvider, false); builder.addResourceIdsPredicate(Lists.newArrayList(500L, 501L)); - builder.addSort(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); + builder.addSortDate(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); GeneratedSql generated; // No range generated = builder.generate(null, null); - assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC NULLS LAST", generated.getSql()); +// assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY CASE WHEN t0.RES_UPDATED IS NULL THEN 1 ELSE 0 END ASC, t0.RES_UPDATED ASC", generated.getSql()); + assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC", generated.getSql()); assertThat(generated.getBindVariables().toString(), generated.getBindVariables(), contains("Patient", 500L, 501L)); // Max only generated = builder.generate(null, 10); - assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC NULLS LAST limit ?", generated.getSql()); +// assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY CASE WHEN t0.RES_UPDATED IS NULL THEN 1 ELSE 0 END ASC, t0.RES_UPDATED ASC limit ?", generated.getSql()); + assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC limit ?", generated.getSql()); assertThat(generated.getBindVariables().toString(), generated.getBindVariables(), contains("Patient", 500L, 501L, 10)); // Range generated = builder.generate(10, 5); - assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC NULLS LAST limit ?, ?", generated.getSql()); +// assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY CASE WHEN t0.RES_UPDATED IS NULL THEN 1 ELSE 0 END ASC, t0.RES_UPDATED ASC limit ?, ?", generated.getSql()); + assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC limit ?, ?", generated.getSql()); assertThat(generated.getBindVariables().toString(), generated.getBindVariables(), contains("Patient", 500L, 501L, 10, 5)); } @@ -351,22 +360,25 @@ public class SearchQueryBuilderTest { dialectProvider.setDialectForUnitTest(new MariaDB103Dialect()); SearchQueryBuilder builder = new SearchQueryBuilder(myFhirContext, myModelConfig, myPartitionSettings, myRequestPartitionId, "Patient", mySqlBuilderFactory, dialectProvider, false); builder.addResourceIdsPredicate(Lists.newArrayList(500L, 501L)); - builder.addSort(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); + builder.addSortDate(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); GeneratedSql generated; // No range generated = builder.generate(null, null); - assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC NULLS LAST", generated.getSql()); +// assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY CASE WHEN t0.RES_UPDATED IS NULL THEN 1 ELSE 0 END ASC, t0.RES_UPDATED ASC", generated.getSql()); + assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC", generated.getSql()); assertThat(generated.getBindVariables().toString(), generated.getBindVariables(), contains("Patient", 500L, 501L)); // Max only generated = builder.generate(null, 10); - assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC NULLS LAST limit ?", generated.getSql()); +// assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY CASE WHEN t0.RES_UPDATED IS NULL THEN 1 ELSE 0 END ASC, t0.RES_UPDATED ASC limit ?", generated.getSql()); + assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC limit ?", generated.getSql()); assertThat(generated.getBindVariables().toString(), generated.getBindVariables(), contains("Patient", 500L, 501L, 10)); // Range generated = builder.generate(10, 5); - assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC NULLS LAST limit ?, ?", generated.getSql()); +// assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY CASE WHEN t0.RES_UPDATED IS NULL THEN 1 ELSE 0 END ASC, t0.RES_UPDATED ASC limit ?, ?", generated.getSql()); + assertEquals("SELECT t0.RES_ID FROM HFJ_RESOURCE t0 WHERE (((t0.RES_TYPE = ?) AND (t0.RES_DELETED_AT IS NULL)) AND (t0.RES_ID IN (?,?) )) ORDER BY t0.RES_UPDATED ASC limit ?, ?", generated.getSql()); assertThat(generated.getBindVariables().toString(), generated.getBindVariables(), contains("Patient", 500L, 501L, 10, 5)); } @@ -405,7 +417,7 @@ public class SearchQueryBuilderTest { dialectProvider.setDialectForUnitTest(new DerbyTenSevenDialect()); SearchQueryBuilder builder = new SearchQueryBuilder(myFhirContext, myModelConfig, myPartitionSettings, myRequestPartitionId, "Patient", mySqlBuilderFactory, dialectProvider, false); builder.addResourceIdsPredicate(Lists.newArrayList(500L, 501L)); - builder.addSort(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); + builder.addSortDate(builder.getOrCreateResourceTablePredicateBuilder().getColumnLastUpdated(), true); GeneratedSql generated; // No range From 1f0ea21cdedf3ee17d995016e8c43633ee9d71c6 Mon Sep 17 00:00:00 2001 From: ianmarshall Date: Mon, 4 Jan 2021 19:09:04 -0500 Subject: [PATCH 4/4] Changes per code review plus test fixes. --- .../ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java | 1 - .../fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java | 1 - .../uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderTest.java | 1 - 3 files changed, 3 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java index a7214933e24..667539ca770 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java @@ -68,7 +68,6 @@ import org.slf4j.LoggerFactory; import javax.annotation.Nonnull; import javax.annotation.Nullable; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.List; import java.util.UUID; diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java index 1f531ffcadd..a040ea1d3da 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderMySqlTest.java @@ -12,7 +12,6 @@ import ca.uhn.fhir.jpa.search.builder.predicate.StringPredicateBuilder; import com.healthmarketscience.sqlbuilder.Condition; import com.healthmarketscience.sqlbuilder.OrderObject; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.mockito.Mock; import org.mockito.MockitoAnnotations; diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderTest.java index fe0340ed1b9..8ae2e33d00d 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderTest.java @@ -11,7 +11,6 @@ import com.google.common.collect.Lists; import org.hibernate.dialect.DerbyTenSevenDialect; import org.hibernate.dialect.MariaDB103Dialect; import org.hibernate.dialect.MySQL8Dialect; -import org.hibernate.dialect.MySQLDialect; import org.hibernate.dialect.Oracle12cDialect; import org.hibernate.dialect.PostgreSQL95Dialect; import org.hibernate.dialect.SQLServer2005Dialect;