Azure / azure-sdk-for-java

This repository is for active development of the Azure SDK for Java. For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/java/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-java.
MIT License
2.35k stars 1.99k forks source link

[BUG] CosmosPageImpl Doesn't Correctly Calculate hasNext() #32067

Closed trande4884 closed 5 days ago

trande4884 commented 2 years ago

Describe the bug The constructor for CosmosPageImpl only sets this.offset = pagealbe.getOffset(); however this does not work for any page other than the first page. If you are creating a page request object like CosmosPageRequest pageRequest = new CosmosPageRequest(1, 2, null); then no offset is being passed in, so the offset will get set to 0. Because of this function's like hasNext() and isLast() do not work correctly. The implementation likely needs to be something like this.offset = pageable.getOffset() + (pageable.getPageNumber() * pageable.getPageSize()); so that it can account for the already processed results. However, the offset may not exactly be the correct place to do this so an alternative would be to add a new variable to CosmosPageImpl that is something like processedRecords which can be set to pageable.getPageNumber() * pageable.getPageSize() which can then be added to the offset in the hasNext() calculation.

To Reproduce Steps to reproduce the behavior: Generate a query that returns 4 results and grab a CosmosPageRequest such as CosmosPageRequest pageRequest = new CosmosPageRequest(1, 2, null); which will get the second page of results. With the result try retrieving hasNext() which should be false but it will return true.

Expected behavior With the steps to reproduce above, hasNext() should return true.

If you suspect a dependency version mismatch (e.g. you see NoClassDefFoundError, NoSuchMethodError or similar), please check out Troubleshoot dependency version conflict article first. If it doesn't provide solution for the problem, please provide:

Additional context Add any other context about the problem here.

Information Checklist Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

kushagraThapar commented 8 months ago

@trande4884 is this still a valid issue? I believe we have fixed it. Please update it, thanks!

trande4884 commented 8 months ago

This is still an issue. You can see below that even though this page is correctly getting "Page 2 of 2", calling page1.hasNext() is returning true when it should be returning false. image

FabianMeiswinkel commented 8 months ago

I don't quite get it - if the container has many physical partitions getting the second record doesn't mean hasNext should be false - because you would need to drain the query to cover for all the other partitions, correct? And while we suppress empty pages (except for the last one) in this case an empty last page is not really avoidable - unless you always retrieve one page in advance (which will become a problem when customers actually use a continuation token). The semantic of hasNext is only that it must guarantee that the query is fully drained when hasNext returns false. hasNext == true doesn't mean that there will be more records for sure - just that the query hasn't been fully drained yet.

trande4884 commented 8 months ago

@FabianMeiswinkel to further clarify the issue isn't just that we could get 1 empty page if attempting to utilize hasNext(), you could get numerous empty pages like below. If you have a high page size hasNext() will potentially have you get many empty pages before actually returning false. Also in Spring, we get a count of all of the items across all partitions upfront, so we have the total count to cover all partitions from the time we get the first page and we should be able to accurately know when we have retrieved all the results without returning a bunch of empty pages.

image

github-actions[bot] commented 5 days ago

Hi @trande4884, we deeply appreciate your input into this project. Regrettably, this issue has remained unresolved for over 2 years and inactive for 30 days, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.