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

Pagination well runs dry (shows empty results) #52

Closed
dgershman opened this issue Mar 25, 2018 · 14 comments
Closed

Pagination well runs dry (shows empty results) #52

dgershman opened this issue Mar 25, 2018 · 14 comments
Labels

Comments

@dgershman
Copy link
Collaborator

dgershman commented Mar 25, 2018

@jbraswell discovered that if you paginate enough, eventually the well runs dry.

image uploaded from ios

This is because we are limiting the result set to the BMLT to the first 50 results. The BMLT root server does not support pagination, so we would have to continue increasing the result size. This would cause increasing load on the Tomato server potentially.

There are a couple of approaches I think could be taken:

  1. Bump up the results area incrementally on demand (by a small amount per additional query), basically infinite scroll. There could potentially be a case when every single meeting in the world would be returned by https://github.com/jbraswell/tomato.git.

  2. Show a message indicated you've reached the end of the result set and to run another query.

Any other alternatives?

@dgershman dgershman added the bug label Mar 25, 2018
@jbraswell
Copy link
Contributor

jbraswell commented Mar 26, 2018

I would opt for showing a message that indicates the end of the result set has been reached instead of showing those empty results.

As for pagination, I could add a secret pagination option to GetSearchResults on tomato. I think it would be trivial to add page_size and page_num parameters.

@dgershman
Copy link
Collaborator Author

dgershman commented Mar 26, 2018

If pagination were added, then an empty result set would almost never happen.

@jbraswell
Copy link
Contributor

I'll do a bit of research to see how easily I could add page_size and page_num params without messing up the lazy streaming of results to the client.

@dgershman
Copy link
Collaborator Author

Handle at postgres. LIMIT and OFFSET.

@dgershman
Copy link
Collaborator Author

I will still need to display end of result messages for when pointing directly to a root server (not-tomato)

@jbraswell
Copy link
Contributor

It is handled at postgres, but it has to be passed through the ORM to postgres.

The results from postgres are lazily evaluated, so that adds a bit of complexity. Evaluating the result set sooner than the client needs it results in memory bubbles.

Good point about non-tomato root servers.

@jbraswell
Copy link
Contributor

Okay, yeah. This looks easy. Adding limit/offset to a Django QuerySet no longer forces evaluation of that QuerySet.

@jbraswell
Copy link
Contributor

bmlt-enabled/tomato#24

@jbraswell
Copy link
Contributor

So, I merged that PR. It adds paging to tomato's GetSearchResults via page_size and page_num parameters. I realize that this won't work when using a BMLT Root Server as your data source, but if you wanted to make a special case for tomato, you could.

We could also implement those params in the root server. Seems like it might not be that difficult using LIMIT and OFFSET. I do believe that the root server sometimes filters results outside of the database (like in the SearchString params), so it wouldn't always be as easy as just adding LIMIT and OFFSET to sql queries.

@LittleGreenViper
Copy link

I can certainly add pagination parameters to the semantic interface. Giving it some thought, here's the kind of thing I envision:

Added to the query for &switcher=GetSearchResults and &switcher=GetChanges:

&page_size=XXX (The maximum number of results per page 1-based integer)
&page_index=X (The initial page -0-based integer)

I have also considered adding this:

&get_count_only=1

If that is specified, then only an integer (no JSON wrapper) would be returned, with the search count. It's likely to be fairly fast. It would also be mutually exclusive with &get_formats_only=1

I could add something like:

&get_page_count=1

If &page_size=XXX is specified. This would also return an integer only (no JSON wrapper).

For example, say you have a search that returns 103 meetings.

If the page size is specified to be 50, then this would result in 3 pages: 0-49, 50-99, 100-102.

@jbraswell
Copy link
Contributor

That’s pretty much what I just did in tomato, except the parameters are ‘page_size’ and ‘page_num’, and ‘page_num’ is 1-indexed, not 0-indexed. If you could use the same variable names as tomato that would be ideal - of course, I could also modify tomato to match your choice.

‘get_page_count’ is an interesting idea, and would be pretty easy to implement on the tomato side of things. I would say that it’s less important than just getting the basic paging params in place, though.

@dgershman
Copy link
Collaborator Author

dgershman commented Mar 29, 2018 via email

@LittleGreenViper
Copy link

LittleGreenViper commented Mar 29, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants