Enable always sorting when using pager #192
Enable always sorting when using pager #192graysonarts merged 3 commits intotableau:developmentfrom graysonarts:always_sort_with_pager
Conversation
t8y8
left a comment
There was a problem hiding this comment.
Other than the small nit, the code looks solid.
I think it will miss the case where someone passes in RequestOptions to get a bigger pageSize but doesn't realize they need a sort order too.
tableauserverclient/server/pager.py
Outdated
| self._options = RequestOptions() | ||
|
|
||
| # Pager assumes deterministic order but solr doesn't guarantee sort order unless specified | ||
| if len(self._options.sort) == 0: |
There was a problem hiding this comment.
Can just be if not self._options.sort -- any non empty set will be True and any empty set will be False
|
@t8y8 Not sure what you mean? It shouldn't miss the case where someone passes in the RequestsOptions because the check for sort = empty set is checked regardless of whether they pass in the requests options or not |
t8y8
left a comment
There was a problem hiding this comment.
🚀
You're right, I read the 'if' as nested because my coffee cup isn't empty yet.
|
We talked about this offline. The client library does not feel like the correct place to fix this. Let's talk more but I think we might want to revert this. |
|
This is a temporary fix until the server is fixed so that the Pager is usable by default. Once we release the fix, we can remove this. |
|
The change itself looks fine but if we put in a default sort order for all queries it might not always be on name for every noun. Then Pager would have a weird behavior |
|
I don't follow what you mean here. Once the default sort is in, we'll delete this change. Would you rather not have this temporary workaround in the meantime? |
|
Nope. That sounds good although it might be a slight change in behavior for people |
|
Sorry ... I didn't see your comment after my first comment and before my second comment. :) |
* Enble always sorting when using pager because queries are not currently deterministic * I forgot to format the files * Fixing tyler's nit
* Enble always sorting when using pager because queries are not currently deterministic * I forgot to format the files * Fixing tyler's nit
because queries are not currently deterministic