Skip to content

ResultSet: handle empty non-final pages on ResultSet iteration #1110

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

Merged
merged 1 commit into from
Sep 17, 2021
Merged

ResultSet: handle empty non-final pages on ResultSet iteration #1110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cassandra/cluster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5141,6 +5141,7 @@ def next(self):
if not self.response_future._continuous_paging_session:
self.fetch_next_page()
self._page_iter = iter(self._current_rows)
return self.next()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm just a bit worried that this complicates next() a bit more than is necessary. The intermingling of self.next() and the global next(iter) functions here aren't as clear as one might like. Since our main goal is to ensure that we get a non-empty page out of fetch_next_page() why not test that explicitly?

        if not self.response_future._continuous_paging_session:
            # Some servers may return empty pages (Scylla is known to do so
            # in at least some cases) so make sure we have something non-empty
            # before we return.
            while True:
                self.fetch_next_page()
                if self._current_rows:
                    break
            self._page_iter = iter(self._current_rows)

Your code is effectively doing the same thing, it's just doing it by recursively calling self.next() to do so. I'm wondering if we can avoid the recursion entirely (and be clearer about our intent) by just handling the empty page case here.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @absurdfarce

IMHO the right place to handle this is right where I've put it in the code logic because it makes it clearer that "in that case", we need to "replay the same logic of handling the iteration on pages".

As such, I feel the recursion is clean and less confusing than trying to be smart in a function which name has nothing to do with what needs to be done in the situation that we're covering here.

Once again, that's just my opinion of course and as you can see, that one line is all it takes ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apologies for the delay in getting back to you @ultrabug. I'm trying to juggle a number of things at once, ideally without dropping any of them on the floor. :)

I take your point about the minimal number of changes required to support your fix. I'm not 💯 sure I agree with doing it this way but I can certainly see the benefit of your approach. How about something of a compromise: perhaps you could add a simple comment in there noting that (a) empty pages are possible in some impls and (b) if the page we just fetched happens to be empty we'll do the right thing when we recurse? I think if that were there it would go a long way towards addressing my concern about clarity of the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ultrabug After thinking about this some more there's no reason to hold up accepting this PR for docs. What you have here is a good change and I certainly appreciate your work so far (including the aforementioned nice test!). I'll merge this in now and sort out the documentation question later.


return next(self._page_iter)

Expand Down
13 changes: 13 additions & 0 deletions tests/unit/test_resultset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,19 @@ def test_iter_paged(self):
type(response_future).has_more_pages = PropertyMock(side_effect=(True, True, False)) # after init to avoid side effects being consumed by init
self.assertListEqual(list(itr), expected)

def test_iter_paged_with_empty_pages(self):
expected = list(range(10))
response_future = Mock(has_more_pages=True, _continuous_paging_session=None)
response_future.result.side_effect = [
ResultSet(Mock(), []),
ResultSet(Mock(), [0, 1, 2, 3, 4]),
ResultSet(Mock(), []),
ResultSet(Mock(), [5, 6, 7, 8, 9]),
]
rs = ResultSet(response_future, [])
itr = iter(rs)
self.assertListEqual(list(itr), expected)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice test case... a very effective demonstration of the problem!

def test_list_non_paged(self):
# list access on RS for backwards-compatibility
expected = list(range(10))
Expand Down