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

Avro - toJson should work exactly as toBinary #256

Merged
merged 1 commit into from Sep 27, 2016
Merged

Avro - toJson should work exactly as toBinary #256

merged 1 commit into from Sep 27, 2016

Conversation

mariussoutier
Copy link
Contributor

@mariussoutier mariussoutier commented Sep 20, 2016

Adapt toJson to work similarly as toBinary in order to support generated Scala case classes as specific records.

val klass = classTag[T].runtimeClass.asInstanceOf[Class[T]]
val writer = new SpecificDatumWriter[T](klass)
val reader = new SpecificDatumReader[T](klass)
def toJson[T <: SpecificRecordBase: ClassTag]: Injection[T, String] = {
Copy link
Collaborator

@johnynek johnynek Sep 23, 2016

Choose a reason for hiding this comment

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

can we add a new method name and not break binary compatibility?

Copy link
Contributor Author

@mariussoutier mariussoutier Sep 24, 2016

Choose a reason for hiding this comment

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

Sure, it's what I suggested when we changed toBinary, but you guys decided it was unnecessary. I'll update the PR.

@johnynek
Copy link
Collaborator

johnynek commented Sep 24, 2016

We since added a binary compatibility check to CI and changed the process
for publishing when that check fails. Sorry for the hassle.
On Sat, Sep 24, 2016 at 03:19 Marius Soutier notifications@github.com
wrote:

@mariussoutier commented on this pull request.

In
bijection-avro/src/main/scala/com/twitter/bijection/avro/AvroCodecs.scala
#256:

@@ -121,10 +121,11 @@ object SpecificAvroCodecs {
* @tparam T compiled Avro record
* @return Injection
*/

  • def toJson[T <: SpecificRecordBase: ClassTag](schema: Schema): Injection[T, String] = {
  • val klass = classTag[T].runtimeClass.asInstanceOf[Class[T]]
  • val writer = new SpecificDatumWriterT
  • val reader = new SpecificDatumReaderT
  • def toJson[T <: SpecificRecordBase: ClassTag]: Injection[T, String] = {

Sure, it's what I suggested when we changed toBinary, but you guys
decided it was unnecessary. I'll update the PR.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#256, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEJdtgope7S0Gfqsvpdz3NigV8MSek4ks5qtPkigaJpZM4KCG5w
.

@mariussoutier
Copy link
Contributor Author

mariussoutier commented Sep 24, 2016

Makes sense. Is the updated PR not binary compatible?

Edit: Ah I think there's a formatting error, but I'm not sure what it is.

@johnynek
Copy link
Collaborator

johnynek commented Sep 24, 2016

Failed the scala format test. Can you run sbt scalafmt and commit the changes?

@codecov-io
Copy link

codecov-io commented Sep 24, 2016

Current coverage is 66.14% (diff: 100%)

Merging #256 into develop will increase coverage by 0.07%

@@            develop       #256   diff @@
==========================================
  Files            50         50          
  Lines          1394       1397     +3   
  Methods        1363       1347    -16   
  Messages          0          0          
  Branches         31         50    +19   
==========================================
+ Hits            921        924     +3   
  Misses          473        473          
  Partials          0          0          

Powered by Codecov. Last update b68dd06...d0ba2c3

@mariussoutier
Copy link
Contributor Author

mariussoutier commented Sep 24, 2016

Done!

Copy link
Collaborator

@johnynek johnynek left a comment

If we can, let's remove the ClassTag on the toBinary. If I'm missing something, let's add a comment and leave it.

Thanks for updating the PR!

}

/**
* Returns Injection capable of serializing and deserializing a generic avro record using org.apache.avro.io.JsonEncoder to a
* UTF-8 String
* @tparam T compiled Avro record
* @param schema The record's schema
* @return Injection
*/
def toJson[T <: SpecificRecordBase: ClassTag](schema: Schema): Injection[T, String] = {
Copy link
Collaborator

@johnynek johnynek Sep 26, 2016

Choose a reason for hiding this comment

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

I'd live to remove the ClassTag here if possible, but that would be a binary incompatible break, so we can leave it as a TODO for now.

Copy link
Contributor Author

@mariussoutier mariussoutier Sep 26, 2016

Choose a reason for hiding this comment

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

Ok I added a todo.

* @param schema The record's schema
* @return Injection
*/
def toBinary[T <: SpecificRecordBase: ClassTag](schema: Schema): Injection[T, Array[Byte]] = {
Copy link
Collaborator

@johnynek johnynek Sep 26, 2016

Choose a reason for hiding this comment

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

does this actually use the ClassTag? I don't see where. If not, since we already dropped it in a previous version, why not correct that issue?

Copy link
Contributor Author

@mariussoutier mariussoutier Sep 26, 2016

Choose a reason for hiding this comment

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

Good point, done!

Copy link
Collaborator

@johnynek johnynek left a comment

Thanks!

@mariussoutier
Copy link
Contributor Author

mariussoutier commented Sep 27, 2016

All checks successful, merge away ;)

@johnynek johnynek merged commit 66bbeb7 into twitter:develop Sep 27, 2016
1 check passed
@johnynek
Copy link
Collaborator

johnynek commented Sep 27, 2016

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.

None yet

3 participants