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: Adds support for non streaming ORDER BY #4362

Merged

Conversation

neildsh
Copy link
Contributor

@neildsh neildsh commented Mar 20, 2024

Description

This change adds support for non streaming ORDER BY. Prior to this, the SDK assumed that the documents returned by the backend in response to ORDER BY queries were totally ordered across continuations. With the addition of non streaming ORDER BY to the backend, this is no longer true. The backend indicates this with a new field in the response body, _streaming that will be set to false. In this case, a new non streaming order by pipeline stage is created. This is a blocking pipeline stage similar to GROUP BY. This stage accumulates all backend responses before it yields any results. This stage is exercised only if _streaming is set to false in the backend response to an ORDER BY query.

This change also fixes a few bugs in the SDK code:

The ResponseLengthInBytes property of QueryPage was being set incorrectly in several pipeline stages and has been removed.

The IQueryPipelineStage interface had an awkward SetCancellationToken method that has been removed in favor of taking the CancellationToken as a parameter to the MoveNextAsync method on ITracingAsyncEnumerator instead.

The index utilization header was not being accumulated correctly in SkipEmptyPageQueryPipelineStage, and similar code in ClientAggregateQueryPipelineStage had an unnecessary allocation in a loop.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Closing issues

To automatically close an issue: closes #IssueNumber

@sboshra
Copy link
Contributor

sboshra commented Mar 20, 2024

    private sealed class StaticQueryPageParameters

I'd avoid 'Static' here #Resolved


Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:71 in dd5ae23. [](commit_id = dd5ae23475604edfca950939b757aa0ea313e4dd, deletion_comment = False)

@sboshra
Copy link
Contributor

sboshra commented Mar 20, 2024

        public int MaxConcurrency { get; }

new lines #Resolved


Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:50 in dd5ae23. [](commit_id = dd5ae23475604edfca950939b757aa0ea313e4dd, deletion_comment = False)

@sboshra
Copy link
Contributor

sboshra commented Mar 20, 2024

    private OrderByCrossPartitionQueryPipelineStage(InitializationParameters init)

parameters or something else #Resolved


Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:110 in dd5ae23. [](commit_id = dd5ae23475604edfca950939b757aa0ea313e4dd, deletion_comment = False)

@sboshra
Copy link
Contributor

sboshra commented Mar 20, 2024

                return true;

this.bufferedPages.Count > 0 #Resolved


Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:152 in dd5ae23. [](commit_id = dd5ae23475604edfca950939b757aa0ea313e4dd, deletion_comment = False)

@sboshra
Copy link
Contributor

sboshra commented Mar 20, 2024

Should this be added as a resource value? #ByDesign


Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:1885 in eba9d5a. [](commit_id = eba9d5a, deletion_comment = False)

@sboshra
Copy link
Contributor

sboshra commented Mar 20, 2024

        {

Consider only using the MultiLevel class #Pending


Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:2031 in 4271b86. [](commit_id = 4271b8691ad67e7eb7605b19a5ebd85efad439fc, deletion_comment = False)

sboshra
sboshra previously approved these changes Mar 22, 2024
Copy link
Contributor

@sboshra sboshra left a comment

Choose a reason for hiding this comment

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

:shipit:

@neildsh
Copy link
Contributor Author

neildsh commented Mar 27, 2024

All the other pipeline stages also use inline constants. This code will be invoked in loop, where we may not want to pay the performance cost of doing a resource lookup. I will keep this as is for now.


In reply to: 2010479803


Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CrossPartition/OrderBy/OrderByCrossPartitionQueryPipelineStage.cs:1885 in eba9d5a. [](commit_id = eba9d5a, deletion_comment = False)

@neildsh neildsh force-pushed the users/ndeshpan/nonStreamingOrderBy branch from dd5ae23 to 3065ed1 Compare March 28, 2024 07:12
Copy link
Contributor

@adityasa adityasa left a comment

Choose a reason for hiding this comment

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

:shipit:

@neildsh
Copy link
Contributor Author

neildsh commented Mar 30, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@neildsh
Copy link
Contributor Author

neildsh commented Apr 1, 2024

/azp automerge

Copy link

Command 'automerge' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Enables automation to merge PRs QUERY
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants