Skip to content
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

Query: Fixes handling of pipeline execution on partition merge #2531

Merged
merged 10 commits into from
Jun 11, 2021

Conversation

ealsur
Copy link
Member

@ealsur ealsur commented Jun 10, 2021

Pull Request Template

Description

The CrossPartitionRangePageAsyncEnumerator when facing a merge, was adding the merge result as part of the list of Enumerators, instead of maintaining the current Range and draining it.

In this sample scenario, let's assume a 2 partition collection (partition A and B).

When the query pipeline starts, it generates a queue with 2 enumerators, one for A, one for B. Assume each partition has 10 documents.

Assume we fetch the first page which contains 10 documents from A, which returns a ContinuationToken like:

{token: null, range {B}}

Which normally would make the query continue with B and drain it.

If there is a merge and A and B merge into C (with C having all the 20 documents), the current logic adds a new enumerator for C in the queue copying the continuation from the parent (B), so the query would not continue on C (full range) with continuation null (all the documents).

The effect is that the query end ups returning 30 documents (10 from the first page, 20 from the second because it used C).

What is the fix

In the case of a merge where the result is a single parent range, instead of creating a new Queue Enumerator for the merge result, we re-enqueue the current Range Enumerator.

When this happens, the logic in the RequestInvokerHandler kicks in and applies the EPK filtering:

https://github.com/Azure/azure-cosmos-dotnet-v3/blob/master/Microsoft.Azure.Cosmos/src/Handler/RequestInvokerHandler.cs#L244-L263

There was a test currently trying to cover this scenario but it was failing to catch the bug due to a bug in the test itself and how the InMemoryContainer behaved on a merge.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@ealsur ealsur added bug Something isn't working QUERY labels Jun 10, 2021
@ealsur ealsur self-assigned this Jun 10, 2021
Copy link
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks

@ealsur ealsur merged commit c704377 into master Jun 11, 2021
@ealsur ealsur deleted the users/ealsur/querymerge branch June 11, 2021 18:23
ealsur added a commit that referenced this pull request Jun 11, 2021
* fix

* fixing inmemorycontainer

* fixing merge test

* undo small change

* OrderBy fix too

* Correct orderby handling

* more tests

* text

Co-authored-by: j82w <j82w@users.noreply.github.com>
kirankumarkolli pushed a commit that referenced this pull request Jun 14, 2021
* Query: Fixes InvalidOperationException on merge to a single partition (#2510)

* Add support to FlakyDocumentContainer send back 410s based on a delegate

* Fix bug in CrossPartitionRangePageAsyncEnumerator where we dont handle a merge to a single partition

* Fix InvalidOperationException on merge to single partition in OrderByCrossPartitionQueryPipelineStage

* Incorporate CR feedback

* more CR feedback

Co-authored-by: Samer Boshra <sboshra@microsoft.com>

* Query: Fixes handling of pipeline execution on partition merge (#2531)

* fix

* fixing inmemorycontainer

* fixing merge test

* undo small change

* OrderBy fix too

* Correct orderby handling

* more tests

* text

Co-authored-by: j82w <j82w@users.noreply.github.com>

* [Internal] Client Encryption : Adds test to verify that update of Client Encryption Policy is not allowed via ReplaceContainer (#2349)

Adds / updates existing tests to verify -
a. Update of Client Encryption Policy is not allowed via ReplaceContainer
b. CreateContainer request ensures that the ClientEncryptionKey exists when creating Client Encryption Policy

Co-authored-by: neildsh <35383880+neildsh@users.noreply.github.com>
Co-authored-by: Samer Boshra <sboshra@microsoft.com>
Co-authored-by: j82w <j82w@users.noreply.github.com>
Co-authored-by: anujtoshniwal <62551957+anujtoshniwal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QUERY
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants