Add `DateTimeType` to search #419

epicadk · 2021-04-02T11:08:22Z

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straighforward changes).

Fixes #375

Description
Clear and concise code change description.

Add date to search

Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Followed a similar approach to that of string and reference

Type
Choose one: Feature

Screenshots (if applicable)

Checklist

I have read and acknowledge the Code of conduct
I have read How to Contribute
I have read the Developer's guide
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 reference app(s) to verify my change fixes the issue and/or does not break the reference app(s)

jingtang10 · 2021-04-03T18:48:56Z

Well done in figuring out how the index tables work.

epicadk · 2021-04-03T20:51:10Z

Well done in figuring out how the index tables work.

Thankyou 😄. I'm not sure how I should handle the case when the Prefix is null should we throw an error or have a default value?

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

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

epicadk · 2021-04-12T19:12:15Z

@jingtang10 I've added a test for the SQL query. But I think we should resolve #433 before we merge this. Converting to draft will add tests for all the cases. I just wanted to know whether I should continue adding the tests in FhirEngineImplTest or should I add them to DatabaseImplTest ?

epicadk · 2021-04-19T03:56:50Z

@jingtang10 I've added a test for the SQL query. But I think we should resolve #433 before we merge this. Converting to draft will add tests for all the cases. I just wanted to know whether I should continue adding the tests in FhirEngineImplTest or should I add them to DatabaseImplTest ?

Hi, can I have some help here?

jingtang10 · 2021-04-19T14:43:41Z

Hi @epicadk sorry for the delay. I approved and merged #433. Can you mark this draft as ready for review? thanks

epicadk · 2021-04-19T14:49:53Z

Hi @epicadk sorry for the delay. I approved and merged #433. Can you mark this draft as ready for review? thanks

Marked as ready to review. But I haven't really added tests. I'm having trouble in figuring out where I should test if the query works as intended . Should I add the tests in the FHIRengineImpl test or the DatabaseImpl test?

epicadk · 2021-04-23T16:46:21Z

Added an empty commit because build test was failing here but seemed to be working fine locally. If it fails again I'll look into it.

epicadk · 2021-06-10T08:23:23Z

Hi @epicadk, could you please provide an ETA for completion of changes mentioned in the PR?

I think this and #436 are pretty similar. Once that is approved and ready to merge I'll make similar changes to this pr. I'd say we can get this done in the next 4-5 days

Tarun-Bhardwaj · 2021-06-15T13:57:53Z

@aditya-07 , could you please review the changes?

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

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

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

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

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

jingtang10

Awesome! 😎

Thanks for the PR!

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

deepankarb

Thanks @epicadk this is cool!
I've left a few non-blocking comments. LGTM, approved 🚀

deepankarb · 2021-06-21T15:37:19Z

engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt

