Integrate avro datum factory in scio-avro #5152
Conversation
scio-avro/src/main/scala/com/spotify/scio/avro/AvroDatumFactory.scala
Outdated
Show resolved
Hide resolved
scio-avro/src/main/scala/com/spotify/scio/avro/AvroDatumFactory.scala
Outdated
Show resolved
Hide resolved
scio-avro/src/main/scala/com/spotify/scio/avro/AvroDatumFactory.scala
Outdated
Show resolved
Hide resolved
Co-authored-by: Claire McGinty <clairem@spotify.com>
RustedBones
force-pushed
the
avro-datum-factory
branch
from
f62313e to
ee71a06
Compare
|
|
||
| // overloaded API. We can't use default params | ||
| def avroFile(path: String, schema: Schema, suffix: String): SCollection[GenericRecord] = | ||
| self.read(GenericRecordIO(path, schema))(AvroIO.ReadParam(suffix)) | ||
| def avroFile( |
There was a problem hiding this comment.
In the ideal situation we should have avroGenericFile and avroSpecificFile to avoid those method overload
There was a problem hiding this comment.
Agree that the overloading in these methods has always been confusing -- particularly with the schema param since it's only required for Generic reads. On the other hand, it's probably the most commonly used Scio API and migration pain would be high, I think... maybe we can think about this for 0.15?
There was a problem hiding this comment.
sure. nothing urgent. Just wanted to raise this since adding another new parameter to the API will be a pain.
| val schema = AvroCoders.schemaForClass(clazz).getOrElse { | ||
| val msg = | ||
| "Failed to create a coder for SpecificRecord because it is impossible to retrieve an " + | ||
| s"Avro schema by instantiating $clazz. Use only a concrete type implementing " + | ||
| s"SpecificRecord or use GenericRecord type in your transformations if a concrete " + | ||
| s"type is not known in compile time." |
There was a problem hiding this comment.
I dropped this. Non standard avro classes should be handled using custom datum factory and coder
subset of #4928