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

Queryset pagination order and verbosity #153

Merged
merged 9 commits into from
Aug 4, 2019

Conversation

DannyAziz
Copy link
Contributor

Added .order_by('id') as was getting inconsistent results after rebuilding the index as shown in #151

Verbosity isn't important but I liked the feature in 'django-haystack'

@DannyAziz DannyAziz mentioned this pull request Jan 28, 2019
@DannyAziz
Copy link
Contributor Author

@crazyscientist Are there any other changes that I should make? I would love to see this merged

@crazyscientist
Copy link

@DannyAziz To me the PR looks good now, but I have no permission to merge/accept.

@sabricot What do you think?

@ntcong
Copy link

ntcong commented Jul 31, 2019

@safwanrahman can you take a look at this? order_by is really important when you have a big dataset. Adding order_by can be a good immediate step before a better approach, like #102

@safwanrahman
Copy link
Collaborator

If the pk is not incremental integer, will this work? I have doubt that in the case the primary key is not integer, like uuid, it will work actually!

@safwanrahman
Copy link
Collaborator

@DannyAziz Can you rebase it upon master so its possible to merge?

@safwanrahman
Copy link
Collaborator

Thanks @ntcong for mentioning me here

@DannyAziz
Copy link
Contributor Author

DannyAziz commented Jul 31, 2019

@safwanrahman I will rebase soon 👍

If you look at these 2 lines in my fork in this file: django_elasticsearch_dsl/documents.py

primary_key_field_name = self._doc_type.model._meta.pk.name
return self._doc_type.model._default_manager.all().order_by(primary_key_field_name)

It gets the name of the primary key for that model instead of assuming you are using PK, I am using UUID's in my project and it works fine

@DannyAziz
Copy link
Contributor Author

@safwanrahman I've rebased the fork to master would love to see this merged 👍

@DannyAziz
Copy link
Contributor Author

My bad this needs some fixing, hold on...

@safwanrahman
Copy link
Collaborator

This looks good. r+ from my end

Copy link
Collaborator

@safwanrahman safwanrahman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work. @DannyAziz

@safwanrahman safwanrahman merged commit 51db109 into django-es:master Aug 4, 2019
adarve added a commit to adarve/django-elasticsearch-dsl that referenced this pull request Dec 29, 2019
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.

4 participants