-
Notifications
You must be signed in to change notification settings - Fork 436
Enable always sorting when using pager #192
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
Enable always sorting when using pager #192
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
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