flipp-oss / deimos

Framework to work with Kafka, Avro and ActiveRecord
Other
59 stars 22 forks source link

Fix coercion of timestamp-millis and -micros #98

Closed jmccance closed 3 years ago

jmccance commented 3 years ago

Pull Request Template

Description

Adds logic to AvroSchemaCoercer to check the logical types of fields before coercing them. For timestamp-millis and timestamp-micros logical types, do not coerce them at all and let Avro's LogicalTypes modules handle it.

Fixes #97

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

Added unit tests in avro_based_shared.rb to verify that the affected fields are not coerced.

Checklist:

jmccance commented 3 years ago

The fix itself is currently working as a monkey-patch in our application. However, I'm having issues getting the tests to work correctly. Opening as a draft in case others can share some light.

The issue seems to be that when full_schema gets loaded by Avro::Schema.real_parse, the fields' logical types get discarded. Avro::Schema::RecordSchema.make_field_objects seems to totally disregard them. It's unclear why this isn't an issue in our application. Perhaps schema gets loaded differently outside of this spec?

dorner commented 3 years ago

I'm loving the contributions! ❤️ I'll pull this and take a look today.

dorner commented 3 years ago

@jmccance Got it! The schema has to change in order to make use of logicalTypes. Currently the schema in the test looks like this:

        {
          'name' => 'timestamp-millis-field',
          'type' => 'long',
          'logicalType' => 'timestamp-millis'
        },

It needs to look like this:

        {
          'name' => 'timestamp-millis-field',
          'type' => {
            'type' => 'long',
            'logicalType' => 'timestamp-millis'
          }
        },
jmccance commented 3 years ago

D'oh! Amazing how tests work better with valid schemas. Thanks for spotting me!

I'll get the PR updated. Might be a few days because of the holiday here.

dorner commented 3 years ago

LGTM! Thanks!