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

Changed Avro type for Python int to long #83

Merged
merged 3 commits into from
Nov 12, 2020
Merged

Changed Avro type for Python int to long #83

merged 3 commits into from
Nov 12, 2020

Conversation

panasenco
Copy link
Contributor

@panasenco panasenco commented Nov 10, 2020

... and Avro type for Python float to double.

Reasoning:

  • "Integers have unlimited precision." [1]
  • "Floating point numbers are usually implemented using double in C;" [1] and "almost all platforms map Python floats to IEEE-754 “double precision”." [2]

@panasenco
Copy link
Contributor Author

Haven't actually ensured unit tests pass, brb...

@codecov-io
Copy link

Codecov Report

Merging #83 (81d1ff4) into master (2ee644f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #83   +/-   ##
=======================================
  Coverage   99.50%   99.50%           
=======================================
  Files           7        7           
  Lines         607      608    +1     
  Branches       93       93           
=======================================
+ Hits          604      605    +1     
  Misses          1        1           
  Partials        2        2           
Impacted Files Coverage Δ
dataclasses_avroschema/fields.py 99.45% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ee644f...81d1ff4. Read the comment docs.

@marcosschroh
Copy link
Owner

Hi @panasenco ,

Thanks for the PR. Make sense your changes. I am wondering whether this can break the current process of the users. I Think won't be a problem as avro considers:

avro type json type example
int,long integer 1.
float, double number 1.1

With this changes the resulting schema will replace int by long and float by double and I think everything should work. Any ideas?

@panasenco
Copy link
Contributor Author

Hey Marcos,

Honestly, the change will probably be breaking some existing schemas if people upgrade without reading the changelog.

However, I think that even those people will agree the change is necessary. I was testing the schema with an integer like 12345678950 and was getting an overflow when using Kafka Connect to write to a SQL table. When people work with Python they expect practically unlimited integers and double-precision floats, because that's what Python has.

I think the benefits far outweigh the risks on this one. :)

Aram

@panasenco
Copy link
Contributor Author

That said, I went ahead and added some notices in big letters for people checking out the GitHub page and the documentation so that they are unlikely to miss it before upgrading.

@marcosschroh
Copy link
Owner

marcosschroh commented Nov 12, 2020

Cool! Let's merge it! Thanks!

@marcosschroh marcosschroh merged commit de22f7d into marcosschroh:master Nov 12, 2020
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.

None yet

3 participants