Fix FHIR Engine search sort for DateTime properties #1384

ekigamba · 2022-05-11T08:31:49Z

Description
Implements a double-join to the tables DateTimeIndexEntity and DateIndexEntity whenever a DateClientParam sort is requested

Alternative(s) considered
Yes, a number of alternatives were considered and discussed here namely:

Merging the two tables into 1
Generating code to hint/guide on the right table depending the the sort property
Add a DateTimeClientParam

Type
Choose one: Bug fix

Screenshots (if applicable)
N/A

Checklist

I have read and acknowledged the Code of conduct.
I have read the Contributing page.
I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

codecov · 2022-05-12T08:27:20Z

Codecov Report

Merging #1384 (26f8e79) into master (8c82ee2) will increase coverage by 85.66%.
The diff coverage is 100.00%.

❗ Current head 26f8e79 differs from pull request most recent head 875fd41. Consider uploading reports for the commit 875fd41 to get more accurate results

@@              Coverage Diff              @@
##             master    #1384       +/-   ##
=============================================
+ Coverage          0   85.66%   +85.66%     
- Complexity        0      714      +714     
=============================================
  Files             0      150      +150     
  Lines             0    10749    +10749     
  Branches          0      856      +856     
=============================================
+ Hits              0     9208     +9208     
- Misses            0     1104     +1104     
- Partials          0      437      +437

Impacted Files	Coverage Δ
.../google/android/fhir/search/DateTimeClientParam.kt	`100.00% <100.00%> (ø)`
.../java/com/google/android/fhir/search/MoreSearch.kt	`95.45% <100.00%> (ø)`
...le/android/fhir/db/impl/entities/UriIndexEntity.kt	`0.00% <0.00%> (ø)`
...apture/views/QuestionnaireItemViewHolderFactory.kt	`100.00% <0.00%> (ø)`
...r/datacapture/views/QuestionnaireItemHeaderView.kt	`90.90% <0.00%> (ø)`
...main/java/com/google/android/fhir/DatastoreUtil.kt	`80.00% <0.00%> (ø)`
...estionnaireItemEditTextStringViewHolderDelegate.kt	`88.88% <0.00%> (ø)`
.../QuestionnaireItemDialogSelectViewHolderFactory.kt	`80.00% <0.00%> (ø)`
...oid/fhir/search/filter/DateParamFilterCriterion.kt	`88.09% <0.00%> (ø)`
...e/android/fhir/db/ResourceNotFoundInDbException.kt	`0.00% <0.00%> (ø)`
... and 141 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

Tarun-Bhardwaj · 2022-05-16T09:36:32Z

@ekigamba , when can we expect this PR to get completed?

ekigamba · 2022-05-16T11:58:55Z

@Tarun-Bhardwaj This PR is complete

aditya-07 · 2022-05-18T12:35:52Z

engine/src/main/java/com/google/android/fhir/search/DateTimeClientParam.kt

+
+import ca.uhn.fhir.rest.gclient.DateClientParam
+
+class DateTimeClientParam(theParamName: String) : DateClientParam(theParamName)


This might not be intuitive to the app developers that they need to pass DateTimeClientParam for date & time in the search sort function . Also they will have to re-define the DateClientParam define in hapi lib as a DateTimeClientParam now.

fun sort(parameter: DateClientParam, order: Order) { sort = parameter this.order = order }

One way to handle date-time could be to pass a boolean param in the sort indicating the type of search i.e Date or DateTime.
Any thoughts @jingtang10 ?

Thanks for bringing it up:

We can have below function to make it more intuitive

fun sort(parameter: DateTimeClientParam, order: Order) { sort = parameter this.order = order }

Do we really need to define DateTimeClientParam in the hapi lib since this is only used for sorting on this lib? Having extended DateClientParam means it can be reused for other places where DateClientParam is supposed to be used without changing the code to accomodate the new class. It's only role will be better readability when passed as a param in other DateClientParam functions

Alternatively, we can use a boolean as you stated. This will require us to track the boolean and have it as a class property of Search that will only be used whenever the sort param is a DateClientParam

jingtang10 · 2022-05-23T14:10:57Z

discussed this on thursday (@f-odhiambo @ekigamba @aditya-07 @maimoonak). the root problem is the fact that there is only 1 type of search param for both date and datetime indices, but at run time we don't know which index table to join.

