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

To.safe #3767

Draft
wants to merge 56 commits into
base: main
Choose a base branch
from
Draft

To.safe #3767

wants to merge 56 commits into from

Conversation

vincenzobaz
Copy link

@vincenzobaz vincenzobaz commented Apr 19, 2021

This PR proposes a solution to the To.safe blocker in Scala 3 on which @MaximeKjaer, @liufengyun and I worked.
It took us some time to get familiar with the API and we tested different strategies before landing on this code, but we think it shows the strengths of Scala 3 macros and reflection by combining the two APIs in a relatively concise and idiomatic function that does not need eval

You can see a smaller diff here

The critical section is the file scio-core/src/main/scala-3/com/spotify/scio/schemas/To.scala where we use Scala 3 macros to pattern match on the type argument T of Expr[Schema[T]] to derive a Schema[T] at compile time which can then be used to invoke To.checkCompatibility. To deal with case class and compile time derived Schemas we rely on the reflection API which allows us to access the names and types of the fields of the case class.
You can see this the implementation here

This draft pr brings along many changes, in particular to build and syntax, that we adopted to use RC2, sbt 1.5, new syntax and to reduce the number of warnings.

We would like to have your opinion on this approach.

A few steps might be needed to finalize this pr:

  • Polish and refine
  • Extensive testing
  • Strengthen case class detection and safety: I am not sure that non-empty caseFields and <: Product is a strong-enough condition
  • Rebase on main as it progresses.

else Some(ls.collect { case Some(x) => x })

private def interpret[T: scala.quoted.Type](using Quotes): Option[Schema[T]] =
Type.of[T] match {
Copy link
Contributor

@jto jto Apr 20, 2021

Choose a reason for hiding this comment

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

(copying my comment here since I originally made it on your fork)

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 of Schema 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 in Schema but probably something we can live with. We don't really expect users to define their own instances of Schema.

I'd be curious to see how the implementation looks like for Java beans and Avro's SpecificRecord.

jto and others added 28 commits May 5, 2021 15:59
* Temporarily silence other jobs

* Refactor scala3 version into a variable

* Create ci job testing and compiling each subproject

* Disable fail-fast

* Use RC1 syntax for givens

* Fix isJavaBean

* Fix shadowing of BitSetCoder by private class

* Remove call to showAnsiColored which does not exist anymore

* Fix ambiguous resolution of implicit

* Disable subprojects that don't build

* Disable scio-core

Co-authored-by: vincenzobaz <[email protected]>
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