-
Notifications
You must be signed in to change notification settings - Fork 288
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
Modify toQueryString to prevent SQLite expression tree from exceeding depth of 1000 #2565
base: master
Are you sure you want to change the base?
Conversation
ae8187f
to
6b3d4c4
Compare
engine/src/main/java/com/google/android/fhir/search/filter/FilterCriterion.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/search/filter/FilterCriterion.kt
Outdated
Show resolved
Hide resolved
80c50de
to
0264816
Compare
d9e6d03
to
d764115
Compare
judging purely by the error message, my hypothesis is that the expression tree depth is O(n) for nested OR operators because the expression tree is constructed naively by parsing the OR operators sequentially. For example, for this expression
if the expression tree is constructed naively you'd get:
where each But what you really want is actually this:
where the tree is more "balanced" and this has depth 4. In other words, this is O(log(n)). If my hypothesis of what causes the problem is correct above, instead of trying to break the OR statements into chunks (and having to come up with a value), all you actually have to do is keep the tree balanced by splitting the top level OR statment at the middle of the list of params. Does this make sense? |
Yeah, it makes sense. |
there's no guarantee what i said is true @LZRS - i've done no testing or verification and depending on sqlite's implementation what i said could be complete garbage... so pls test and see if this is true :) (for example you could try to see if you'll hit the depth limit a bit later to bifurcate the tree instead of chunking the parameters) |
Alright, no problem. I'll test it out and get back |
@jingtang10 I tested this out for 1000, 2000 and upto 5000 parameters, and it worked perfectly. I also went ahead a drafted an implementation in the PR |
af4d912
to
821feab
Compare
engine/src/main/java/com/google/android/fhir/search/filter/FilterCriterion.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/search/filter/FilterCriterion.kt
Outdated
Show resolved
Hide resolved
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.
Can we add a DatabaseImplTest with
FORK - With unmerged PR #9 - WUP #13 SDK - WUP google#2178 - WUP google#2650 - WUP google#2663 PERF - WUP google#2669 - WUP google#2565 - WUP google#2561 - WUP google#2535
engine/src/main/java/com/google/android/fhir/search/filter/FilterCriterion.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/search/filter/FilterCriterion.kt
Outdated
Show resolved
Hide resolved
FORK - With unmerged PR #9 - WUP #13 SDK - WUP google#2178 - WUP google#2650 - WUP google#2663 PERF - WUP google#2669 - WUP google#2565 - WUP google#2561 - WUP google#2535
@@ -5169,6 +5170,29 @@ class DatabaseImplTest { | |||
assertThat(localChangeResourceReferences.size).isEqualTo(locallyCreatedPatients.size) | |||
} | |||
|
|||
@Test | |||
fun searchTasksForManyPatientsReturnCorrectly() = runBlocking { | |||
val patients = (0..5001).map { Patient().apply { id = "task-patient-index-$it" } } |
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.
There are 5002 patients and 5001 tasks. Is this test case deliberately checking this? I feel like if you really want to test the filter clause, it's better to query tasks linked to patient 0-4999 so at least you're filtering out 1 patient to know that the filter clause works.
} | ||
database.insert(*tasks.toTypedArray()) | ||
|
||
val patientsSearchIdList = |
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.
you can also limit this list to 2000 or 3000.
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.
you can also limit this list to 2000 or 3000.
I couldn't set limit the test to either 3000 or 2000, ran to another sqlite error on android
android.database.sqlite.SQLiteException: too many SQL variables (code 1)
It's related to https://issuetracker.google.com/issues/73634057
For now, I just used a limit of 990
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.
but 5000 worked? this is odd...
can you please do some more testing to see what's goign on here?
this is not blocking the pr though.
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.
Initially the way they'd passed for the 5000, I had generated the bifurcated queries and ran them in separate sqlite db browser. That might be the difference, assuming the sqlite version for Android may be limited
engine/src/main/java/com/google/android/fhir/search/filter/FilterCriterion.kt
Outdated
Show resolved
Hide resolved
engine/src/test/java/com/google/android/fhir/impl/FhirEngineImplTest.kt
Outdated
Show resolved
Hide resolved
engine/src/test/java/com/google/android/fhir/impl/FhirEngineImplTest.kt
Outdated
Show resolved
Hide resolved
} | ||
database.insert(*tasks.toTypedArray()) | ||
|
||
val patientsSearchIdList = |
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.
but 5000 worked? this is odd...
can you please do some more testing to see what's goign on here?
this is not blocking the pr though.
} | ||
val searchQuery = | ||
Search(ResourceType.Task) | ||
.apply { filter(Task.SUBJECT, *patientsSearchIdList.toTypedArray()) } |
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.
this filter is checking that tasks are referencing patients 0-990 (inclusive) so that's 991 patients.
but you only have tasks for patients 0-989 (990 in total).
so the filter criteria is not really being tests because the filter criteria is broader than what you have in the database.
if you want to test it, make the filter more strict. for example, create N patients, and in the filter list N-1 IDs. and check that you only get N-1 results.
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #2561
Description
Recursively bifurcates the conditional params expressions to prevent occurences of SQLite expression tree exceeding depth of 1000, as suggested in this comment
Alternative(s) considered
Chunking large expression list to limit 50 within parantheses to avoid crashing with
Expression tree is too large (maximum depth 1000)
, as described hereType
Enhancement Feature
Screenshots (if applicable)
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.