we talked about 2 options:

merge date index table with datetime index table.
update the generated code so that there's a hint as to which params are for date index and which are for datetime index.

@ekigamba can you investigate these 2 options a little bit and discuss the pros and cons so we can make a decision? thanks

Tarun-Bhardwaj · 2022-06-15T11:52:25Z

discussed this on thursday (@f-odhiambo @ekigamba @aditya-07 @maimoonak). the root problem is the fact that there is only 1 type of search param for both date and datetime indices, but at run time we don't know which index table to join.

we talked about 2 options:

merge date index table with datetime index table.

update the generated code so that there's a hint as to which params are for date index and which are for datetime index.

@ekigamba can you investigate these 2 options a little bit and discuss the pros and cons so we can make a decision? thanks

@ekigamba , did you get a chance to work on the comments provided by @jingtang10 ?

ekigamba · 2022-06-15T12:57:31Z

discussed this on thursday (@f-odhiambo @ekigamba @aditya-07 @maimoonak). the root problem is the fact that there is only 1 type of search param for both date and datetime indices, but at run time we don't know which index table to join.
we talked about 2 options:

merge date index table with datetime index table.

update the generated code so that there's a hint as to which params are for date index and which are for datetime index.

@ekigamba can you investigate these 2 options a little bit and discuss the pros and cons so we can make a decision? thanks

@ekigamba , did you get a chance to work on the comments provided by @jingtang10 ?

Yea, in progress. Investigating both and how Hapi FHIR server handles this currently

jingtang10 · 2022-07-05T17:04:42Z

#1363 (comment)

- Add tests for the double-join and double sort for FHIR engine queries Fixes google#1363

- Update query tests

ekigamba · 2022-07-18T09:50:48Z

@jingtang10 Kindly review this

jingtang10 · 2022-07-20T15:09:21Z

engine/src/main/java/com/google/android/fhir/search/MoreSearch.kt

+
+ sortArgs += sort.paramName
+
+ // Fixes issue https://github.com/google/android-fhir/issues/1363
+ if (sort is DateClientParam) {
+ sortJoinStatement +=
+ """
+ LEFT JOIN ${SortTableInfo.DATE_TIME_SORT_TABLE_INFO.tableName} bDateTime
+ ON a.resourceType = bDateTime.resourceType AND a.resourceUuid = bDateTime.resourceUuid AND bDateTime.index_name = ?
+ """
+
+ sortArgs += sort.paramName
+ }
+
 sortOrderStatement =
 """
 ORDER BY b.${sortTableName.columnName} ${order.sqlString}
 """.trimIndent()
- sortArgs += sort.paramName
+
+ // Fixes issue https://github.com/google/android-fhir/issues/1363
+ if (sort is DateClientParam) {
+ sortOrderStatement +=
+ ", bDateTime.${SortTableInfo.DATE_TIME_SORT_TABLE_INFO.columnName} ${order.sqlString}"
+ }


this code is very procedural and difficult to read - i think what should happen is for the when clause on line 65, you sould return a list of SortTableInfos. And for each of them, you add the join statement and the sort order statement. that way it's much clearer.

so in this case or the DateClientParam, you just need to add both date and datetime tables.

I have made the changes as requested. However, I think that the forEachIndexed can be confusing since it makes it seem as if the engine allows multiple sort params. It is also not clear that the are implemented for an exception. The for loops feel like generic solutions while they help fix the issue for 1 exception.

@jingtang10

Remove the if for DateTimeClient when performing double-joins and use a generic for loop

jingtang10

just one comment on test - otherwise good to go

engine/src/test/java/com/google/android/fhir/search/SearchTest.kt

engine/src/main/java/com/google/android/fhir/search/MoreSearch.kt

Co-authored-by: Jing Tang <[email protected]>

jingtang10

thanks @ekigamba

ekigamba force-pushed the ek/1363-search-date-time-property-not-working branch from 54c0366 to cc725cd Compare May 16, 2022 06:27

Rkareko approved these changes May 16, 2022

View reviewed changes

Tarun-Bhardwaj assigned ekigamba May 16, 2022

ekigamba marked this pull request as ready for review May 16, 2022 11:58

ekigamba requested review from aditya-07, jingtang10, kevinmost, ktarasenko, omarismail94, stevenckngaa, vvikramraj, wantingzhang77 and yigit as code owners May 16, 2022 11:58

aditya-07 reviewed May 18, 2022

View reviewed changes

Tarun-Bhardwaj added the P1 High priority issue label Jun 27, 2022

Fix FHIR search datetime sort adding double-join

b24b4c9

- Add tests for the double-join and double sort for FHIR engine queries Fixes google#1363

ekigamba force-pushed the ek/1363-search-date-time-property-not-working branch from 26f8e79 to b24b4c9 Compare July 14, 2022 14:57

Spotless fixes

5719d00

ekigamba force-pushed the ek/1363-search-date-time-property-not-working branch 2 times, most recently from 368fe62 to 0bff104 Compare July 15, 2022 13:29

Fix FHIR search datetime sort double-join query

f95cabf

- Update query tests

ekigamba force-pushed the ek/1363-search-date-time-property-not-working branch from 0bff104 to f95cabf Compare July 15, 2022 13:45

Rkareko approved these changes Jul 18, 2022

View reviewed changes

jingtang10 requested changes Jul 20, 2022

View reviewed changes

Refactor updates to use generic for-loops

30b6cba

Remove the if for DateTimeClient when performing double-joins and use a generic for loop

jingtang10 requested changes Jul 22, 2022

View reviewed changes

engine/src/test/java/com/google/android/fhir/search/SearchTest.kt Outdated Show resolved Hide resolved

jingtang10 requested changes Jul 22, 2022

View reviewed changes

engine/src/main/java/com/google/android/fhir/search/MoreSearch.kt Show resolved Hide resolved

ekigamba and others added 3 commits July 25, 2022 17:21

Apply suggestions from code review

2724622

Co-authored-by: Jing Tang <[email protected]>

Merge branch 'master' into ek/1363-search-date-time-property-not-working

df53a1b

Fix styling

875fd41

jingtang10 approved these changes Jul 25, 2022

View reviewed changes

jingtang10 merged commit ac746f4 into google:master Jul 25, 2022

ekigamba deleted the ek/1363-search-date-time-property-not-working branch July 25, 2022 19:29

Tarun-Bhardwaj added the FHIR engine label Jul 28, 2022

aditya-07 added the type:bug Something isn't working label Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix FHIR Engine search sort for DateTime properties #1384

Fix FHIR Engine search sort for DateTime properties #1384

ekigamba commented May 11, 2022 •

edited

Loading

codecov bot commented May 12, 2022 •

edited

Loading

Tarun-Bhardwaj commented May 16, 2022

ekigamba commented May 16, 2022

aditya-07 May 18, 2022

ekigamba May 18, 2022

jingtang10 commented May 23, 2022

Tarun-Bhardwaj commented Jun 15, 2022

ekigamba commented Jun 15, 2022

jingtang10 commented Jul 5, 2022

ekigamba commented Jul 18, 2022

jingtang10 Jul 20, 2022

ekigamba Jul 25, 2022

ekigamba Jul 26, 2022

jingtang10 left a comment

jingtang10 left a comment


		import ca.uhn.fhir.rest.gclient.DateClientParam

		class DateTimeClientParam(theParamName: String) : DateClientParam(theParamName)

Fix FHIR Engine search sort for DateTime properties #1384

Fix FHIR Engine search sort for DateTime properties #1384

Conversation

ekigamba commented May 11, 2022 • edited Loading

codecov bot commented May 12, 2022 • edited Loading

Codecov Report

Tarun-Bhardwaj commented May 16, 2022

ekigamba commented May 16, 2022

aditya-07 May 18, 2022

Choose a reason for hiding this comment

ekigamba May 18, 2022

Choose a reason for hiding this comment

jingtang10 commented May 23, 2022

Tarun-Bhardwaj commented Jun 15, 2022

ekigamba commented Jun 15, 2022

jingtang10 commented Jul 5, 2022

ekigamba commented Jul 18, 2022

jingtang10 Jul 20, 2022

Choose a reason for hiding this comment

ekigamba Jul 25, 2022

Choose a reason for hiding this comment

ekigamba Jul 26, 2022

Choose a reason for hiding this comment

jingtang10 left a comment

Choose a reason for hiding this comment

jingtang10 left a comment

Choose a reason for hiding this comment

ekigamba commented May 11, 2022 •

edited

Loading

codecov bot commented May 12, 2022 •

edited

Loading