-
Notifications
You must be signed in to change notification settings - Fork 513
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
Make Sparkey testable #5128
Make Sparkey testable #5128
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5128 +/- ##
==========================================
+ Coverage 63.34% 63.42% +0.08%
==========================================
Files 291 293 +2
Lines 10837 10902 +65
Branches 753 716 -37
==========================================
+ Hits 6865 6915 +50
- Misses 3972 3987 +15 ☔ View full report in Codecov by Sentry. |
@@ -127,6 +128,7 @@ private[values] class MultiMapSideInput[K, V](val view: PCollectionView[JMap[K, | |||
JMapWrapper.ofMultiMap(context.sideInput(view)) | |||
} | |||
|
|||
@deprecated(since = "0.14.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we include a deprecation message advising users what methods to use instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this is just a pure "don't do this"
scio-extra/src/main/scala/com/spotify/scio/extra/sparkey/SparkeyIO.scala
Outdated
Show resolved
Hide resolved
scio-extra/src/main/scala/com/spotify/scio/extra/sparkey/SparkeyIO.scala
Outdated
Show resolved
Hide resolved
…eyIO.scala Co-authored-by: Claire McGinty <[email protected]>
scio-extra/src/main/scala/com/spotify/scio/extra/sparkey/SparkeyIO.scala
Outdated
Show resolved
Hide resolved
scio-extra/src/main/scala/com/spotify/scio/extra/sparkey/SparkeyIO.scala
Outdated
Show resolved
Hide resolved
scio-extra/src/test/scala/com/spotify/scio/extra/sparkey/SparkeyTest.scala
Outdated
Show resolved
Hide resolved
scio-extra/src/main/scala/com/spotify/scio/extra/sparkey/SparkeyIO.scala
Show resolved
Hide resolved
scio-extra/src/main/scala/com/spotify/scio/extra/sparkey/SparkeyIO.scala
Show resolved
Hide resolved
scio-extra/src/main/scala/com/spotify/scio/extra/sparkey/instances/MockSparkeyReader.scala
Show resolved
Hide resolved
scio-extra/src/main/scala/com/spotify/scio/extra/sparkey/instances/MockSparkeyReader.scala
Outdated
Show resolved
Hide resolved
val input = Map("a" -> "b", "c" -> "d") | ||
JobTest[SparkeyJob.type] | ||
.args("--input=foo", "--output1=bar", "--output2=baz") | ||
.input(SparkeyIO("foo"), Seq(MockStringSparkeyReader(input))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead have SparkeyIO
expect a Map[K, V]
(or Seq[(K, V)]
) instead of a reader ? We would create the reader in the IO after isTest
is checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this initially; the type parameter on ScioIO
has to match the type of the mock data iterable passed to input
, then in the internals of JobTest
the SCollection returned must also be of the same type, so without changing the contract of ScioIO I don't think this is possible
Adds
SparkeyIO
, aTestIO
(rather than full-fledgedScioIO
). Deprecates sideinputmap
.Suggestions around naming the output Sparkey IO, currently
SparkeyIO.output[K, V]
, are welcome as this feels awkward. An alternative is to useSparkeyIOOutput[K, V]
directly, but this is also ugly.Instead of #3172
Fixes #3173
Fixes #4819