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.

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

Add index to DateTimeIndexEntity table index_from column #1964

Merged

Conversation

ellykits
Copy link
Collaborator

@ellykits ellykits commented Apr 12, 2023

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

Fixes #1944

Description
Index the column that contains _lastUpdatedAt date to improve performance on search/sorting

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

Type
Choose one: (Bug fix | Feature | Documentation | Testing | Code health | Builds | Releases | Other)

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

allan-on added a commit to opensrp/android-fhir that referenced this pull request Apr 26, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
  - With unmerged PR google#1978
  - With unmerged PR google#1964
allan-on added a commit to opensrp/android-fhir that referenced this pull request Apr 26, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
  - With unmerged PR google#1978
  - With unmerged PR google#1964
Copy link
Collaborator

@aditya-07 aditya-07 left a comment

Choose a reason for hiding this comment

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

@ellykits Can you please confirm that the index works as expected while sorting data.

@aditya-07
Copy link
Collaborator

During the testing on my end, I found that the query plan seems to skip this index (_lastUpdatedAt) when used to sort data. But it looks like even though the query plan seems to skip it, having this index makes the query faster See _lastUpdatedAt Performance Tests discussion.

@ellykits
Copy link
Collaborator Author

I'm okay with merging this @jingtang10

@aditya-07
Copy link
Collaborator

@ellykits Can you please update the branch to latest.

@ellykits ellykits requested review from a team and santosh-pingle as code owners May 11, 2023 12:32
@ellykits
Copy link
Collaborator Author

During the testing on my end, I found that the query plan seems to skip this index (_lastUpdatedAt) when used to sort data. But it looks like even though the query plan seems to skip it, having this index makes the query faster See _lastUpdatedAt Performance Tests discussion.

Got the following response from this query that searches & sort Groups resources to confirm @aditya-07's observation.

EXPLAIN QUERY PLAN SELECT a.serializedResource
FROM ResourceEntity a
         LEFT JOIN DateIndexEntity b
                   ON a.resourceType = b.resourceType AND a.resourceUuid = b.resourceUuid AND b.index_name = '_lastUpdated'
         LEFT JOIN DateTimeIndexEntity c
                   ON a.resourceType = c.resourceType AND a.resourceUuid = c.resourceUuid AND c.index_name = '_lastUpdated'
WHERE a.resourceType = 'Group'
  AND a.resourceUuid IN (SELECT resourceUuid
                         FROM TokenIndexEntity
                         WHERE resourceType = 'Group'
                           AND index_name = 'type'
                           AND (index_value = 'person' AND IFNULL(index_system, '') = 'http:https://hl7.org/fhir/group-type'))
  AND a.resourceUuid IN (SELECT resourceUuid
                         FROM TokenIndexEntity
                         WHERE resourceType = 'Group'
                           AND index_name = 'code'
                           AND (index_value = '35359004' AND IFNULL(index_system, '') = 'https://www.snomed.org'))
ORDER BY b.index_from DESC, c.index_from DESC
LIMIT 10 OFFSET 0;
12	0	0	SEARCH a USING INDEX index_ResourceEntity_resourceType_resourceId (resourceType=?)
20	0	0	LIST SUBQUERY 1
23	20	0	SEARCH TokenIndexEntity USING INDEX index_TokenIndexEntity_resourceType_index_name_index_system_index_value (resourceType=? AND index_name=?)
46	0	0	LIST SUBQUERY 2
49	46	0	SEARCH TokenIndexEntity USING INDEX index_TokenIndexEntity_resourceType_index_name_index_system_index_value (resourceType=? AND index_name=?)
69	0	0	SEARCH b USING INDEX index_DateIndexEntity_resourceType_index_name_index_from_index_to (resourceType=? AND index_name=?) LEFT-JOIN
80	0	0	SEARCH c USING INDEX index_DateTimeIndexEntity_resourceType_index_name_index_from_index_to (resourceType=? AND index_name=?) LEFT-JOIN
112	0	0	USE TEMP B-TREE FOR ORDER BY

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request May 11, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
  - With unmerged PR google#1978
  - With unmerged PR google#1964
  - With unmerged PR google#1963
  - With unmerged PR google#1907
Copy link
Collaborator

@aditya-07 aditya-07 left a comment

Choose a reason for hiding this comment

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

Approving it based on @ellykits and @ekigamba 's findings.

@jingtang10 jingtang10 enabled auto-merge (squash) May 17, 2023 14:01
@jingtang10
Copy link
Collaborator

Hi @ellykits - can you fix this test: migrate1To2_should_not_throw_exception? This is what's stopping the code from being merged.

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request May 23, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
  - With unmerged PR google#1978
  - With unmerged PR google#1964
  - With unmerged PR google#1907
auto-merge was automatically disabled May 24, 2023 08:32

Head branch was pushed to by a user without write access

Copy link
Collaborator

@aditya-07 aditya-07 left a comment

Choose a reason for hiding this comment

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

Left a small comment, otherwise looks good.

Copy link
Collaborator

@aditya-07 aditya-07 left a comment

Choose a reason for hiding this comment

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

LGTM

@jingtang10 jingtang10 enabled auto-merge (squash) May 30, 2023 13:19
@jingtang10 jingtang10 merged commit f787d5b into google:master May 30, 2023
1 of 2 checks passed
@jingtang10 jingtang10 deleted the issue/1944-add-index-to-index_from-column branch May 30, 2023 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Engine: Add an API to allow adding database indexes
5 participants