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

Introduce schema comparison #82

Merged
merged 12 commits into from
May 13, 2019
Merged

Conversation

vil1
Copy link
Contributor

@vil1 vil1 commented Apr 23, 2019

This PR introduces a mechanism for comparing two schemas.

Schemas are compared asymmetrically: one is considered the "writer" and the other the "reader". The comparison builds a result that can be either:

  • a Match with a (possibly empty) list of compatible transformations from writer to reader
  • a Mismatch with also a list of compatibility violations

A compatible transformation is a difference between writer and reader such that message written using writer remain compatible with reader.

In a first iteration, I've decided to remain as close as possible to avro's spec in the set of accepted transformations. More might be implemented later.

Copy link
Member

@pepegar pepegar left a comment

Choose a reason for hiding this comment

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

Added some comments. They're mostly related to naming. Apart of them, this looks awesome, thank you very much @vil1 !


sealed trait Transformation[T]

object Transformation {
Copy link
Member

Choose a reason for hiding this comment

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

I would name all cases of this ADT using nouns:

sealed trait Transformation[T]

object Transformation {
final case class NumericWiddening[T](relativePath: Path, from: T, to: T) extends Transformation[T]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
final case class NumericWiddening[T](relativePath: Path, from: T, to: T) extends Transformation[T]
final case class NumericWidening[T](relativePath: Path, from: T, to: T) extends Transformation[T]

final case class MadeOptional[T](relativePath: Path) extends Transformation[T]
final case class PromotedToEither[T](relativePath: Path, either: T) extends Transformation[T]
final case class PromotedToCoproduct[T](relativePath: Path, coproduct: T) extends Transformation[T]
final case class CoproductWiddening[T](relativePath: Path, coproduct: T) extends Transformation[T]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
final case class CoproductWiddening[T](relativePath: Path, coproduct: T) extends Transformation[T]
final case class CoproductWidening[T](relativePath: Path, coproduct: T) extends Transformation[T]

final case class Addition[T](relativePath: Path, added: T) extends Transformation[T]
final case class Removal[T](relativePath: Path, removed: T) extends Transformation[T]
final case class MadeOptional[T](relativePath: Path) extends Transformation[T]
final case class PromotedToEither[T](relativePath: Path, either: T) extends Transformation[T]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
final case class PromotedToEither[T](relativePath: Path, either: T) extends Transformation[T]
final case class EitherPromotion[T](relativePath: Path, either: T) extends Transformation[T]

final case class Removal[T](relativePath: Path, removed: T) extends Transformation[T]
final case class MadeOptional[T](relativePath: Path) extends Transformation[T]
final case class PromotedToEither[T](relativePath: Path, either: T) extends Transformation[T]
final case class PromotedToCoproduct[T](relativePath: Path, coproduct: T) extends Transformation[T]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
final case class PromotedToCoproduct[T](relativePath: Path, coproduct: T) extends Transformation[T]
final case class CoproductPromotion[T](relativePath: Path, coproduct: T) extends Transformation[T]

import cats.Show
import cats.syntax.show._

sealed trait Transformation[T]

Choose a reason for hiding this comment

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

Given that all the Transformation[T] classes have a relativePath field, I wonder if we can split them in two classes:

final case class At[T](relativePath: Path, transformation: Transformation[T])
sealed trait Transformation[T]
  final case class NumericWidening[T](from: T, to: T)    extends Transformation[T]
  final case class StringConversion[T](rfrom: T, to: T)   extends Transformation[T]
  final case class Addition[T](added: T)                 extends Transformation[T]
  final case class Removal[T](removed: T)                extends Transformation[T]
  final case class PromotionToOption[T]()                  extends Transformation[T]
  final case class PromotionToEither[T](either: T)       extends Transformation[T]
  final case class PromotionToCoproduct[T](coproduct: T) extends Transformation[T]
  final case class CoproductWidening[T](coproduct: T)    extends Transformation[T]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be better now

case (TCoproduct(i), TCoproduct(i2)) =>
AlignUnionMembers(
i.zipWithIndex
.map {

Choose a reason for hiding this comment

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

In order to trim down the complexity for this case (essentially MxN cases), perhaps we could add a quick "canFit" predicate, one which would approximate the full "is compatible" case. Approximate, in this case, would mean that if canFit(p._1, p._2) is false, that implies that p._1 cannot be compatible with p._2. The benefit of course would be that canFit would only look at the top level (not recursive).

Copy link
Contributor Author

@vil1 vil1 May 10, 2019

Choose a reason for hiding this comment

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

I think this must indeed be addressed, but should be in another PR.

Beside the approach you suggest, we should be able to avoid a whole bunch of unnecessary comparison by making the traversal monadic (hyloM) or using another scheme (maybe apo but I'm not completely sure about that).

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

4 participants