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

Avroparquet: Lower bound higher kinded #2273

Merged
merged 19 commits into from Apr 29, 2020

Conversation

paualarco
Copy link
Contributor

@paualarco paualarco commented Apr 17, 2020

Hello!
There is no issue related with this PR.

While looking at how to implement protobuf parquet connector, I realised that the current implementation of avroparquet was statically typead as GenericRecord, which for the Akka user is not nice.

On continuation with that the tests were simplified and improved by not just checking the number of records written but also the actual content of the file.

@probot-autolabeler probot-autolabeler bot added dependency-change For PRs changing the version of a dependency. p:avroparquet labels Apr 17, 2020
@paualarco paualarco changed the title Higher kinded avroparquet Higher kinded Avroparquet Apr 17, 2020
@paualarco
Copy link
Contributor Author

paualarco commented Apr 20, 2020

I have noticed that it breaks the site creation with paradox, once I got some feedback I would fix that by creating a new test to support that.

@paualarco paualarco changed the title Higher kinded Avroparquet Lower higher kinded Avroparquet Apr 22, 2020
@paualarco paualarco changed the title Lower higher kinded Avroparquet Lower bound higher kinded Avroparquet Apr 22, 2020
@ennru ennru changed the title Lower bound higher kinded Avroparquet Avroparquet: Lower bound higher kinded Apr 24, 2020
@ennru
Copy link
Member

ennru commented Apr 24, 2020

Thank you for this PR. I rebased it (as we had some problem with tests not running), fixed Paradox and added the required Mima exclusions.

Please improve the docs and examples to show that you may use a more specific type than Generic Record now.

We plan to release Alpakka 2.0 next week, it would be great to get this in.

@paualarco
Copy link
Contributor Author

paualarco commented Apr 26, 2020

Thank you for this PR. I rebased it (as we had some problem with tests not running), fixed Paradox and added the required Mima exclusions.

Please improve the docs and examples to show that you may use a more specific type than Generic Record now.

We plan to release Alpakka 2.0 next week, it would be great to get this in.

Have pushed the changes, note that I have only added a scala example that will demonstrate the improvement, and I have explicitly mentioned that in the documentation.
Hope it will be enough for now :)

I think that for the time being this is a good improvement, but still could be even better to completely erase the type constrain on GenericRecord, in which therefore any implementation of ParquetWriter or ParquetReader could be used and not just the Avro one.

Sadly I have not the time to implement it at that moment, but I will in the future if no one does... that would also imply a wider test coverage as well as renaming the whole sub-module.

@ennru
Copy link
Member

ennru commented Apr 29, 2020

Thank you, I've pushed some updates as I'd like to see the PR for the release.

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM.

@ennru ennru added this to the 2.0.0 milestone Apr 29, 2020
@ennru ennru merged commit 158893c into akka:master Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change dependency-change For PRs changing the version of a dependency. documentation p:avroparquet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants