8000 Enable always sorting when using pager by graysonarts · Pull Request #192 · tableau/server-client-python · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 3 commits into from
May 12, 2017
Merged

Enable always sorting when using pager #192

merged 3 commits into from
May 12, 2017

Conversation

graysonarts
Copy link
Contributor

because queries are not currently deterministic

Copy link
Collaborator
@t8y8 t8y8 left a 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.

self._options = RequestOptions()

# Pager assumes deterministic order but solr doesn't guarantee sort order unless specified
if len(self._options.sort) == 0:
Copy link
Collaborator

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

@graysonarts
Copy link
Contributor Author

@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

Copy link
Collaborator
@t8y8 t8y8 left a 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.

@graysonarts graysonarts merged commit 47332fc into tableau:development May 12, 2017
@graysonarts graysonarts deleted the always_sort_with_pager branch May 12, 2017 18:11
@LGraber
Copy link
Contributor
LGraber commented May 13, 2017

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.

@graysonarts
Copy link
Contributor Author

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.

@LGraber
Copy link
Contributor
LGraber commented May 13, 2017

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

@graysonarts
Copy link
Contributor Author

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?

@LGraber
Copy link
Contributor
LGraber commented May 13, 2017

Nope. That sounds good although it might be a slight change in behavior for people

@LGraber
Copy link
Contributor
LGraber commented May 13, 2017

Sorry ... I didn't see your comment after my first comment and before my second comment. :)

t8y8 pushed a commit to t8y8/server-client-python that referenced this pull request Jun 28, 2017
* Enble always sorting when using pager because queries are not currently deterministic

* I forgot to format the files

* Fixing tyler's nit
t8y8 pushed a commit to t8y8/server-client-python that referenced this pull request Jun 28, 2017
* Enble always sorting when using pager because queries are not currently deterministic

* I forgot to format the files

* Fixing tyler's nit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0