Skip to content

[BEAM-8354] Add RowJsonSerializer#9732

Merged
kennknowles merged 1 commit intoapache:masterfrom
TheNeuralBit:row-json-serializer
Oct 7, 2019
Merged

[BEAM-8354] Add RowJsonSerializer#9732
kennknowles merged 1 commit intoapache:masterfrom
TheNeuralBit:row-json-serializer

Conversation

@TheNeuralBit
Copy link
Member

Adds a RowJsonSerializer, and renames RowJsonDeserializerTest to RowJsonTest, as it now includes roundtrip tests from row to json and back.

R: @apilloud

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
--- Build Status
Build Status
Build Status
Build Status
--- --- Build Status
XLang --- --- --- Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status
Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

Copy link
Member

@kennknowles kennknowles left a comment

Choose a reason for hiding this comment

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

This looks good. There's a Java tradition (which we don't follow all the time) of putting all tests for Foo in FooTest. It might make sense to consolidate the deserializer and serializer into an aggregating class just to make this more "normal".

@kennknowles kennknowles merged commit ba0e404 into apache:master Oct 7, 2019
@TheNeuralBit
Copy link
Member Author

I like the idea of combining serializer and seserializer into an outer class. Would there be any concern with changing the API since RowJsonDeserializer has been public for a while? Or is it generally considered ok to change around classes in util?

@kennknowles
Copy link
Member

Ah, everything in the util package is not for users. (they may or may not be on board with this design decision)

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