Skip to content

Support nested iteration #310

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 1 commit into from
Mar 29, 2023
Merged

Conversation

bdewater
Copy link
Contributor

@bdewater bdewater commented Dec 8, 2022

Alternate take on #295 - unlike the suggested fix in that PR, we retain the existing "start after" cursor logic this gem uses (see eg array and CSVEnumerator):

  • cursor: nil yields the first item
  • cursor: 0 yields the second item
  • cursor: 1 yields the third item
  • etcetera

The main difference in code:

     def each(&block)
       return to_enum unless block_given?
 
-      iterate([], [], 0, &block)
+      iterate([], 0, &block)
     end
 
     private
 
-    def iterate(current_items, current_cursor, index, &block)
+    def iterate(current_items, index, &block)
       cursor = @cursor[index]
       enum = @enums[index].call(*current_items, cursor)
 
       enum.each do |item, cursor_value|
         if index == @cursor.size - 1
-          yield item, current_cursor + [cursor_value]
+          yield item, @cursor
         else
-          iterate(current_items + [item], current_cursor + [cursor_value], index + 1, &block)
+          iterate(current_items + [item], index + 1, &block)
+          @cursor[index + 1] = nil
         end
+        @cursor[index] = cursor_value
       end
     end
   end

cc @fatkodima
closes #295

@bdewater bdewater force-pushed the nested-iteration-v3 branch 2 times, most recently from dc4d1b6 to 40ed3ef Compare December 8, 2022 06:11
@fatkodima
Copy link
Contributor

Thanks for working on this! I tested locally all the edge cases, seems like it is working correctly 👍.

I haven't grasped your suggestion initially, because with these changes current cursor != [parent_id, child_id] (so "need to do the mapping in the head") and it still can reprocess previous records (I initially wanted to avoid that).

@bdewater
Copy link
Contributor Author

@Mangara anything I can do to help get this over the finish line?

Copy link
Contributor

@Mangara Mangara left a comment

Choose a reason for hiding this comment

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

The code LGTM. Just one question and a test failure.

Thank you @fatkodima for starting this, and @bdewater for finishing it ❤️

@@ -189,7 +189,7 @@ def iterate_with_enumerator(enumerator, arguments)
# Deferred until 2.0.0
# assert_valid_cursor!(cursor_from_enumerator)

tags = instrumentation_tags.merge(cursor_position: cursor_from_enumerator)
tags = instrumentation_tags.merge(cursor_position: cursor_from_enumerator).deep_dup
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest I'm not entirely sure why this is happening, but the added test in test/unit/iteration_test.rb fails otherwise: the notification payload is 12 times {:job_class=>"JobIterationTest::JobWithNestedEnumerator", :cursor_position=>[2, nil]} instead of the expected increasing nested cursor 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to hang on to @cursors yielded from the nested enumerator.

irb(#<JobIterationTest:0x00000001066631c0>):003:0> events.map { |e| e[:cursor_position].object_id }
=> [243980, 243980, 243980, 243980, 243980, 243980, 243980, 243980, 243980, 243980, 243980, 243980]

cursor_from_enumerator.dup instead of the tags object works too, I'll push that up in a sec.

@bdewater bdewater force-pushed the nested-iteration-v3 branch from fea96ac to baf2cf0 Compare March 28, 2023 22:13
Co-authored-by: Bart de Water <[email protected]>
@bdewater bdewater force-pushed the nested-iteration-v3 branch from baf2cf0 to d6ba2c9 Compare March 28, 2023 22:25
@Mangara Mangara merged commit 158ca2b into Shopify:main Mar 29, 2023
@bdewater bdewater deleted the nested-iteration-v3 branch March 29, 2023 12:19
@lavoiesl lavoiesl mentioned this pull request Aug 15, 2023
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems August 23, 2023 19:41 Inactive
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