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

TryExecuteQuery #837

Merged
merged 14 commits into from
Oct 24, 2019
Merged

TryExecuteQuery #837

merged 14 commits into from
Oct 24, 2019

Conversation

bchong95
Copy link
Contributor

TryExecuteQuery

Description

This PR adds support for a TryExecuteQuery method which will try to execute a query with a set of supported query features. If the query is supported than the method will return a QueryIterator. If the query is not supported than the method returns a QueryPlan instead.

The purpose of this method is to support SQL Query Compute Gateway. We are going to initially only support queries that don't require any distribution work and over time allow more queries. If the query is not supported than we will send back a bad request with the query plan as usual and allow the SDK to do the cross partition work. The method will also always return a QueryPlan even if the query is executable, since we might want to cache the query plan in the future.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

@bchong95 bchong95 self-assigned this Sep 25, 2019
@sboshra
Copy link
Contributor

sboshra commented Oct 7, 2019

    public async Task<PartitionedQueryExecutionInfo> GetQueryPlanAsync(

please delete if not used anymore


Refers to: Microsoft.Azure.Cosmos/src/Query/Core/QueryPlanHandler.cs:30 in 602869d. [](commit_id = 602869d, deletion_comment = False)

sboshra
sboshra previously approved these changes Oct 7, 2019
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:

Copy link
Member

@kirankumarkolli kirankumarkolli left a comment

Choose a reason for hiding this comment

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

Not an SDK concern and needs to be part of friends.

@kirankumarkolli
Copy link
Member

@bchong95 won;t it get solved by query wire protocol versioning and configuring SDK to use target version?

Copy link
Member

@kirankumarkolli kirankumarkolli left a comment

Choose a reason for hiding this comment

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

@bchong95 wont it get solved by query wire protocol versioning and configuring SDK to use target version?

@bchong95
Copy link
Contributor Author

There isn't any versioning or configuring here

@bchong95
Copy link
Contributor Author

This is merely exposing a TryExecute variant of CreateDocumentQuery. They are calling into the same version of code.

@kirankumarkolli
Copy link
Member

Is 'TryExecuteQuery' purely an optimization to avoid NotSupported exception?

@bchong95
Copy link
Contributor Author

It also allows the caller to determine what queries they want to allow.

@kirankumarkolli
Copy link
Member

@bchong95 please refresh the description also with current state of PR.

@kirankumarkolli kirankumarkolli merged commit a10e411 into master Oct 24, 2019
@kirankumarkolli kirankumarkolli deleted the users/brchon/TryExecuteQuery branch October 24, 2019 03:39
j82w pushed a commit that referenced this pull request Oct 29, 2019
* allowed for retrieval of query plan without exception

* wired query plan through the stack

* wired a try execute function

* added test

* resolved iteration comments

* removed contracts

* added GROUP BY support

* resolved iteration comments

* added try execute query

* added back methods that require private members
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.

4 participants