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

Baleen <-> Avro generators #6

Merged
merged 10 commits into from
Jun 20, 2018

Conversation

kdallmeyer-sr
Copy link
Contributor

@kdallmeyer-sr kdallmeyer-sr commented Jun 18, 2018

Provide a way to generate base Baleen descriptions from legacy Avro schemas. Additionally add the reverse generater from Baleen to Avro.

TODO

  • Additional Unit tests for new Baleen types created
  • Missing Unit tests in AvroEncoder and BaleenEncoder
  • Resolve some questions around "style" and naming choices
  • General cleanup

WONTDO (Out of scope for now)

  • Missing some Avro types like "fixed"
  • Missing Avro logical types like date, timestamp-micros, etc

In order to add validation to existing Avro schemas, use this utility to create Baleen descriptions from your legacy Avro schemas
Convert your Baleen data descriptions into Avro schemas to use in data serialization. This updates the Work-in-progress Avro schema generator
Copy link
Contributor

@HawaiianSpork HawaiianSpork left a comment

Choose a reason for hiding this comment

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

Looks good. Just had a minor comment.

value == null -> sequenceOf(ValidationError(dataTrace, "is null", value))
value is String && !enum.contains(value) ->
sequenceOf(ValidationError(dataTrace,
"is not contained in enum [${enum.joinToString(", ")}]",
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be a good idea to create the string version of the enum at construction time so the building of the string doesn't need to happen every request.


import com.shoprunner.baleen.BaleenType

abstract class CoercibleType(val type: BaleenType) : BaleenType
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice


constructor(enum: Array<out Enum<*>>) : this(enum.map { it.name })

override fun name() = "enum"
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to require enum types to have a name. That will make the error messages clearer. Could you add a name attribute to the constructor that could be used for the name field and validate method?

@@ -42,12 +82,23 @@ object AvroEncoder {
dataDescription.nameSpace, false, fields)
}

fun encode(dataDescription: DataDescription, outputStream: PrintStream) {
infix fun DataDescription.encodeTo(directory: File): File {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good if this takes a type mapping function so custom types can be mapped.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can get the syntax you want without making the function infix. infix functions can only take a single argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow... Can you give an example of how it would look?

@kdallmeyer-sr
Copy link
Contributor Author

@HawaiianSpork made the changes as per our conversation

@kdallmeyer-sr kdallmeyer-sr changed the title WIP - Baleen <-> Avro generators Baleen <-> Avro generators Jun 20, 2018
@kdallmeyer-sr kdallmeyer-sr requested a review from a team June 20, 2018 20:36
@HawaiianSpork HawaiianSpork merged commit 09fb7e8 into ShopRunner:master Jun 20, 2018
@kdallmeyer-sr kdallmeyer-sr deleted the baleen-avro branch June 21, 2018 03:21
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

2 participants