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

Add support for encoding and decoding Decimals #93

Merged
merged 4 commits into from
Mar 14, 2024

Conversation

jbruggem
Copy link
Contributor

This PR proposes to add support for Decimal values in avro_ex.

As decimal values aren't accurately represented by native number in Elixir, it proposes to (optionally) decode these decimals losslessly as Decimal structs.

In order to ensure consistency with the official implementation, this PR proposes to test the decoding/encoding using a reference Avro encoded file generated with a Java implementation.

@jbruggem jbruggem requested a review from a team as a code owner January 29, 2024 15:03
is_number(value) ->
value / :math.pow(10, -scale)

match?(%Decimal{}, value) ->
Copy link
Member

Choose a reason for hiding this comment

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

how does this work with optional dependencies? Will this compile if the user does not have :decimal installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it doesn't. Thanks !

I have fixed it by trying to follow the same approach followed in Jason.

I've tested it in an empty project, it seems to work.

@@ -316,6 +316,34 @@ defmodule AvroEx.Decode.Test do

assert Time.truncate(time, :millisecond) == now
end

test "decimal" do
schema = "test/fixtures/decimal.avsc" |> File.read!() |> AvroEx.decode_schema!()
Copy link
Member

Choose a reason for hiding this comment

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

it would be useful to test if round tripping the value through the encoder results in the same result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. There are now some round-trip tests in the encode_test. Do you think it is sufficient our should I add some here as well ?

if :math.pow(2, bits) > abs(value) do
bits
else
value_size(value, bits + 8)
Copy link
Member

Choose a reason for hiding this comment

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

are you sure that this will terminate for all input values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum, I am not. Should we stop when we reach 64 bytes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought: I don't actually see a situation where this won't terminate. The value of bits can only by positive multiples of 8, so we're always bound to eventually get a 2^bits that's bigger than any arbitrary value.

WDYT ?

@jbruggem
Copy link
Contributor Author

jbruggem commented Mar 8, 2024

@davydog187 is there anything I can do to help with this ?

@davydog187
Copy link
Member

@jbruggem looks good to me! Can you please fix dialyzer and the tests? Then we can get this released

@jbruggem
Copy link
Contributor Author

@davydog187 thanks ! The compilation error is fixed.

@davydog187 davydog187 merged commit b2319a7 into beam-community:master Mar 14, 2024
2 checks passed
@davydog187
Copy link
Member

Thank you!

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.

2 participants