Skip to content

Add logicalType/decimal support to read bytes#20

Merged
tPl0ch merged 1 commit intoflix-tech:masterfrom
ziffmedia:feature-logical-type-decimal
Jan 26, 2024
Merged

Add logicalType/decimal support to read bytes#20
tPl0ch merged 1 commit intoflix-tech:masterfrom
ziffmedia:feature-logical-type-decimal

Conversation

@ralphschindler
Copy link

@ralphschindler ralphschindler commented Jul 14, 2023

This PR adds support for (at least) reading streams topics that have a schema that includes an Avro byte type with logicalType = "decimal" to be able to decode it into a decimal (instead of returning the bytes in the resulting structure.

Outstanding questions:

  • I would like to add a test, do I do that here?
  • Do I need to add the same functionality to write_bytes()?

Reference for this implementation is here: https://avro.apache.org/docs/1.10.2/spec.html#Decimal

Thanks for your library and assistance!

@ralphschindler
Copy link
Author

Hi @tPl0ch just pinging if there is any insight you have on this. Thanks again for maintaining this!

@ralphschindler ralphschindler marked this pull request as ready for review September 1, 2023 13:26
@stefankleff
Copy link

stefankleff commented Jan 15, 2024

Thank you @ralphschindler for implementing this feature. We had the same need and will use your PR.
@tPl0ch It would be great if this could be merged.

@tPl0ch
Copy link
Collaborator

tPl0ch commented Jan 26, 2024

@stefankleff , could you test this on the dev-master branch after the merge before I release a new version? I currently don't have the necessary confidence in the testing / CI of this project?

@tPl0ch tPl0ch merged commit 85a7841 into flix-tech:master Jan 26, 2024
@stefankleff
Copy link

@tPl0ch Thanks for merging and your overall effort!
We've forked the project and using it with the change for more two weeks now, and we don't have any issues.

@tPl0ch
Copy link
Collaborator

tPl0ch commented Feb 27, 2024

@stefankleff @ralphschindler released as https://github.com/flix-tech/avro-php/releases/tag/5.0.1

@ralphschindler
Copy link
Author

Apologies for being silent, I am unsure why I didn't get notifications of communications on updates to this issue. In any case, thank you for merging! I'll be glad to assist in testing future PRs, I would just need a little direction. Thanks again!

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