Magnolify API

[kellen]

Adds magnolify-based 'typed' APIs.

This is a kind of brute-force change that adds several ugly but easily renamed APIs while not conflicting with the existing macro-based APIs, which we hope to deprecate in a future version. I attempted some advanced type gymnastics to allow better naming, but Miles Sabin I am not.

Deprecates AvroType and BigQueryType, breaks binary compatibility.

Avro

• Adds the horrifically-named sc.typedAvroFileMagnolify, avoiding conflicts with avroFile and macro-based typedAvroFile. Alternative: typedAvro (no File)
• Adds coll.saveAsAvroFile

Bigtable

• Adds typedBigtable
• Adds saveAsBigtable

BigQuery

• Adds sc.typedBigQuerySelect, typedBigQueryTable; Select and Table to avoid conflicts with the macro read variants
• Adds abhorrent typedBigQueryStorageMagnolify, avoiding conflicts with bigQueryStorage and macro-based typedBigQueryStorage
• Adds saveAsBigQueryTable, avoiding macro-based saveAsTypedBigQueryTable. Other saveAsBigQueryTable variants are scoped to TableRow and GenericRecord.

Datastore

• Adds typedDatastore
• Adds saveAsDatastore

Protobuf

• Adds typedProtobufFile
• Adds saveAsProtobufFile

Tensorflow

• Adds typedTfRecordFile
• Adds saveAsTfRecordFile

[codecov]

Codecov Report

Attention: Patch coverage is 48.59813% with 110 lines in your changes are missing coverage. Please review.

Project coverage is 62.55%. Comparing base (a658edb) to head (c65354f).
Report is 9 commits behind head on v0.15.x.

❗ Current head c65354f differs from pull request most recent head 9bc45a1. Consider uploading reports for the commit 9bc45a1 to get more accurate results

Files	Patch %	Lines
...n/scala/com/spotify/scio/bigtable/BigTableIO.scala	11.32%	47 Missing ⚠️
...n/scala/com/spotify/scio/bigquery/BigQueryIO.scala	22.72%	17 Missing ⚠️
...scala/com/spotify/scio/datastore/DatastoreIO.scala	19.04%	17 Missing ⚠️
...otify/scio/bigtable/syntax/ScioContextSyntax.scala	38.46%	8 Missing ⚠️
...io/bigquery/dynamic/syntax/SCollectionSyntax.scala	0.00%	6 Missing ⚠️
...otify/scio/bigtable/syntax/SCollectionSyntax.scala	50.00%	4 Missing ⚠️
...scala/com/spotify/scio/tensorflow/TFRecordIO.scala	84.00%	4 Missing ⚠️
.../main/scala/com/spotify/scio/avro/ProtobufIO.scala	76.92%	3 Missing ⚠️
...main/scala/com/spotify/scio/avro/AvroTypedIO.scala	81.81%	2 Missing ⚠️
...rc/main/scala/com/spotify/scio/bigquery/taps.scala	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           v0.15.x    #5286      +/-   ##
===========================================
- Coverage    62.63%   62.55%   -0.09%     
===========================================
  Files          301      302       +1     
  Lines        10842    11013     +171     
  Branches       748      746       -2     
===========================================
+ Hits          6791     6889      +98     
- Misses        4051     4124      +73     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[RustedBones]

shouldn't this be wrapped in a transform too ?

[kellen]

Unfortunately when calling write on an underlying IO, the returned tap obscures the contained steps. Specifically, this doesn't work:

data.transform { scoll =>
  scoll
      .map(t => pt.to(t))
      .write(underlying)(params.copy(metadata = metadata))
}

You can use writeWithContext directly ala:

underlying.writeWithContext(
  data.map(t => pt.to(t)),
  params.copy(metadata = metadata)
}

but this doesn't solve our issue as there is a single map step here so we wouldn't be combining any steps inside a transform (data.transform(_.map(t=>pt.to(t))) == data.map(t=>pt.to(t))) and any steps in the underlying transform are chained afterwards.

For these cases we'd need some way to capture or rewrite previous steps in the graph.

[RustedBones]

IMHO this was a mistake to name that typedAvro.... This should have been avroTyped....
This improves developer experience when APIs are grouped by prefix

[kellen]

Agree, I've always found the typed prefix annoying. We could scalafix it I guess?

[RustedBones]

yes, the question is if we want that change for 0.15? We can discuss this later anyway. I'd better get this change in another PR

[clairemcginty]

can we make T: AvroMagnolifyType part of TypedMagnolifyAvroSCollectionOps's context bound so that all methods have access to it?

[RustedBones]

I vouched for the opposite in another comment :)
Reason: I'd rather have the method available to the IDE completion suggestion, even if there is no implicit found. This is also consistent with the coder behavior, required on the method rather that the implicit extensions

[clairemcginty]

makes sense! I'm in favor of making APIs more IDE-friendly 👍

[RustedBones]

Note: since all Scio IOs are public, we should create scalafix migration rules to change to the new names

[RustedBones]

Great work.
Deprecation of the BQ api makes sense on the AvroType and toTable.

BQ from* does not really have an equivalent. I feel the redirection to magnolify can be confusing but I don't have better alternative.

[RustedBones]

should we create overload ?

[kellen]

I'm ambivalent to/slightly against providing every possible override combination, so did not do that here.

[RustedBones]

I'm fine with that