-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add Map ItemBatcher/ItemSelector #294
Conversation
43adee0
to
a521ed3
Compare
4ab7b13
to
494553e
Compare
209a1da
to
2cd4ab2
Compare
input.each_slice(max_items(context, state_input)).map do |batch| | ||
output.merge("Items" => batch) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
future: would be nice to not resolve all input at the start.
end | ||
|
||
def validate! | ||
if [max_items_per_batch, max_items_per_batch_path].all?(&:nil?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optimization (read: feel free to ignore)
Count these and compare to 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean like [max_items_per_batch, max_items_per_batch_path].compact.count.zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea
unless [max_items_per_batch, max_items_per_batch_path].compact.count == 1
parser_error!("must have one of \"MaxItemsPerBatch\", \"MaxItemsPerBatchPath\"")
end
lib/floe/workflow/item_batcher.rb
Outdated
invalid_field_error!("MaxItemsPerBatch", max_items_per_batch, "must be a positive integer") if max_items_per_batch && max_items_per_batch <= 0 | ||
invalid_field_error!("MaxInputBytesPerBatch", max_input_bytes_per_batch, "must be a positive integer") if max_input_bytes_per_batch && max_input_bytes_per_batch <= 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we care, but these will throw a ruby runtime error if these are not an integer. I really want to avoid that. Maybe do a to_i
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add a kind_of?(Integer)
check, I don't want to "blindly" to_i it because that would hide someone passing in a float which is kind of unexpected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I did not run.
I had put in a bunch of thoughts, but I do want some form of protection around invalid references to avoid ruby runtime errors. to_i
is just one suggestion for a fix.
lib/floe/workflow/item_batcher.rb
Outdated
return if max_items_per_batch_path.nil? | ||
|
||
result = max_items_per_batch_path.value(context, state_input) | ||
raise runtime_field_error!("MaxItemsPerBatchPath", result, "must be a positive integer") if result <= 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use a to_i
lib/floe/workflow/item_batcher.rb
Outdated
invalid_field_error!("MaxItemsPerBatch", max_items_per_batch, "must be a positive integer") if max_items_per_batch && max_items_per_batch <= 0 | ||
invalid_field_error!("MaxInputBytesPerBatch", max_input_bytes_per_batch, "must be a positive integer") if max_input_bytes_per_batch && max_input_bytes_per_batch <= 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to_i
for both of these (to avoid the runtime error)
spec/workflow/item_batcher_spec.rb
Outdated
it "raises an exception" do | ||
expect { subject.value(context, input, state_input) } | ||
.to raise_error(Floe::ExecutionError, "Map.ItemBatcher field \"MaxItemsPerBatchPath\" value \"0\" must be a positive integer") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we want a test with invalid values. (are we doing tests for these?)
e.g.:
let(:state_input) { {"batchSize" => "a", "items" => input} }
Added Integer type checks and tests for floats and strings |
2cd4ab2
to
eb89144
Compare
The ItemBatcher provides a way to group large array inputs into larger batches for better performance
You can provide a
MaxItemsPerBatch
orMaxItemsPerBatchPath
which limits the number of items in a batch, as well as aMaxInputBytesPerBatch
andMaxInputBytesPerBatchPath
which limits the size of the input in bytes.The output will be a set of
{"Items": []}
payloads, one for each batch.https://docs.aws.amazon.com/step-functions/latest/dg/input-output-itembatcher.html
#241