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

Support order DESC for find_each, find_in_batches and in_batches #30590

Merged
merged 1 commit into from Mar 24, 2020
Merged

Support order DESC for find_each, find_in_batches and in_batches #30590

merged 1 commit into from Mar 24, 2020

Conversation

le0pard
Copy link
Contributor

@le0pard le0pard commented Sep 13, 2017

Summary

This changes provide ability for find_each, find_in_batches and in_batches work also in ORDER BY DESC way.

Use cases

Batch processing methods allow you to work with the records in batches, thereby greatly reducing memory consumption. But it’s not possible to set the order. That is automatically set to ascending on the primary key (“id ASC”) to make the batch ordering work.

But in some cases for methods you need launch some tasks for new records first of all (like update "new records" first). In this case you cannot use batch processing methods, because it is always works from old to new records (primary key is number and auto increment). But by changing condition from

SELECT ... WHERE id > N ORDER BY id ASC LIMIT 1000

to

SELECT ... WHERE id < N ORDER BY id DESC LIMIT 1000

You can have the same batch processing methods, but in this case its will work in reverse order, which is useful for cases "new records should update/be present first".

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@eugeneius
Copy link
Member

Could you describe a use case for this feature?

@le0pard
Copy link
Contributor Author

le0pard commented Sep 14, 2017

@eugeneius added "Use cases" in description.

@le0pard
Copy link
Contributor Author

le0pard commented Oct 29, 2017

Should I add something else to this change? Maybe just someone waiting from me something and I am not understanding this. Thanks.

@sj26
Copy link
Contributor

sj26 commented Jan 2, 2019

I needed this today. My use case:

I'm trying to batch process a very large table of records. I probably won't process the whole table, there are about 250m records and it's going at ~75/sec — that's going to take a month — but I would like process a good chunk of it, and prioritise recent data. find_each(reverse_order: true) would let me process the newest first until I am comfortable to discard the rest.

@rails-bot
Copy link

rails-bot bot commented Dec 18, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 18, 2019
@sj26
Copy link
Contributor

sj26 commented Dec 18, 2019

This pull request is still relevant. What does it need to get merged?

@rails-bot rails-bot bot removed the stale label Dec 18, 2019
@le0pard
Copy link
Contributor Author

le0pard commented Dec 18, 2019

@sj26 I am waiting several years for accept or reject reason. Right now it have conflicts. I can resolve them, but I don't sure, that after this resolution it will be waiting something or someone.

@rails-bot
Copy link

rails-bot bot commented Mar 17, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Mar 17, 2020
Copy link
Member

@jeremy jeremy left a comment

Choose a reason for hiding this comment

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

Nice feature @le0pard. Thank you for your patience on this one.

I know you switched from order: :desc to a boolean reverse_order: true flag, but that does not fit with the existing API.

Let's switch back to order:, with :asc default, and raise ArgumentError on non-:asc/desc values.

@rails-bot rails-bot bot removed the stale label Mar 18, 2020
@jeremy jeremy assigned jeremy and unassigned eileencodes Mar 18, 2020
@le0pard
Copy link
Contributor Author

le0pard commented Mar 18, 2020

@jeremy Ok, thanks. I can do this. Just give me little time - I need fix conflicts

@le0pard
Copy link
Contributor Author

le0pard commented Mar 19, 2020

@jeremy it is ready for review.

@le0pard le0pard requested a review from jeremy March 20, 2020 08:11
Copy link
Member

@jeremy jeremy left a comment

Choose a reason for hiding this comment

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

Great. Just need a changelog entry and we're ready to merge. Your PR description is great! Would start with that for the changelog.

@le0pard
Copy link
Contributor Author

le0pard commented Mar 20, 2020

@jeremy thanks, added CHANGELOG

activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
@le0pard le0pard requested a review from jeremy March 24, 2020 10:02
@jeremy jeremy added this to the 6.1.0 milestone Mar 24, 2020
@jeremy jeremy merged commit 4d0c335 into rails:master Mar 24, 2020
@le0pard
Copy link
Contributor Author

le0pard commented Mar 24, 2020

Big thanks @jeremy

@le0pard le0pard deleted the find_each_order branch March 24, 2020 19:20
yahonda pushed a commit that referenced this pull request Jun 12, 2022
Follow-up #30590.
In the feature introduced in #30590, the case of calling `in_batches` without a block was not supported.
Perhaps the reason it was not supported was a correction omission.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants