-
Notifications
You must be signed in to change notification settings - Fork 22
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
CE-534: Add support to handle producing union type of multiple records #222
base: master
Are you sure you want to change the base?
Conversation
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.
Just one question - otherwise looks good!
_is_integer_string?(val) || | ||
int_classes.any? { |klass| val.is_a?(klass) } | ||
when :float, :double | ||
val.is_a?(Numeric) || _is_float_string?(val) |
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.
Shouldn't this be Float
instead of Numeric
?
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 just tested with Float and it still works so I changed it to Float. Added a test for it as well. I copied it from this method for reference https://github.com/flipp-oss/deimos/blob/master/lib/deimos/schema_backends/avro_schema_coercer.rb#L56C7-L58C19
When determining data types from a union of different data types, compare the fields for schema type: record with the values to determine the right schema type.
Check for Float instead of Numeric type when looking for schema within a Union Schema
Description
Added support to detect the right schema type from a UnionSchema
Fixes https://flippit.atlassian.net/browse/CIE-534
Type of change
Bug fix (non-breaking change which fixes an issue)
How Has This Been Tested?
Tested the change on Item-Platform using this branch (to reproduce the issue) https://github.com/wishabi/item-platform/commit/560cdd748b9335dc9dee9aeeaa65e94f23cb1988
Verified that the existing Kafka specs pass on Item-Platform
Checklist: