forked from spotify/scio
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Port To.safe macro to Scala 3 #10
Open
MaximeKjaer
wants to merge
31
commits into
scala3-main
Choose a base branch
from
tosafe
base: scala3-main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 23 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
84cb877
Add first implementation of macro interpreter
vincenzobaz a3186d6
Correct syntax
vincenzobaz e55b1a1
Continue To.safe macro implementation
MaximeKjaer ccf83a8
Update implicit parameter to Scala 3 syntax
MaximeKjaer 55a9393
Remove withDottyCompath from scio-macros. Add withDottyCompat to over…
vincenzobaz b40e626
Fix compilatin issue
vincenzobaz 7c4c7e4
Make Scala 3 To.safe macro compile
MaximeKjaer d90ed49
Make To.safe take implicit parameters in Scala 3
MaximeKjaer 3a9c506
Add To.safe test
MaximeKjaer 7d87a58
Relace expr-based schema interpreter with a reflection-based one
vincenzobaz bb323dc
Add simple tests
vincenzobaz 2556de8
Enable scio-core tests in ci
vincenzobaz 91b6f0d
Use scalatest it in To.safe tests
MaximeKjaer 78dfc9d
Add missing cases to To.safe
MaximeKjaer e227e8c
Add tests for To.safe conversions of Java types
MaximeKjaer fa5c956
Prototype a compile time schema generation for java beans
vincenzobaz 6658e8b
Simplify test
vincenzobaz afb37aa
Specify versions
vincenzobaz 4aa8e57
Drop .tree usage in javabean schema derivation
vincenzobaz 232995b
Drop usage of .tree in IsJavaBean macro
vincenzobaz 47044fe
Add JavaBeans
vincenzobaz d4b9f8c
Disable Scala 2 test
vincenzobaz 75c4d4e
Reorder cases in To.safe interpreter
MaximeKjaer c49e5e9
Update to Scala 3
vincenzobaz 93d192f
Fix IsJavaBean
vincenzobaz e082144
Update scalatest
vincenzobaz 997be91
Update sbt
vincenzobaz 26f5857
Disable scoverage plugin to compile in Scala 3
vincenzobaz 63f0dcc
Migrate ci to 3.0.0
vincenzobaz 1004f2d
Fix typo
vincenzobaz 1ec8f25
Use transparent to whitebox isjavabean
vincenzobaz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package com.spotify.scio; | ||
|
||
class JavaBeanA implements java.io.Serializable { | ||
private String firstName = null; | ||
private String lastName = null; | ||
private int age = 0; | ||
|
||
public JavaBeanA() { | ||
} | ||
public String getFirstName(){ | ||
return firstName; | ||
} | ||
public String getLastName(){ | ||
return lastName; | ||
} | ||
public int getAge(){ | ||
return age; | ||
} | ||
public void setFirstName(String firstName){ | ||
this.firstName = firstName; | ||
} | ||
public void setLastName(String lastName){ | ||
this.lastName = lastName; | ||
} | ||
public void setAge(int age){ | ||
this.age = age; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package com.spotify.scio; | ||
|
||
class JavaBeanB implements java.io.Serializable { | ||
private String name = null; | ||
private String uuid = null; | ||
private int money = 0; | ||
|
||
public JavaBeanB() { | ||
} | ||
public String getName(){ | ||
return name; | ||
} | ||
public String getUuid(){ | ||
return uuid; | ||
} | ||
public int getMoney(){ | ||
return money; | ||
} | ||
public void setName(String name){ | ||
this.name = name; | ||
} | ||
public void setUuid(String uuid){ | ||
this.uuid = uuid; | ||
} | ||
public void setMoney(int money){ | ||
this.money = money; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package com.spotify.scio; | ||
|
||
class JavaBeanC implements java.io.Serializable { | ||
private String firstName = null; | ||
private String lastName = null; | ||
private int age = 0; | ||
|
||
public JavaBeanC() { | ||
} | ||
public String getFirstName(){ | ||
return firstName; | ||
} | ||
public String getLastName(){ | ||
return lastName; | ||
} | ||
public int getAge(){ | ||
return age; | ||
} | ||
public void setFirstName(String firstName){ | ||
this.firstName = firstName; | ||
} | ||
public void setLastName(String lastName){ | ||
this.lastName = lastName; | ||
} | ||
public void setAge(int age){ | ||
this.age = age; | ||
} | ||
} |
64 changes: 64 additions & 0 deletions
64
scio-core/src/test/scala/com/spotify/scio/ToSafeSuite.scala
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
package com.spotify.scio | ||
|
||
import com.spotify.scio.schemas.To | ||
|
||
import org.scalatest.flatspec.AnyFlatSpec | ||
import org.scalatest.matchers.should.Matchers | ||
|
||
case class JavaListInt(l: java.util.List[java.lang.Integer]) | ||
case class JavaListString(l: java.util.List[java.lang.String]) | ||
case class ListInt(l: List[Int]) | ||
case class JavaSource(b: java.lang.Boolean) | ||
case class Source(b: Boolean) | ||
case class Dest(b: Boolean) | ||
case class Mistake(b: Int) | ||
case class Mistake2(c: Boolean) | ||
|
||
case class Sources(name: String, links: List[Array[Byte]]) | ||
case class Destinations(name: String, links: List[Array[Byte]]) | ||
case class DestinationsWrong(name: String, links: List[Array[Int]]) | ||
|
||
class ToSafeTest extends AnyFlatSpec with Matchers { | ||
"To.safe" should "generate a conversion on compatible flat case class schemas" in { | ||
To.safe[Source, Dest] | ||
} | ||
|
||
it should "generate a conversion between java.lang.Boolean and Boolean" in { | ||
To.safe[JavaSource, Source] | ||
To.safe[Source, JavaSource] | ||
} | ||
|
||
it should "generate a conversion between java.util.List[java.lang.Integer] and List[Int]" in { | ||
To.safe[JavaListInt, ListInt] | ||
To.safe[ListInt, JavaListInt] | ||
} | ||
|
||
it should "fail on incompatible Java types" in { | ||
"To.safe[JavaListString, JavaListInt]" shouldNot compile | ||
"To.safe[JavaListString, ListInt]" shouldNot compile | ||
} | ||
|
||
it should "fail on incompatible flat case class schemas" in { | ||
"To.safe[Source, Mistake2]" shouldNot compile | ||
"To.safe[Source, Mistake]" shouldNot compile | ||
} | ||
|
||
it should "generate a conversion on compatible nested case class schemas" in { | ||
To.safe[Sources, Destinations] | ||
} | ||
|
||
it should "fail on incompatible nested case class schemas" in { | ||
"To.safe[Sources, DestinationsWrong]" shouldNot compile | ||
} | ||
|
||
it should "work with java beans" in { | ||
"To.safe[JavaBeanA, JavaBeanB]" shouldNot compile | ||
"To.safe[JavaBeanB, JavaBeanA]" shouldNot compile | ||
|
||
"To.safe[JavaBeanB, JavaBeanC]" shouldNot compile | ||
"To.safe[JavaBeanC, JavaBeanB]" shouldNot compile | ||
|
||
To.safe[JavaBeanA, JavaBeanC] | ||
To.safe[JavaBeanC, JavaBeanA] | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm missing something this means that the possible set of supported schema types is effectively limited to those already defined in scio.
If a user adds a
given
instance ofSchema
for type that isn't supported (say for example a Java class), the derivation will simply ignore it. This could be a problem for aliaswa support inSchema
but probably something we can live with. We don't really expect users to define their own instances ofSchema
.I'd be curious to see how the implementation looks like for Java beans and Avro's
SpecificRecord
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the assumption here.
It's still possible to support those use cases by registering custom interpreters to be tried as a last resort. Indeed, it would complicate things and can be subtle: think the case where the user defines the schema and use it in the same project.
It's not clear what you mean above. Could you please clarify?
@vincenzobaz and @MaximeKjaer are going to work on it. I'll let them report progress on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey. Yeah sorry I wanted to talk about LogicalType actually but got the exact term used in beam wrong... I think that's the only case for which a user might want to define a custom type. Last time I checked the implementation was broken in beam and fixing it did not seem to be the priority so I guess we can live without it.
The most common use case for us would be to convert from avro's
SpecificRecord
to case classes and vice-versa so this one needs to be working really well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was checking out how we could handle java beans and I hit an important blocker I think.
It is really easy to work around the implicit instance of
IsJavaBean[T]
because an instance of such type it is just an empty evidence, moreover the code to instantiate it is already ported to macro. It is straightforward to add a case to the pattern match which could like this:I also remarked that we only need a
BSchema
for the compatibility check, so we might avoid altogether reaching for aRawRecord[T]
.The major pain point is that
JavaBeanSchema.schemaFor
which is used to create theBSchema
in question requires aClass[T]
(which explains the implicit requirement of aClassTag[T]
).Class[T]
is available only at runtime, so it is not possible to have instances of it in the macro.I am not sure how we can handle this problem besides maybe implementing our own schema derivation for java beans.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This blocker might affect
AvroInstances.avroSchema
as well given that it relies on an implicitClassTag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be the case but seems unlikely. I think r-eimplementing it is an acceptable solution. We can always have tests to validate the the Schema is the same as what beam would derive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC for
Avro
we actually only need to access the avro Schema. There's a schema field in the generated code so it should be possible to access it at compile time since it's just a String ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For avro I see two methods:
implicit def avroSchema[T <: SpecificRecord: ClassTag]: Schema[T]
which relies on theClassTag
to feed aClass
toTypeDescriptor
and therefore suffers from the same problem as the Java bean discussed here.def fromAvroSchema(schema: org.apache.avro.Schema): Schema[GenericRecord]
which is not generic nor implicit.I imagine that you refer to the second one. In this case we can pattern match on the call of
fromAvroSchema
but we have not compile time information aboutschema: avro.Schema
, so I am not sure about how to obtain theSchema[GenericRecord]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jto commit 93de09c proposes an implementation of compile time schema derivation for java beans similar to the one used for case classes. It seems to be accepted by (simple) tests.
Does it look reasonable to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Seems reasonable :)