Skip to content

Commit 98cce2a

Browse files
jmarchionattomichaelabuckleyjuan.marchionatto
authored
3664 fast elastic load (hapifhir#3769)
* Start direct HSearch path * Support no HSearch * Spike out the direct resource query * Implement hsearch fast load * Fix last master merge in issues * Implement revision requests * Test direct resources (no IDs query) sorting * Use mock to count freetext searches to avoid implementing interface in test * Remove fixme * Make listener optional as it is used only for tests * Provide new dependency * Widen fast path test scope and fix previously untested configurations * Make method transactional as it can be called from outside a TX (at least testObservationLastNAllParamsPopulated does) * Update test validation Co-authored-by: Michael Buckley <[email protected]> Co-authored-by: juan.marchionatto <[email protected]>
1 parent 5575e72 commit 98cce2a

File tree

8 files changed

+51
-75
lines changed

8 files changed

+51
-75
lines changed

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FulltextSearchSvcImpl.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ public List<IBaseResource> getResources(Collection<Long> thePids) {
359359
dispatchEvent(IHSearchEventListener.HSearchEventType.SEARCH);
360360
List<ExtendedHSearchResourceProjection> rawResourceDataList = session.search(ResourceTable.class)
361361
.select(
362-
f -> buildResourceSelectClause(f)
362+
this::buildResourceSelectClause
363363
)
364364
.where(
365365
f -> f.id().matchingAny(thePids) // matches '_id' from resource index
@@ -403,11 +403,12 @@ public long count(String theResourceName, SearchParameterMap theParams) {
403403

404404

405405
@Override
406+
@Transactional(readOnly = true)
406407
public List<IBaseResource> searchForResources(String theResourceType, SearchParameterMap theParams) {
407408
int offset = 0; int limit = DEFAULT_MAX_PAGE_SIZE;
408409
if (theParams.getOffset() != null && theParams.getOffset() != 0) {
409410
offset = theParams.getOffset();
410-
limit = theParams.getCount();
411+
limit = theParams.getCount() == null ? DEFAULT_MAX_PAGE_SIZE : theParams.getCount();
411412
// indicate param was already processed, otherwise queries DB to process it
412413
theParams.setOffset(null);
413414
}

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
2525
import ca.uhn.fhir.model.api.IQueryParameterType;
2626
import ca.uhn.fhir.rest.api.Constants;
27+
import ca.uhn.fhir.rest.api.SearchContainedModeEnum;
2728
import ca.uhn.fhir.rest.param.DateParam;
2829
import ca.uhn.fhir.rest.param.NumberParam;
2930
import ca.uhn.fhir.rest.param.QuantityParam;
@@ -34,6 +35,8 @@
3435
import ca.uhn.fhir.rest.server.util.ISearchParamRegistry;
3536
import com.google.common.collect.Lists;
3637
import com.google.common.collect.Sets;
38+
import org.apache.commons.collections4.CollectionUtils;
39+
import org.apache.commons.lang3.BooleanUtils;
3740
import org.apache.commons.lang3.StringUtils;
3841

3942
import java.util.ArrayList;
@@ -72,16 +75,16 @@ public boolean isSupportsSomeOf(SearchParameterMap myParams) {
7275
*/
7376
public boolean isSupportsAllOf(SearchParameterMap myParams) {
7477
return
75-
myParams.getRevIncludes() == null && // ???
76-
myParams.getIncludes() == null && // ???
78+
CollectionUtils.isEmpty( myParams.getRevIncludes() ) && // ???
79+
CollectionUtils.isEmpty( myParams.getIncludes() ) && // ???
7780
myParams.getEverythingMode() == null && // ???
78-
! myParams.isDeleteExpunge() && // ???
81+
BooleanUtils.isFalse( myParams.isDeleteExpunge() ) && // ???
7982

8083
// not yet supported in HSearch
8184
myParams.getNearDistanceParam() == null && // ???
8285

8386
// not yet supported in HSearch
84-
myParams.getSearchContainedMode() == null && // ???
87+
myParams.getSearchContainedMode() == SearchContainedModeEnum.FALSE && // ???
8588

8689
myParams.entrySet().stream()
8790
.filter(e -> !ourUnsafeSearchParmeters.contains(e.getKey()))
@@ -135,10 +138,8 @@ private boolean isParamTypeSupported(IQueryParameterType param) {
135138
return false;
136139
}
137140
} else if (param instanceof DateParam) {
138-
if (EMPTY_MODIFIER.equals(modifier)) {
139-
return true;
140-
}
141-
return false;
141+
return modifier.equals(EMPTY_MODIFIER);
142+
142143
} else if (param instanceof UriParam) {
143144
return modifier.equals(EMPTY_MODIFIER);
144145

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,7 @@ public IBundleProvider registerSearch(final IFhirResourceDao<?> theCallingDao, f
362362
// return searchStrategy.get();
363363

364364
if (mySearchStrategyFactory.isSupportsHSearchDirect(theResourceType, theParams, theRequestDetails)) {
365+
ourLog.info("Search {} is using direct load strategy", searchUuid);
365366
SearchStrategyFactory.ISearchStrategy direct = mySearchStrategyFactory.makeDirectStrategy(searchUuid, theResourceType, theParams, theRequestDetails);
366367
return direct.get();
367368
}

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchStrategyFactory.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import org.hl7.fhir.instance.model.api.IBaseResource;
3030

3131
import javax.annotation.Nullable;
32+
import java.util.Collections;
3233
import java.util.List;
3334
import java.util.function.Supplier;
3435

@@ -68,10 +69,13 @@ public boolean isSupportsHSearchDirect(String theResourceType, SearchParameterMa
6869

6970
public ISearchStrategy makeDirectStrategy(String theSearchUUID, String theResourceType, SearchParameterMap theParams, RequestDetails theRequestDetails) {
7071
return () -> {
72+
if (myFulltextSearchSvc == null) {
73+
return new SimpleBundleProvider(Collections.emptyList(), theSearchUUID);
74+
}
75+
7176
List<IBaseResource> resources = myFulltextSearchSvc.searchForResources(theResourceType, theParams);
7277
SimpleBundleProvider result = new SimpleBundleProvider(resources, theSearchUUID);
73-
// we don't know the size
74-
result.setSize(null);
78+
result.setSize(resources.size());
7579
return result;
7680
};
7781
}

‎hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseR4SearchLastN.java‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -226,12 +226,8 @@ public void testLastNNoPatients() {
226226
private void executeTestCase(SearchParameterMap params, List<String> sortedPatients, List<String> sortedObservationCodes, List<String> theCategories, int expectedObservationCount) {
227227
List<String> actual;
228228
params.setLastN(true);
229-
230-
Map<String, String[]> requestParameters = new HashMap<>();
231229
params.setLastNMax(100);
232230

233-
when(mySrd.getParameters()).thenReturn(requestParameters);
234-
235231
actual = toUnqualifiedVersionlessIdValues(myObservationDao.observationsLastN(params, mockSrd(), null));
236232

237233
assertEquals(expectedObservationCount, actual.size());

‎hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchLastNIT.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,6 @@ public void testLastN_onEnablingStoringObservationWithIndexMapping_shouldSkipLoa
125125
when(mySrd.getParameters()).thenReturn(requestParameters);
126126

127127
List<String> results = toUnqualifiedVersionlessIdValues(myObservationDao.observationsLastN(params, mockSrd(), null));
128-
verifyResourcesLoadedFromElastic(observationIds, results);
129128

130129
}
131130

‎hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchLastNUsingExtendedHSearchIndexIT.java‎

Lines changed: 16 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,18 @@
11
package ca.uhn.fhir.jpa.dao.r4;
22

33
import ca.uhn.fhir.jpa.api.config.DaoConfig;
4-
import ca.uhn.fhir.jpa.dao.IFulltextSearchSvc;
4+
import ca.uhn.fhir.jpa.dao.IHSearchEventListener;
5+
import ca.uhn.fhir.jpa.test.util.TestHSearchEventDispatcher;
56
import org.hl7.fhir.instance.model.api.IIdType;
67
import org.junit.jupiter.api.AfterEach;
78
import org.junit.jupiter.api.BeforeEach;
89
import org.junit.jupiter.api.extension.ExtendWith;
9-
import org.mockito.ArgumentCaptor;
10+
import org.mockito.Mock;
11+
import org.mockito.Mockito;
1012
import org.springframework.beans.factory.annotation.Autowired;
11-
import org.springframework.boot.test.mock.mockito.SpyBean;
1213
import org.springframework.test.context.junit.jupiter.SpringExtension;
1314

1415
import java.util.List;
15-
import java.util.stream.Collectors;
16-
17-
import static org.hamcrest.MatcherAssert.assertThat;
18-
import static org.hamcrest.Matchers.is;
19-
import static org.junit.jupiter.api.Assertions.assertEquals;
20-
import static org.mockito.Mockito.times;
21-
import static org.mockito.Mockito.verify;
2216

2317
/**
2418
* Run entire @see {@link FhirResourceDaoR4SearchLastNIT} test suite this time
@@ -28,14 +22,18 @@
2822
*/
2923
@ExtendWith(SpringExtension.class)
3024
public class FhirResourceDaoR4SearchLastNUsingExtendedHSearchIndexIT extends FhirResourceDaoR4SearchLastNIT {
31-
// awkward override so we can spy
32-
@SpyBean
33-
@Autowired(required = false)
34-
IFulltextSearchSvc myFulltestSearchSvc;
25+
26+
@Autowired
27+
private TestHSearchEventDispatcher myHSearchEventDispatcher;
28+
29+
@Mock
30+
private IHSearchEventListener mySearchEventListener;
31+
3532

3633
@BeforeEach
3734
public void enableAdvancedHSearchIndexing() {
3835
myDaoConfig.setAdvancedHSearchIndexing(true);
36+
myHSearchEventDispatcher.register(mySearchEventListener);
3937
}
4038

4139
@AfterEach
@@ -44,24 +42,13 @@ public void disableAdvancedHSearchIndex() {
4442
}
4543

4644
/**
47-
* We pull the resources from Hibernate Search when LastN uses Hibernate Search.
48-
* Override the test verification
45+
* We pull the resources from Hibernate Search when LastN uses Hibernate Search
46+
* Override the test verification to validate only one search was performed
4947
*/
5048
@Override
5149
void verifyResourcesLoadedFromElastic(List<IIdType> theObservationIds, List<String> theResults) {
52-
List<Long> expectedArgumentPids =
53-
theObservationIds.stream().map(IIdType::getIdPartAsLong).collect(Collectors.toList());
54-
55-
ArgumentCaptor<List<Long>> actualPids = ArgumentCaptor.forClass(List.class);
56-
57-
verify(myFulltestSearchSvc, times(1)).getResources(actualPids.capture());
58-
assertThat(actualPids.getValue(), is(expectedArgumentPids));
59-
60-
// we don't include the type in the id returned from Hibernate Search for now.
61-
List<String> expectedObservationList = theObservationIds.stream()
62-
.map(id -> id.toUnqualifiedVersionless().getIdPart()).collect(Collectors.toList());
63-
assertEquals(expectedObservationList, theResults);
64-
50+
Mockito.verify(mySearchEventListener, Mockito.times(1))
51+
.hsearchEvent(IHSearchEventListener.HSearchEventType.SEARCH);
6552
}
6653

6754
}

‎hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java‎

Lines changed: 16 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
1111
import ca.uhn.fhir.jpa.api.svc.ISearchCoordinatorSvc;
1212
import ca.uhn.fhir.jpa.bulk.export.api.IBulkDataExportJobSchedulingHelper;
13-
import ca.uhn.fhir.jpa.dao.IFulltextSearchSvc;
1413
import ca.uhn.fhir.jpa.dao.IHSearchEventListener;
1514
import ca.uhn.fhir.jpa.dao.TestDaoSearch;
1615
import ca.uhn.fhir.jpa.dao.data.IResourceTableDao;
@@ -22,8 +21,6 @@
2221
import ca.uhn.fhir.jpa.model.entity.ResourceTable;
2322
import ca.uhn.fhir.jpa.partition.SystemRequestDetails;
2423
import ca.uhn.fhir.jpa.search.reindex.IResourceReindexingSvc;
25-
import ca.uhn.fhir.jpa.searchparam.MatchUrlService;
26-
import ca.uhn.fhir.jpa.searchparam.ResourceSearch;
2724
import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
2825
import ca.uhn.fhir.jpa.sp.ISearchParamPresenceSvc;
2926
import ca.uhn.fhir.jpa.term.api.ITermCodeSystemStorageSvc;
@@ -211,14 +208,9 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl
211208
@RegisterExtension
212209
LogbackLevelOverrideExtension myLogbackLevelOverrideExtension = new LogbackLevelOverrideExtension();
213210

214-
@Autowired
215-
private IFulltextSearchSvc myIFulltextSearchSvc;
216-
217211
@Autowired
218212
private TestHSearchEventDispatcher myHSearchEventDispatcher;
219213

220-
@Autowired
221-
private MatchUrlService myMatchUrlService;
222214

223215
@Mock private IHSearchEventListener mySearchEventListener;
224216

@@ -2282,32 +2274,27 @@ public void allTogetherNow() {
22822274

22832275
}
22842276

2285-
@Nested
2286-
public class NoIdsQuery {
2287-
2288-
@Test
2289-
public void simpleTokenSkipsSql() {
2290-
String idA = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "code-a"))).getIdPart();
2291-
String idC = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "code-c"))).getIdPart();
2292-
String idB = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "code-b"))).getIdPart();
2293-
myCaptureQueriesListener.clear();
2294-
myHSearchEventDispatcher.register(mySearchEventListener);
2295-
2296-
List<IBaseResource> result = searchForFastResources("Observation?_sort=-code");
2297-
myCaptureQueriesListener.logSelectQueriesForCurrentThread();
2277+
@Test
2278+
public void directResourceLoadWhenSorting() {
2279+
String idA = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "code-a"))).getIdPart();
2280+
String idC = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "code-c"))).getIdPart();
2281+
String idB = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "code-b"))).getIdPart();
2282+
myCaptureQueriesListener.clear();
2283+
myHSearchEventDispatcher.register(mySearchEventListener);
22982284

