Update SqlQueryGenerator to support client-side paging #3536

almostchristian · 2023-09-23T13:35:27Z

Description

Updates SqlQueryGenerator to support client-side paging by generating queries that are paged using OFFSET. If the configuration SupportsIndexedPaging is set to false, the old query generation will be used. If the ct parameter is numeric, indexed paging will be used regardless of the SupportsIndexedPaging configuration.

Related issues

Addresses #2454.

Testing

Added tests in ChainingSearchTests

FHIR Team Checklist

Update the title of the PR to be succinct and less than 65 characters
Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
Tag the PR with Schema Version backward compatible or Schema Version backward incompatible or Schema Version unchanged if this adds or updates Sql script which is/is not backward compatible with the code.
CI is green before merge
Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

… pagination.

almostchristian · 2023-09-23T14:06:28Z

@microsoft-github-policy-service agree

LTA-Thinking · 2023-10-17T16:16:18Z

/azp run

LTA-Thinking

Looks good on first pass, still running some tests on the behavior. If we include this it will need to default to off as it otherwise would be a breaking change.
Something I need to still test is how this handles updates compared with the old CTs.

LTA-Thinking · 2023-10-17T14:55:36Z

src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs

@@ -118,7 +119,18 @@ public SearchOptions Create(string resourceType, IReadOnlyList<Tuple<string, str
                            string.Format(Core.Resources.MultipleQueryParametersNotAllowed, KnownQueryParameterNames.ContinuationToken));
                    }

-                    continuationToken = ContinuationTokenConverter.Decode(query.Item2);
+                    // check is continuation token is a numerical format which means it must use an offset
+                    if (int.TryParse(query.Item2, out _))


This check is being done both here and in the continuation token converter. Making a note to see if it is needed in both locations.

...lth.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs

LTA-Thinking · 2023-10-17T16:35:34Z

src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/SortRewriter.cs

@@ -95,7 +108,8 @@ public override Expression VisitSqlRoot(SqlRootExpression expression, SqlSearchO
                    // for resources that have a value for the _sort parameter. So we will generate
                    // the appropriate Sort expression below.
                }
