Skip to content

ActiveRecordBatchEnumerator#each should rewind at the end#95

Merged
etiennebarrie merged 1 commit into
masterfrom
batch-enumerator-each
May 28, 2021
Merged

ActiveRecordBatchEnumerator#each should rewind at the end#95
etiennebarrie merged 1 commit into
masterfrom
batch-enumerator-each

Conversation

@etiennebarrie

@etiennebarrie etiennebarrie commented May 27, 2021

Copy link
Copy Markdown
Member

While looking at the code for #94, I found out that our each is a bit surprising since it remembers its state (because it keeps the cursor). Calling each again on a enumerator doesn't iterate again, it doesn't yield.

Calling each twice in a row should yield all the batches again, like an Array or any Enumerator does:

results = []
array = [1, 2]
array.each { |i| results << i }
array.each { |i| results << i }
results # => [1, 2, 1, 2]

I tested that by creating an enumerator with a cursor, and checking that we only get the remaining batches, twice. At first I reset the cursor entirely, but that would cause 4 batches first, then the full 5 batches on the second call to each, which was also an unexpected behavior. I don't really expect users to use this a lot but it's more correct at very little cost so worth it!

Calling each twice in a row should yield all the batches again, like:

	results = []
	array = [1, 2]
	array.each { |i| results << i }
	array.each { |i| results << i }
	results # => [1, 2, 1, 2]

@adrianna-chang-shopify adrianna-chang-shopify left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I had the feeling having an enum with state tracked inside @cursor was going to throw something off. 😆 Since we've bothered to make this a proper Ruby Enumerator it definitely makes sense to me to make this correct and rewind properly based on the cursor we first built the enumerator with. Nice catch! 🚀

@etiennebarrie etiennebarrie merged commit f8c69e2 into master May 28, 2021
@etiennebarrie etiennebarrie deleted the batch-enumerator-each branch May 28, 2021 13:35
@shopify-shipit shopify-shipit Bot temporarily deployed to rubygems May 28, 2021 13:37 Inactive
@pedropb pedropb mentioned this pull request May 28, 2021
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.

2 participants