-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 filter strategy for optimized entity name/property queries #6267
Conversation
e1e5fb1
to
4ef9c1c
Compare
37b4d9b
to
5e6fff8
Compare
cc00fc1
to
3e86933
Compare
import org.odk.collect.entities.storage.EntitiesRepository | ||
import org.odk.collect.entities.storage.Entity | ||
|
||
class LocalEntitiesInstanceAdapter(private val entitiesRepository: EntitiesRepository) { |
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'm thinking we'll eventually pull out an interface (InstanceDataAdapter
for example) from this to use for normal secondary instances as well. The entity InstanceProvider
and FilterStrategy
are both actually generic enough to support that with minimal changes and could be pulled out into another package, but that felt too far for the moment.
collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt
Show resolved
Hide resolved
collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt
Show resolved
Hide resolved
collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt
Show resolved
Hide resolved
...java/org/odk/collect/entities/javarosa/intance/LocalEntitiesExternalInstanceParserFactory.kt
Outdated
Show resolved
Hide resolved
collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/entities/JsonFileEntitiesRepository.kt
Outdated
Show resolved
Hide resolved
...es/src/test/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategyTest.kt
Outdated
Show resolved
Hide resolved
...es/src/test/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategyTest.kt
Show resolved
Hide resolved
entities/src/main/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategy.kt
Show resolved
Hide resolved
entities/src/main/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategy.kt
Show resolved
Hide resolved
This is the only node required to have children for the itemset to be verified.
a1ef660
to
d4d8250
Compare
@grzesiek2010 OK that's the missed comment fixed! |
Marking this as a draft while I fix problems with the local entities setting that are complicated by JavaRosa's static state. |
@grzesiek2010 ok that's fixed! |
Tested with Success! Verified on a device with Android 10 Verified Cases:
|
Tested with Success! Verified on a device with Android 14 |
Closes #5623
Blocked by #6261Blocked by getodk/javarosa#780Blocked by getodk/javarosa#782The "closes" here is slightly cheating as we'll also need to a better implementation of
EntitiesRepository
(using SQLite most likely) to get the real world load time gains. Additionally, the total memory footprint won't have dropped - the currentEntitiesRepository
implementation requires a whole list in memory for bothgetById
andgetByProperty
. That all said, we're assuming that our new implementation (which we'll build as part #6012) will give us faster lookups and memory use that's only increased by the number of results (as opposed to needing the whole list in memory ever). @lognaturel is putting together a project for testing all this, and we've decided that this implementation is a good enough structural change to serve as a foundation we can tweak to get the performance we want.Why is this the best possible solution? Were any other approaches considered?
The basic idea here is to query the
EntitiesRepository
forname=
and<property>=
predicates instead of letting JavaRosa use its own built inFilterStrategy
chain. Combined with "partial parsing" (provided byLocalEntitiesInstanceProvider
) and the newDataInstance#replacePartialElement
this lets us keep the majority of the entity list out of memory until its all needed (like when we show an unfiltered select).How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
This risk here will be to forms that use filtered selects and calculations based on secondary instances. Entity forms are the most likely to have problems, but there is some risk that there are some unintended consequences for non-entity forms as well.
There's no need to test the actual performance increase/decrease here. As I said in the introduction, that will be hard to properly verify until other changes are made.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest
(or./gradlew testLab
) and confirmed all checks still passDateFormatsTest