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.

Sign up for GitHub

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

Jump to bottom

Offset limit query support #234

Merged
merged 3 commits into from
Jul 24, 2019
Merged

Offset limit query support #234

merged 3 commits into from
Jul 24, 2019
+409 −5

Conversation

mbhaskar
Copy link
Member

@mbhaskar mbhaskar commented Jul 22, 2019 •
edited
Loading

Adding support to offset limit queries
This also supports offset-limit queries with continuation token

mbhaskar added 2 commits July 22, 2019 13:56
@mbhaskar
Adding support for offset limit queries
109e210 
Support for offset limit with continuation token
@mbhaskar
Cleanup
90813f6
@mbhaskar mbhaskar changed the title Mbhaskar/offset limit query support Offset limit query support Jul 22, 2019
@mbhaskar mbhaskar requested a review from bchong95 July 22, 2019 21:36
@moderakh moderakh self-requested a review July 23, 2019 14:57
moderakh
moderakh approved these changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

Please add some samples in the examples project as later the customers will ask you for samples.

Is there any special support needed for partition split? If so if it is possible it is worth adding some tests covering partition split. (See DocumentProducerTest)

@mbhaskar
Copy link
Member Author

Please add some samples in the examples project as later the customers will ask you for samples.

Is there any special support needed for partition split? If so if it is possible it is worth adding some tests covering partition split. (See DocumentProducerTest)

At the moment we have tests for this. Will add samples in seperate PR. Splits should work in the same way as any other query

christopheranderson
christopheranderson approved these changes Jul 24, 2019
View reviewed changes
Copy link
Contributor

@christopheranderson christopheranderson left a comment

Choose a reason for hiding this comment

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

Approved with suggestions. My nits aren't worth waiting for tests again, so you can treat them as advice instead.

Review checklist:

  • Dependencies - there are no dependencies added or modified
  • Public API surface area - there are no changes to the public API surface area
  • Functionality - there are changes to the public functionality. This PR impacts query functionality. The SDK now supports SKIP/TAKE with continuation tokens.
  • Test coverage - this PR includes sufficient test coverage for the new functionality
  • Fundamentals:
  • Perf - this should have no impact on perf
  • Availability - this should have no impact on availability
  • Consistency - this should have no impact on consistency
  • Security - this should have no impact on security
  • Supportability - this will impact supportability because we added functionality. We should be sure to have samples that cover the new use cases. @mbhaskar - please create a tracking item to add this coverage.

sdk/src/test/java/com/microsoft/azure/cosmosdb/rx/OffsetLimitQueryTests.java
FeedOptions options = new FeedOptions();
options.setMaxItemCount(pageSize);
options.setEnableCrossPartitionQuery(true);
options.setMaxDegreeOfParallelism(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: when you're not using the default values for something that's obviously under test, please either use descriptive variable names or leave comments next to literal usage so explain why you chose that value. In this case, why are we using cross partition and why are we using parallelism of 2? In theory, should we be testing a matrix of these values in e2e tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just followed this pattern from tests for other query types. I will clean this up along with the example

@mbhaskar
Copy link
Member Author

Approved with suggestions. My nits aren't worth waiting for tests again, so you can treat them as advice instead.

Review checklist:

  • Dependencies - there are no dependencies added or modified
  • Public API surface area - there are no changes to the public API surface area
  • Functionality - there are changes to the public functionality. This PR impacts query functionality. The SDK now supports SKIP/TAKE with continuation tokens.
  • Test coverage - this PR includes sufficient test coverage for the new functionality
  • Fundamentals:
  • Perf - this should have no impact on perf
  • Availability - this should have no impact on availability
  • Consistency - this should have no impact on consistency
  • Security - this should have no impact on security
  • Supportability - this will impact supportability because we added functionality. We should be sure to have samples that cover the new use cases. @mbhaskar - please create a tracking item to add this coverage.

#235

@christopheranderson christopheranderson merged commit 65606c7 into master Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christopheranderson christopheranderson christopheranderson approved these changes

@bchong95 bchong95 bchong95 approved these changes

@moderakh moderakh moderakh approved these changes

@kushagraThapar kushagraThapar Awaiting requested review from kushagraThapar kushagraThapar is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mbhaskar @christopheranderson @bchong95 @moderakh