-                else if (continuationToken != null && continuationToken.SortValue == null)
+                else if ((continuationToken != null && continuationToken.SortValue == null) ||
+                    (continuationToken == null && context.Sort[0].sortOrder == SortOrder.Ascending))
                {
                    // We have a ct for resourceid but not for the sort value.


Could you update the comment to fit the new logic?

I removed this change

LTA-Thinking · 2023-10-17T16:46:31Z

...lth.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs

@@ -773,7 +792,7 @@ void AppendOrderBy()

            StringBuilder.Append("SELECT DISTINCT ");

-            if (includeExpression.Reversed)
+            if (includeExpression.Reversed && context.UseIndexedPaging)


Why is this case only needed when using indexed paging? This is used to prevent too many results from being returned from a revinclude.

I reverted this change. This was an old change i did when I was messing around.

LTA-Thinking · 2023-10-17T17:59:29Z

A difference I found in behavior between the two CTs:

User A runs a search, gets a CT
User B executes an action that adds a resource that would have been on the first page of User A's results
User A runs a search with their CT
With the current CTs User A's sees the next resource they were looking for. With index based CTs User A's first result is the last results from the first page of results, because User B's action pushed it to now be on the second page of results.

This could cause issues for services with heavy usage as the user could get "more" results then they are expecting, or get duplicates if they aren't looking for this issue.

LTA-Thinking · 2023-10-18T14:58:14Z

@almostchristian do you have performance numbers comparing runtimes of your change to our default method?

almostchristian · 2023-10-19T07:25:36Z

A difference I found in behavior between the two CTs:
* User A runs a search, gets a CT

* User B executes an action that adds a resource that would have been on the first page of User A's results

* User A runs a search with their CT

* With the current CTs User A's sees the next resource they were looking for. With index based CTs User A's first result is the last results from the first page of results, because User B's action pushed it to now be on the second page of results.
This could cause issues for services with heavy usage as the user could get "more" results then they are expecting, or get duplicates if they aren't looking for this issue.

Thanks for testing, I think this is an expected behavior with this indexed based paging. Indexed based list is suitable for UIs that have page indexes instead of an infinite scroll. Our UIs uses the traditional table with page numbers, and indexed based paging allows us to generate the links without having to walk through the entire list which would be required in the current ct.

…SortRewriter

almostchristian · 2023-10-20T02:58:42Z

@almostchristian do you have performance numbers comparing runtimes of your change to our default method?

I don't have the comparison of the performance of the generated sql, but I was able to measure the e2e performance below.

Test setup: From a fresh db, create patient with same data 1000 times, then search by patient.identifier.
I use nbomber and run with 100 threads for 2 minutes with a 5 second warmup.

I get the first page and second page, with _count of 2.
Numbers below are the average of 3 runs.

paging mode	page1 mean	page1 p95	page1 p99	page2 mean	page2 p95	page2 p99
default	33.88	59.35	68.02	41.47	53.02	59.69
indexed	32.41	58.68	68.69	39.59	53.02	60.36

almostchristian added 4 commits September 23, 2023 18:11

Implement paging in SQL Server via offset to enable client-controlled…

49ef9a3

… pagination.

Merge branch 'microsoft:main' into main

43fdc35

Remove unused constant

c2120d9

Merge branch 'main' of https://github.com/almostchristian/fhir-server

82c33ec

almostchristian mentioned this pull request Sep 23, 2023

Support _skip parameter for pagination #2454

Open

almostchristian marked this pull request as ready for review September 25, 2023 15:25

almostchristian requested a review from a team as a code owner September 25, 2023 15:25

LTA-Thinking added this to the S126 milestone Oct 17, 2023

LTA-Thinking added Enhancement Enhancement on existing functionality. Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs labels Oct 17, 2023

LTA-Thinking reviewed Oct 17, 2023

View reviewed changes

Fixed issues in SqlQueryGenerator and rollback unnecessary change in …

3ee3f57

…SortRewriter

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update SqlQueryGenerator to support client-side paging #3536

Update SqlQueryGenerator to support client-side paging #3536

almostchristian commented Sep 23, 2023

almostchristian commented Sep 23, 2023

LTA-Thinking commented Oct 17, 2023

LTA-Thinking left a comment

LTA-Thinking Oct 17, 2023

LTA-Thinking Oct 17, 2023

almostchristian Oct 20, 2023

LTA-Thinking Oct 17, 2023

almostchristian Oct 19, 2023

LTA-Thinking commented Oct 17, 2023

LTA-Thinking commented Oct 18, 2023

almostchristian commented Oct 19, 2023

almostchristian commented Oct 20, 2023

Update SqlQueryGenerator to support client-side paging #3536

Are you sure you want to change the base?

Update SqlQueryGenerator to support client-side paging #3536

Conversation

almostchristian commented Sep 23, 2023

Description

Related issues

Testing

FHIR Team Checklist

Semver Change (docs)

almostchristian commented Sep 23, 2023

LTA-Thinking commented Oct 17, 2023

LTA-Thinking left a comment

Choose a reason for hiding this comment

LTA-Thinking Oct 17, 2023

Choose a reason for hiding this comment

LTA-Thinking Oct 17, 2023

Choose a reason for hiding this comment

almostchristian Oct 20, 2023

Choose a reason for hiding this comment

LTA-Thinking Oct 17, 2023

Choose a reason for hiding this comment

almostchristian Oct 19, 2023

Choose a reason for hiding this comment

LTA-Thinking commented Oct 17, 2023

LTA-Thinking commented Oct 18, 2023

almostchristian commented Oct 19, 2023

almostchristian commented Oct 20, 2023