bmlt-enabled / yap

Find meetings or someone to talk to over the phone leveraging a BMLT root server.
https://bmlt.app/yap
MIT License
9 stars 10 forks source link

Pagination well runs dry (shows empty results) #52

Closed dgershman closed 6 years ago

dgershman commented 6 years ago

@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?

jbraswell commented 6 years ago

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 commented 6 years ago

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

jbraswell commented 6 years ago

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 commented 6 years ago

Handle at postgres. LIMIT and OFFSET.

dgershman commented 6 years ago

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

jbraswell commented 6 years ago

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 commented 6 years ago

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

jbraswell commented 6 years ago

https://github.com/jbraswell/tomato/pull/24

jbraswell commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Do not suggest 0 indexing page item. Would start at 1.

On Mar 29, 2018, at 3:11 AM, Chris Marshall notifications@github.com wrote:

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.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/radius314/yap/issues/52#issuecomment-377189384, or mute the thread https://github.com/notifications/unsubscribe-auth/ABw9u-I4-m3Xq06h2cqmXY1YMGmMhTFcks5tjLNYgaJpZM4S6MiH.

LittleGreenViper commented 6 years ago

I can do that.

I'll use Jonathan's query params, and make the page selection 1-based.

Chris Marshall

Principal chris@littlegreenviper.com mailto:chris@littlegreenviper.com https://littlegreenviper.com https://littlegreenviper.com/ Little Green Viper Software Development LLC

On Mar 29, 2018, at 11:11 AM, Danny Gershman notifications@github.com wrote:

Do not suggest 0 indexing page item. Would start at 1.

On Mar 29, 2018, at 3:11 AM, Chris Marshall notifications@github.com wrote:

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.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/radius314/yap/issues/52#issuecomment-377189384, or mute the thread https://github.com/notifications/unsubscribe-auth/ABw9u-I4-m3Xq06h2cqmXY1YMGmMhTFcks5tjLNYgaJpZM4S6MiH.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/radius314/yap/issues/52#issuecomment-377267806, or mute the thread https://github.com/notifications/unsubscribe-auth/Ab9HLrtuO2CjiHPMft1WHE_Ag7CeXGT-ks5tjPm8gaJpZM4S6MiH.

LittleGreenViper commented 6 years ago

Giz it a spin: https://tkddevel.com/bmltwork/bmlt/main_server/client_interface/json/?switcher=GetSearchResults&page_size=50

https://tkddevel.com/bmltwork/bmlt/main_server/client_interface/json/?switcher=GetSearchResults&page_size=50&page_num=3