+          .apply {
+            filter(Patient.DEATH_DATE) {
+              prefix = ParamPrefixEnum.STARTS_AFTER
+              value = DateTimeType("2013-03-14")


The DateTimeType supplied above to patient has time zone but not here. Will running the query in different time zones produce consistent results?

Can we add tests for that case or create an issue to add them later?

Thanks, I have opened #567

deepankarb · 2021-06-21T15:40:01Z

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

+): ConditionParam<Long> {
+  val (start, end) = value.rangeEpochMillis
+  return when (prefix) {
+    ParamPrefixEnum.APPROXIMATE -> TODO("Not Implemented")


Can we add more details here about ParamPrefixEnum.APPROXIMATE e.g. link to spec and reason for not implementing.

Also, an issue to track it :)

Thanks opened #568 and added a link to the issue in the TODO

deepankarb · 2021-06-21T15:43:52Z

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

+private val DateTimeType.rangeEpochMillis
+  get() = value.time to precision.add(value, 1).time - 1
+
+private data class ConditionParam<T>(val condition: String, val params: List<T>) {


Can we use varargs for params?

We can't use varargs in the primary constructor of a data class. I've created a secondary constructor for that reason. I realize I was still using listOf in the dateTime code, I've updated that now.

…into date_search

Add date to search

d765f84

Fix tests

81628a2

epicadk marked this pull request as ready for review April 3, 2021 20:28

epicadk requested review from deepankarb, jingtang10, kevinmost, pankajkatoch and yigit as code owners April 3, 2021 20:28

Handle ParamPrefix

aa2df65

epicadk force-pushed the date_search branch from d8b7553 to aa2df65 Compare April 3, 2021 20:31

Refractor: BigDecimal to Long

875fcda

epicadk force-pushed the date_search branch from b9fa5ef to 875fcda Compare April 3, 2021 20:45

jingtang10 reviewed Apr 12, 2021

View reviewed changes

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

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

Add test

a66fa6d

epicadk marked this pull request as draft April 12, 2021 19:12

Merge remote-tracking branch 'upstream/master' into date_search

dee5178

epicadk marked this pull request as ready for review April 19, 2021 14:47

epicadk and others added 3 commits April 22, 2021 21:26

Merge branch 'master' into date_search

d32bff4

Empty commit

f7b5701

Merge branch 'master' into date_search

2455119

Fix failing tests

f2baaf9

epicadk marked this pull request as draft May 10, 2021 06:47

Fix merge conflicts

348682d

epicadk and others added 3 commits June 12, 2021 10:31

Merge branch 'master' into date_search

f6c5dfb

Apply spotless

2610cce

Merge branch 'master' into date_search

5675762

Improve tests

38e4942

jingtang10 requested changes Jun 16, 2021

View reviewed changes

epicadk and others added 4 commits June 16, 2021 22:24

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

c166f67

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

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

f53a1d4

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

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

c5bae83

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

address review comments

9dbc124

epicadk force-pushed the date_search branch from a1640d8 to 9dbc124 Compare June 16, 2021 19:08

epicadk requested a review from jingtang10 June 16, 2021 19:08

jingtang10 requested changes Jun 17, 2021

View reviewed changes

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

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

address review comments

cda990a

epicadk force-pushed the date_search branch from 0727d82 to cda990a Compare June 17, 2021 09:44

epicadk requested a review from jingtang10 June 17, 2021 09:47

Tarun-Bhardwaj assigned jingtang10 Jun 18, 2021

jingtang10 approved these changes Jun 21, 2021

View reviewed changes

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

deepankarb approved these changes Jun 21, 2021

View reviewed changes

epicadk mentioned this pull request Jun 21, 2021

Ensure consistent search results across different timezones in date search #567

Open

epicadk and others added 4 commits June 22, 2021 00:13

address review comments

63bc9e8

Merge branch 'master' into date_search

0db7045

address review comments

cf68cf0

Merge branch 'date_search' of https://github.com/epicadk/android-fhir …

f2e5386

…into date_search

epicadk force-pushed the date_search branch from b734b82 to ab169f2 Compare June 22, 2021 03:38

address review comments

1c6e4a3

epicadk force-pushed the date_search branch from ab169f2 to 1c6e4a3 Compare June 22, 2021 03:45

epicadk merged commit 12c1163 into google:master Jun 22, 2021

epicadk deleted the date_search branch June 22, 2021 04:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `DateTimeType` to search #419

Add `DateTimeType` to search #419

epicadk commented Apr 2, 2021 •

edited by jingtang10

Loading

jingtang10 commented Apr 3, 2021

epicadk commented Apr 3, 2021

epicadk commented Apr 12, 2021 •

edited

Loading

epicadk commented Apr 19, 2021

jingtang10 commented Apr 19, 2021

epicadk commented Apr 19, 2021 •

edited

Loading

epicadk commented Apr 23, 2021 •

edited

Loading

epicadk commented Jun 10, 2021

Tarun-Bhardwaj commented Jun 15, 2021

jingtang10 left a comment

deepankarb left a comment

deepankarb Jun 21, 2021

epicadk Jun 21, 2021

deepankarb Jun 21, 2021

deepankarb Jun 21, 2021

epicadk Jun 21, 2021

deepankarb Jun 21, 2021

epicadk Jun 21, 2021

Add DateTimeType to search #419

Add DateTimeType to search #419

Conversation

epicadk commented Apr 2, 2021 • edited by jingtang10 Loading

jingtang10 commented Apr 3, 2021

epicadk commented Apr 3, 2021

epicadk commented Apr 12, 2021 • edited Loading

epicadk commented Apr 19, 2021

jingtang10 commented Apr 19, 2021

epicadk commented Apr 19, 2021 • edited Loading

epicadk commented Apr 23, 2021 • edited Loading

epicadk commented Jun 10, 2021

Tarun-Bhardwaj commented Jun 15, 2021

jingtang10 left a comment

Choose a reason for hiding this comment

deepankarb left a comment

Choose a reason for hiding this comment

deepankarb Jun 21, 2021

Choose a reason for hiding this comment

epicadk Jun 21, 2021

Choose a reason for hiding this comment

deepankarb Jun 21, 2021

Choose a reason for hiding this comment

deepankarb Jun 21, 2021

Choose a reason for hiding this comment

epicadk Jun 21, 2021

Choose a reason for hiding this comment

deepankarb Jun 21, 2021

Choose a reason for hiding this comment

epicadk Jun 21, 2021

Choose a reason for hiding this comment

Add `DateTimeType` to search #419

Add `DateTimeType` to search #419

epicadk commented Apr 2, 2021 •

edited by jingtang10

Loading

epicadk commented Apr 12, 2021 •

edited

Loading

epicadk commented Apr 19, 2021 •

edited

Loading

epicadk commented Apr 23, 2021 •

edited

Loading