2299-
assertThat( result.stream().map(r -> r.getIdElement().getIdPart()).collect(Collectors.toList()), contains(idC, idB, idA) );
2300-
assertEquals(0, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size(), "we build the bundle with no sql");
2285+
List<IBaseResource> result = searchForFastResources("Observation?_sort=-code");
2286+
myCaptureQueriesListener.logSelectQueriesForCurrentThread();
23012287

2302-
// only one hibernate search took place
2303-
Mockito.verify(mySearchEventListener, Mockito.times(1)).hsearchEvent(IHSearchEventListener.HSearchEventType.SEARCH);
2304-
}
2288+
assertThat( result.stream().map(r -> r.getIdElement().getIdPart()).collect(Collectors.toList()), contains(idC, idB, idA) );
2289+
assertEquals(0, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size(), "we build the bundle with no sql");
23052290

2291+
// only one hibernate search took place
2292+
Mockito.verify(mySearchEventListener, Mockito.times(1)).hsearchEvent(IHSearchEventListener.HSearchEventType.SEARCH);
23062293
}
23072294

2308-
23092295
}
23102296

2297+
23112298
@Nested
23122299
public class NumberParameter {
23132300

@@ -2540,14 +2527,14 @@ private void assertNotFindId(String theMessage, IIdType theResourceId, String th
25402527
public List<IBaseResource> searchForFastResources(String theQueryUrl) {
25412528
SearchParameterMap map = myTestDaoSearch.toSearchParameters(theQueryUrl);
25422529
map.setLoadSynchronous(true);
2530+
25432531
SortSpec sort = (SortSpec) new ca.uhn.fhir.rest.server.method.SortParameter(myFhirCtx)
25442532
.translateQueryParametersIntoServerArgument(fakeRequestDetailsFromUrl(theQueryUrl), null);
25452533
if (sort != null) {
25462534
map.setSort(sort);
25472535
}
25482536

2549-
ResourceSearch search = myMatchUrlService.getResourceSearch(theQueryUrl);
2550-
return runInTransaction( () -> myIFulltextSearchSvc.searchForResources(search.getResourceName(), map) );
2537+
return myTestDaoSearch.searchForResources(theQueryUrl);
25512538
}
25522539

25532540

0 commit comments

Comments
 (0)