Skip to content
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

Improve support for BigInt, enhance union type #166

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

opti
Copy link
Contributor

@opti opti commented Nov 5, 2024

  • Add ranges for int and bigint according to specs.
    This fix allows handling situations where int and long are present and covers issues like:
    Avro::IO::AvroTypeError: The datum 3324284763061284583 is not an example of schema "int"
  • Enhance union type selection during the serialization. Previously, we only checked the value class (now additionally for the union types index picking check if the value is coercible with a particular type)

@opti opti requested review from jturkel and a team as code owners November 5, 2024 04:39
@opti
Copy link
Contributor Author

opti commented Nov 5, 2024

Not sure why Linter is complaining. There is no changes in lib/avromatic/model/raw_serialization.rb:19:9 in this PR.

Inspecting 94 files
.......................C......................................................................

Offenses:

lib/avromatic/model/raw_serialization.rb:19:9: C: Style/AccessModifierDeclarations: private should not be inlined in method definitions.
        private :datum_writer, :datum_reader
        ^^^^^^^

94 files inspected, 1 offense detected

Exited with code exit status 1

@erikkessler1
Copy link

Not sure why Linter is complaining. There is no changes in lib/avromatic/model/raw_serialization.rb:19:9 in this PR.

I'd guess a new version of RuboCop got used that changed how it it applies that rule. You could add # rubocop:disable Style/AccessModifierDeclarations to that line since it seems like a false positive.

Copy link

@erikkessler1 erikkessler1 left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! It certainly seems like a gap.

A question I have is if we can remove the #matched? in favor of special casing integer handling in UnionType#find_index since we really only need matched? for integers.

@@ -76,9 +76,8 @@ def referenced_model_classes
private

def find_index(value)
# TODO: Cache this?

Choose a reason for hiding this comment

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

This can be a hot code path, so it would be nice to be able to cache this index lookup (i.e., build a cache from value.class to union_index). With this change, since we match on the value itself, that cache isn't possible.

Additionally, the matched? call adds overhead for all types, but we only need it for the case where value.is_a?(Integer). What do you think about special casing integer handling here?

def find_index(value)
  if value.is_a?(::Integer)
    if Avromatic::Model::Types::IntegerType.in_range?(value)
      member_types.index(Avromatic::Model::Types::IntegerType)
    end || if Avromatic::Model::Types::BigIntType.in_range?(value)
      member_types.index(Avromatic::Model::Types::BigIntType)
    end
  else
    # TODO: Cache this
    member_types.find_index do |member_type|
      member_type.value_classes.any? { |value_class| value.is_a?(value_class) }
    end
  end
end

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