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

Make BQ annotations serializable #1773

Merged
merged 3 commits into from
Mar 26, 2019

Conversation

anish749
Copy link
Contributor

I was trying to write Scollections to BQ, and for some generic derivations where a nested type contained a class with a big query annotation, I was finding that the annotations are not serializable. I was thinking of making them seralizable because I think that is an easy fix and hopefully wont break anything.

My use case is something like this:

//.groupBy(r => r.userId) - usual Scio code.
.bqTrace("gcpproj.dataset.error_records", predicate = _.size > 1) // Trace errors to BQ.
//.flatMap(...) // usual scio code.

now my SCollection is of type [(String, Iterable[CaseClassWithBQAnnotation])]

@codecov
Copy link

codecov bot commented Mar 25, 2019

Codecov Report

Merging #1773 into master will decrease coverage by 2.49%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1773     +/-   ##
=========================================
- Coverage   71.67%   69.17%   -2.5%     
=========================================
  Files         176      176             
  Lines        5402     5402             
  Branches      410      388     -22     
=========================================
- Hits         3872     3737    -135     
- Misses       1530     1665    +135
Impacted Files Coverage Δ
.../spotify/scio/bigquery/client/BigQueryConfig.scala 0% <0%> (-70.59%) ⬇️
...scala/com/spotify/scio/bigquery/client/Cache.scala 0% <0%> (-66.67%) ⬇️
.../spotify/scio/bigquery/BigQueryPartitionUtil.scala 0% <0%> (-58.98%) ⬇️
...scala/com/spotify/scio/bigquery/BigQueryUtil.scala 50% <0%> (-50%) ⬇️
...la/com/spotify/scio/bigquery/client/QueryOps.scala 0.82% <0%> (-47.11%) ⬇️
...com/spotify/scio/bigquery/types/BigQueryType.scala 37.5% <0%> (-25%) ⬇️
...la/com/spotify/scio/bigquery/client/TableOps.scala 0% <0%> (-22.37%) ⬇️
...la/com/spotify/scio/bigquery/client/BigQuery.scala 27.02% <0%> (-10.82%) ⬇️
...n/scala/com/spotify/scio/bigquery/BigQueryIO.scala 24.29% <0%> (-3.74%) ⬇️
...com/spotify/scio/bigquery/types/TypeProvider.scala 76.59% <0%> (-2.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8181a11...c5f5f07. Read the comment docs.

@jto
Copy link
Contributor

jto commented Mar 25, 2019

Hi @anish749. Can you add a test to ensure that the 2 annotations do indeed serialize ?

Copy link
Contributor

@regadas regadas left a comment

Choose a reason for hiding this comment

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

@anish749 as @jto said a test here would be super 😄

@anish749
Copy link
Contributor Author

sure.. I will add them.. Thanks for pointing out.

@anish749 anish749 changed the title Make BQ annotations serialisable Make BQ annotations serializable Mar 25, 2019
// TODO: mock BigQueryClient for fromTable and fromQuery
class TypeProviderTest extends FlatSpec with Matchers {

val NOW = Instant.now()

@BigQueryType.fromSchema(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved these classes to the companion object because these annotated classes should be in a companion object (?)


case class User(@description("user name") name: String, @description("user age") age: Int)
case class Account(@description("account user") user: User,
@description("in USD") balance: Double)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved these to the companion object because the class SchemaProviderTest extends FlatSpec with Matchers wont be serializable because org.scalatest.FlatSpec isn't serializable. Not sure if this is the right way to do this. What are your thoughts.

I was getting the following exception if I had in in the class and not in the companion object:

[info]   Cause: java.io.NotSerializableException: org.scalatest.Assertions$AssertionsHelper
[info]  - field (class "org.scalatest.FlatSpec", name: "assertionsHelper", type: "class org.scalatest.Assertions$AssertionsHelper")
[info]  - object (class "com.spotify.scio.bigquery.types.TypeProviderTest", TypeProviderTest)
[info]  - field (class "com.spotify.scio.bigquery.types.TypeProviderTest$S1", name: "$outer", type: "class com.spotify.scio.bigquery.types.TypeProviderTest")
[info]  - root object (class "com.spotify.scio.bigquery.types.TypeProviderTest$S1", S1(1))
[info]   at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1182)

I enabled -Dsun.io.serialization.extendedDebugInfo=true to trace this.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

@anish749
Copy link
Contributor Author

@regadas @jto I added a test, but I am not very sure it is in the right class. Please let me know if this is otherwise, I would change it accordingly. I am not very familiar with this part of the code.


case class User(@description("user name") name: String, @description("user age") age: Int)
case class Account(@description("account user") user: User,
@description("in USD") balance: Double)
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

@regadas regadas merged commit b75aaa1 into spotify:master Mar 26, 2019
nevillelyh pushed a commit that referenced this pull request Mar 28, 2019
nevillelyh pushed a commit that referenced this pull request Mar 29, 2019
@anish749 anish749 deleted the serializable-annotations branch August 9, 2019 10:41
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