Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Return unique include and revInclude resources per search filter match. #2544

Merged
merged 20 commits into from
Jul 24, 2024
Merged

Return unique include and revInclude resources per search filter match. #2544

merged 20 commits into from
Jul 24, 2024

Conversation

aditya-07
Copy link
Collaborator

@aditya-07 aditya-07 commented May 14, 2024 •
edited
Loading

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

Fixes #2512 #2567

Description

  1. Return distinct included and revIncluded resource per searchIndex.
  2. When user updates a resource, if the resource has an meta.lastUpdated, use that for index value.

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

Type
Choose one: Bug fix
Screenshots (if applicable)

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).

@aditya-07 aditya-07 requested review from santosh-pingle and a team as code owners May 14, 2024 11:58
MJ1998
MJ1998 approved these changes May 27, 2024
View reviewed changes
Copy link
Collaborator

@MJ1998 MJ1998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. NIT comment.

engine/src/main/java/com/google/android/fhir/db/impl/dao/ResourceDao.kt Show resolved Hide resolved
@aditya-07 aditya-07 enabled auto-merge (squash) May 30, 2024 09:35
@aditya-07 aditya-07 disabled auto-merge May 31, 2024 13:06
jingtang10
jingtang10 requested changes May 31, 2024
View reviewed changes
Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you double check on the performance impact of this? doesn't have to produce a comprehensive report - but would be godo at least for the dataset in the github issue this is not having an appreciatable impact.

jingtang10
jingtang10 approved these changes Jun 19, 2024
View reviewed changes
Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls add your findings using the query plan, and performance tests.

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jun 26, 2024
@ndegwamartin
SDK Release
c78f967 
    - With unmerged PR #9
    - WUP PR google#2178
    - WUP PR google#2511
    - WUP PR google#2511
    - WUP PR google#2544
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jul 9, 2024
@ndegwamartin
SDK Release
3ef1357 
        - With unmerged PR #9
        - WUP PR google#2178
        - WUP PR google#2544
        - WUP PR google#2607
jingtang10
jingtang10 requested changes Jul 10, 2024
View reviewed changes
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt Outdated Show resolved Hide resolved
engine/src/main/java/com/google/android/fhir/search/BaseSearch.kt Outdated Show resolved Hide resolved
engine/src/main/java/com/google/android/fhir/search/BaseSearch.kt Outdated Show resolved Hide resolved
engine/src/main/java/com/google/android/fhir/search/MoreSearch.kt Show resolved Hide resolved
engine/src/test/java/com/google/android/fhir/search/SearchTest.kt Show resolved Hide resolved
engine/src/main/java/com/google/android/fhir/search/MoreSearch.kt Show resolved Hide resolved
engine/src/test/java/com/google/android/fhir/search/SearchTest.kt Show resolved Hide resolved
engine/src/test/java/com/google/android/fhir/search/SearchTest.kt Show resolved Hide resolved
@aditya-07 aditya-07 merged commit 5589179 into google:master Jul 24, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MJ1998 MJ1998 MJ1998 approved these changes

@jingtang10 jingtang10 jingtang10 approved these changes

@santosh-pingle santosh-pingle Awaiting requested review from santosh-pingle santosh-pingle is a code owner

@elegantmush elegantmush Awaiting requested review from elegantmush elegantmush was automatically assigned from google/android-fhir

Assignees
No one assigned
Labels
None yet
Projects
Android FHIR SDK
Status: Complete
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Several records of resources with same resource Id returned leading to duplication
4 participants
@aditya-07 @MJ1998 @jingtang10 @android-fhir-bot