From c2ed5f84190bf036afe52cf33ff6c82173c4d399 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 3 Jul 2024 13:40:18 -0400 Subject: [PATCH] Work on combo --- .../jpa/search/builder/SearchBuilder.java | 39 +++++++++---- ...UniqueSearchParameterPredicateBuilder.java | 15 ++--- ...UniqueSearchParameterPredicateBuilder.java | 3 +- .../uhn/fhir/jpa/util/PermutationBuilder.java | 7 ++- .../jpa/searchparam/SearchParameterMap.java | 16 ------ ...rResourceDaoR4ComboNonUniqueParamTest.java | 55 +++++++++++++++++++ 6 files changed, 97 insertions(+), 38 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java index 4161ff5f290..8fecacfffb4 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java @@ -314,9 +314,7 @@ public class SearchBuilder implements ISearchBuilder { * parameters all have no modifiers. */ private boolean isCompositeUniqueSpCandidate() { - return myStorageSettings.isUniqueIndexesEnabled() - && myParams.getEverythingMode() == null - && myParams.isAllParametersHaveNoModifier(); + return myStorageSettings.isUniqueIndexesEnabled() && myParams.getEverythingMode() == null; } @SuppressWarnings("ConstantConditions") @@ -1954,7 +1952,8 @@ public class SearchBuilder implements ISearchBuilder { IQueryParameterType nextOr = nextPermutation.get(paramIndex); String nextOrValue = nextOr.getValueAsQueryToken(myContext); - RuntimeSearchParam nextParamDef = mySearchParamRegistry.getActiveSearchParam(myResourceName, nextParamName); + RuntimeSearchParam nextParamDef = + mySearchParamRegistry.getActiveSearchParam(myResourceName, nextParamName); if (theComboParam.getComboSearchParamType() == ComboSearchParamType.NON_UNIQUE) { if (nextParamDef.getParamType() == RestSearchParameterTypeEnum.STRING) { nextOrValue = StringUtil.normalizeStringForSearchIndexing(nextOrValue); @@ -1975,7 +1974,7 @@ public class SearchBuilder implements ISearchBuilder { String indexString = searchStringBuilder.toString(); ourLog.debug( - "Checking for {} combo index for query: {}", theComboParam.getComboSearchParamType(), indexString); + "Checking for {} combo index for query: {}", theComboParam.getComboSearchParamType(), indexString); indexStrings.add(indexString); } @@ -2008,10 +2007,16 @@ public class SearchBuilder implements ISearchBuilder { theParams.clean(); } + /** + * Returns {@literal true} if the actual parameter instances in a given query are actually usable for + * searching against a combo param with the given parameter names. This might be {@literal false} if + * parameters have modifiers (e.g. ?name:exact=SIMPSON), prefixes + * (e.g. ?date=gt2024-02-01), etc. + */ private boolean validateParamValuesAreValidForComboParam( - RequestDetails theRequest, @Nonnull SearchParameterMap theParams, List comboParamNames) { + RequestDetails theRequest, @Nonnull SearchParameterMap theParams, List theComboParamNames) { boolean paramValuesAreValidForCombo = true; - for (String nextParamName : comboParamNames) { + for (String nextParamName : theComboParamNames) { List> nextValues = theParams.get(nextParamName); if (nextValues.isEmpty()) { @@ -2024,7 +2029,9 @@ public class SearchBuilder implements ISearchBuilder { if (nextOrValue instanceof DateParam) { DateParam dateParam = (DateParam) nextOrValue; if (dateParam.getPrecision() != TemporalPrecisionEnum.DAY) { - String message = "Search with params " + comboParamNames + " is not a candidate for combo searching - Date search with non-DAY precision for parameter '" + nextParamName + "'"; + String message = "Search with params " + theComboParamNames + + " is not a candidate for combo searching - Date search with non-DAY precision for parameter '" + + nextParamName + "'"; firePerformanceInfo(theRequest, message); paramValuesAreValidForCombo = false; break; @@ -2033,12 +2040,23 @@ public class SearchBuilder implements ISearchBuilder { if (nextOrValue instanceof BaseParamWithPrefix) { BaseParamWithPrefix paramWithPrefix = (BaseParamWithPrefix) nextOrValue; if (paramWithPrefix.getPrefix() != null) { - String message = "Search with params " + comboParamNames + " is not a candidate for combo searching - Parameter '" + nextParamName + "' has prefix: '" + paramWithPrefix.getPrefix().getValue() + "'"; + String message = "Search with params " + theComboParamNames + + " is not a candidate for combo searching - Parameter '" + nextParamName + + "' has prefix: '" + + paramWithPrefix.getPrefix().getValue() + "'"; firePerformanceInfo(theRequest, message); paramValuesAreValidForCombo = false; break; } } + if (isNotBlank(nextOrValue.getQueryParameterQualifier())) { + String message = "Search with params " + theComboParamNames + + " is not a candidate for combo searching - Parameter '" + nextParamName + + "' has modifier: '" + nextOrValue.getQueryParameterQualifier() + "'"; + firePerformanceInfo(theRequest, message); + paramValuesAreValidForCombo = false; + break; + } } } @@ -2452,8 +2470,7 @@ public class SearchBuilder implements ISearchBuilder { .add(RequestDetails.class, theRequest) .addIfMatchesType(ServletRequestDetails.class, theRequest) .add(StorageProcessingMessage.class, message); - CompositeInterceptorBroadcaster.doCallHooks( - myInterceptorBroadcaster, theRequest, pointcut, params); + CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequest, pointcut, params); } public static int getMaximumPageSize() { diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ComboNonUniqueSearchParameterPredicateBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ComboNonUniqueSearchParameterPredicateBuilder.java index 9f144f30daf..636a4c82c3b 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ComboNonUniqueSearchParameterPredicateBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ComboNonUniqueSearchParameterPredicateBuilder.java @@ -44,19 +44,20 @@ public class ComboNonUniqueSearchParameterPredicateBuilder extends BaseSearchPar myColumnHashComplete = getTable().addColumn("HASH_COMPLETE"); } - public Condition createPredicateHashComplete(RequestPartitionId theRequestPartitionId, List theIndexStrings) { + public Condition createPredicateHashComplete( + RequestPartitionId theRequestPartitionId, List theIndexStrings) { PartitionablePartitionId partitionId = - PartitionablePartitionId.toStoragePartition(theRequestPartitionId, getPartitionSettings()); + PartitionablePartitionId.toStoragePartition(theRequestPartitionId, getPartitionSettings()); Condition predicate; if (theIndexStrings.size() == 1) { long hash = ResourceIndexedComboTokenNonUnique.calculateHashComplete( - getPartitionSettings(), partitionId, theIndexStrings.get(0)); + getPartitionSettings(), partitionId, theIndexStrings.get(0)); predicate = BinaryCondition.equalTo(myColumnHashComplete, generatePlaceholder(hash)); } else { - List hashes = theIndexStrings - .stream() - .map(t -> ResourceIndexedComboTokenNonUnique.calculateHashComplete(getPartitionSettings(), partitionId, t)) - .collect(Collectors.toList()); + List hashes = theIndexStrings.stream() + .map(t -> ResourceIndexedComboTokenNonUnique.calculateHashComplete( + getPartitionSettings(), partitionId, t)) + .collect(Collectors.toList()); predicate = new InCondition(myColumnHashComplete, generatePlaceholders(hashes)); } return combineWithRequestPartitionIdPredicate(theRequestPartitionId, predicate); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ComboUniqueSearchParameterPredicateBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ComboUniqueSearchParameterPredicateBuilder.java index 7a5d677fd39..51b3ca14b90 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ComboUniqueSearchParameterPredicateBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ComboUniqueSearchParameterPredicateBuilder.java @@ -41,7 +41,8 @@ public class ComboUniqueSearchParameterPredicateBuilder extends BaseSearchParamP myColumnString = getTable().addColumn("IDX_STRING"); } - public Condition createPredicateIndexString(RequestPartitionId theRequestPartitionId, List theIndexStrings) { + public Condition createPredicateIndexString( + RequestPartitionId theRequestPartitionId, List theIndexStrings) { Condition predicate; if (theIndexStrings.size() == 1) { predicate = BinaryCondition.equalTo(myColumnString, generatePlaceholder(theIndexStrings.get(0))); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/PermutationBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/PermutationBuilder.java index 331112b1931..0cb78d47ce8 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/PermutationBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/PermutationBuilder.java @@ -65,13 +65,15 @@ public class PermutationBuilder { * @param The type associated with {@literal theInput}. The actual type doesn't matter, this method does not look at the * values at all other than to copy them to the output lists. */ - private static void doCalculatePermutationsIntoIndicesArrayAndPopulateList(int thePositionX, int[] theIndices, List> theInput, List> theOutput) { + private static void doCalculatePermutationsIntoIndicesArrayAndPopulateList( + int thePositionX, int[] theIndices, List> theInput, List> theOutput) { if (thePositionX != theInput.size()) { // If we're not at the end of the list of input vectors, recursively self-invoke once for each // possible option at the current position in the list of input vectors. for (int positionY = 0; positionY < theInput.get(thePositionX).size(); positionY++) { theIndices[thePositionX] = positionY; - doCalculatePermutationsIntoIndicesArrayAndPopulateList(thePositionX + 1, theIndices, theInput, theOutput); + doCalculatePermutationsIntoIndicesArrayAndPopulateList( + thePositionX + 1, theIndices, theInput, theOutput); } } else { // If we're at the end of the list of input vectors, then we've been passed the @@ -97,5 +99,4 @@ public class PermutationBuilder { } return retVal; } - } diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java index 698d064e423..34232b478a7 100644 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java @@ -353,22 +353,6 @@ public class SearchParameterMap implements Serializable { return this; } - /** - * This will only return true if all parameters have no modifier of any kind - */ - public boolean isAllParametersHaveNoModifier() { - for (List> nextParamName : values()) { - for (List nextAnd : nextParamName) { - for (IQueryParameterType nextOr : nextAnd) { - if (isNotBlank(nextOr.getQueryParameterQualifier())) { - return false; - } - } - } - } - return true; - } - /** * If set, tells the server to load these results synchronously, and not to load * more than X results diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboNonUniqueParamTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboNonUniqueParamTest.java index 10dd80e5c1b..5bc306cb048 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboNonUniqueParamTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ComboNonUniqueParamTest.java @@ -2,6 +2,7 @@ package ca.uhn.fhir.jpa.dao.r4; import ca.uhn.fhir.interceptor.api.IInterceptorService; import ca.uhn.fhir.jpa.model.entity.ResourceIndexedComboTokenNonUnique; +import ca.uhn.fhir.jpa.model.entity.StorageSettings; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.jpa.searchparam.submit.interceptor.SearchParamValidatingInterceptor; import ca.uhn.fhir.jpa.util.SqlQuery; @@ -58,6 +59,8 @@ public class FhirResourceDaoR4ComboNonUniqueParamTest extends BaseComboParamsR4T @AfterEach public void restoreInterceptor() { + myStorageSettings.setIndexMissingFields(new StorageSettings().getIndexMissingFields()); + if (myInterceptorFound) { myInterceptorService.unregisterInterceptor(mySearchParamValidatingInterceptor); } @@ -765,6 +768,58 @@ public class FhirResourceDaoR4ComboNonUniqueParamTest extends BaseComboParamsR4T } + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testStringModifier(boolean theUseExact) { + IIdType id1 = createObservation("2021-01-02"); + createObservation("2023-01-02"); + + SearchParameterMap params = SearchParameterMap + .newSynchronous() + .add("note-text", new StringParam("Hello").setExact(theUseExact)) + .add("date", new DateParam("2021-01-02")); + myCaptureQueriesListener.clear(); + IBundleProvider results = myObservationDao.search(params, mySrd); + List actual = toUnqualifiedVersionlessIdValues(results); + myCaptureQueriesListener.logSelectQueries(); + assertThat(actual).contains(id1.toUnqualifiedVersionless().getValue()); + + if (theUseExact) { + assertComboIndexNotUsed(); + assertThat(myMessages.toString()).contains("INFO Search with params [date, note-text] is not a candidate for combo searching - Parameter 'note-text' has modifier: ':exact'"); + } else { + assertComboIndexUsed(); + } + + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testMissing(boolean theUseMissing) { + myStorageSettings.setIndexMissingFields(StorageSettings.IndexEnabledEnum.ENABLED); + + IIdType id1 = createObservation("2021-01-02"); + createObservation("2023-01-02"); + + SearchParameterMap params = SearchParameterMap + .newSynchronous() + .add("note-text", new StringParam("Hello").setMissing(theUseMissing ? false : null)) + .add("date", new DateParam("2021-01-02")); + myCaptureQueriesListener.clear(); + IBundleProvider results = myObservationDao.search(params, mySrd); + List actual = toUnqualifiedVersionlessIdValues(results); + myCaptureQueriesListener.logSelectQueries(); + assertThat(actual).contains(id1.toUnqualifiedVersionless().getValue()); + + if (theUseMissing) { + assertComboIndexNotUsed(); + assertThat(myMessages.toString()).contains("INFO Search with params [date, note-text] is not a candidate for combo searching - Parameter 'note-text' has modifier: ':missing'"); + } else { + assertComboIndexUsed(); + } + + } + private void assertComboIndexUsed() { String sql = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false); assertThat(sql).contains("HFJ_IDX_CMB_TOK_NU");