Skip to content

issue #262 modify SQLQueryBuilder to improve performance.#308

Merged
albertwang-ibm merged 10 commits intomasterfrom
Albert-Master-New
Oct 29, 2019
Merged

issue #262 modify SQLQueryBuilder to improve performance.#308
albertwang-ibm merged 10 commits intomasterfrom
Albert-Master-New

Conversation

@albertwang-ibm
Copy link
Copy Markdown
Contributor

@albertwang-ibm albertwang-ibm commented Oct 25, 2019

Changes made:
(1) Rewrite From/Where Clauses generators for the basic QuerySegmentAggregator;
(2) Simplify InclusionQuerySegmentAggregator for search _include query.

Test results:
Huge improvement of search performance, e.g,
(1) testSearchPatientWithBaseParametersAndExtensions of SearchExtensionsTest:
--Before the changes--
always timeout whenever there are about 50 patients created
--After the changes--
Even with more than 5000 patient created, this search test can still finish in just few seconds!
(2) testSearchObservationWithSubjectIncluded and testSearchObservationWithSubjectIncluded_filter_elements of SearchTest:
--Before the changes--
took more than 15 seconds if there were about 1000 patient created
always timeout whenever there are about 5000 patients created
--After the changes--
Even with more than 5000 patient created, this search test can still finish in just few seconds!

albertwang-ibm and others added 2 commits October 24, 2019 11:59
Signed-off-by: Albert Wang <xuwang@us.ibm.com>
@JohnTimm
Copy link
Copy Markdown
Collaborator

I see that you added buildWhereClause2. Do we need both buildWhereClause and buildWhereClause2? Can we remove one of them?

@lmsurpre
Copy link
Copy Markdown
Member

@albertwang-ibm do you have examples of the old sql vs the new sql? or the comments in this class still up-to-date for the queries being generated?

@albertwang-ibm
Copy link
Copy Markdown
Contributor Author

@JohnTimm I had tried to use one unified optimized version for both, but unfortunately didn't work well even though I had spent a whole afternoon trying to make it work. the system one doesn't perform well for the extension search etc, and the other one failed about 10 test cases of the system search. and yes, I think I can spend one more day to try to create a workable unified version for both.

@lmsurpre yes, after we agree with the PR, I will update the comments with the new generated sql.

@lmsurpre
Copy link
Copy Markdown
Member

While reviewing this PR and our related functionality, I found #314 and I opened a competing pull request to fix that. I'm pretty sure there will be conflicts, so please review my pull request before proceeding with this one.
I think its especially important for you to pick up the tests from my pull request to verify this one doesn't break anything.

Signed-off-by: Albert Wang <xuwang@us.ibm.com>
@albertwang-ibm
Copy link
Copy Markdown
Contributor Author

@JohnTimm Done! successfully use unified from/where clausebuilder codes now!

albertwang-ibm and others added 2 commits October 27, 2019 20:34
Signed-off-by: Albert Wang <xuwang@us.ibm.com>
@albertwang-ibm
Copy link
Copy Markdown
Contributor Author

Changes:
(1) Fixed _include search with much better performance.
(2) Removed the sample sqls in comments which have caused confusings which were not updated together with the codes for a while. Actually, it's pretty easy to get the sql either through debugging or in db logs.

Signed-off-by: Albert Wang <xuwang@us.ibm.com>
Signed-off-by: Albert Wang <xuwang@us.ibm.com>
Signed-off-by: Albert Wang <xuwang@us.ibm.com>
@lmsurpre lmsurpre self-requested a review October 29, 2019 12:11
Signed-off-by: Albert Wang <xuwang@us.ibm.com>
Copy link
Copy Markdown
Member

@lmsurpre lmsurpre left a comment

Choose a reason for hiding this comment

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

I still don't fully understand why the SQL query plan engines aren't smart enough to execute the subquery just once, but this LGTM pending the one last javadoc consistency change in my last 2 comments.

Signed-off-by: Albert Wang <xuwang@us.ibm.com>
Copy link
Copy Markdown
Collaborator

@JohnTimm JohnTimm left a comment

Choose a reason for hiding this comment

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

LGTM. FHIR when ready. :)

@albertwang-ibm albertwang-ibm merged commit b8b3851 into master Oct 29, 2019
@albertwang-ibm albertwang-ibm deleted the Albert-Master-New branch October 29, 2019 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants