-
Notifications
You must be signed in to change notification settings - Fork 305
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
Fix Date/DateTime indexes to speed up Left joins on Search #2047
Conversation
- With unmerged PR #1 - With unmerged PR google#1917 - With unmerged PR google#1978 - With unmerged PR google#1907 - With unmerged PR google#2032 - With unmerged PR google#1669 - With unmerged PR google#2047
engine/src/main/java/com/google/android/fhir/db/impl/ResourceDatabase.kt
Outdated
Show resolved
Hide resolved
@@ -70,7 +70,22 @@ val MIGRATION_2_3 = | |||
object : Migration(/* startVersion = */ 2, /* endVersion = */ 3) { | |||
override fun migrate(database: SupportSQLiteDatabase) { | |||
database.execSQL( | |||
"CREATE INDEX IF NOT EXISTS `index_DateTimeIndexEntity_index_from` ON `DateTimeIndexEntity` (`index_from`)" | |||
"CREATE INDEX IF NOT EXISTS `index_DateTimeIndexEntity_resourceType_index_name_resourceUuid_index_from_index_to` ON `DateTimeIndexEntity` (`resourceType`, `index_name`, `resourceUuid`, `index_from`, `index_to`)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this needs to change.
the way migration works is that, if you're on db version n, the migration code would be run to migrate your db schema from n to n+1.
this means you should NOT modify existing migration code - people already on v3 would not get your new db index!
you should create a new version, and write another migration step to handle 3 to 4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aditya-07 fyi since aditya added migration code to begin with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jingtang10 DB version in the latest released engine library is 2. DB version 3 was added for a new change and hasn't been released via the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure - but as far as i know @khyativyasargus, @nsabale7, @dubdabasoduba fork our libs... I know theoretically we're not making promises in between releases, but i don't really see the downside of creating another migration step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted to how I have it but for some reason GitHub hated me doing that on this PR and the change is not getting picked up here.
Created a new PR: #2063 that has the migration to a new version
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #2040
Description
Clear and concise code change description.
In the Search API, we perform a LEFT JOIN when sorting:
this join uses the resourceType, resourceUuid and index_name columns. To speed this up, we should create a composite index made up of these 3 columns
TESTED:
Downloaded 7000 households. Opened App Inspector and ran query generated. Returned results almost instantaneously as opposed to many seconds
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 | Feature | Documentation | Testing | Code health | Builds | Releases | Other)
Checklist
./gradlew spotlessApply
and./gradlew spotlessCheck
to check my code follows the style guide of this project../gradlew check
and./gradlew connectedCheck
to test my changes locally.