Unwrap internal SCollection coders to be reused in transforms #4690
Conversation
| @@ -27,7 +27,7 @@ private[values] trait PCollectionWrapper[T] extends TransformNameable { | |||
| /** The [[org.apache.beam.sdk.values.PCollection PCollection]] being wrapped internally. */ | |||
| val internal: PCollection[T] | |||
|
|
|||
| implicit def coder: Coder[T] = Coder.beam(internal.getCoder) | |||
There was a problem hiding this comment.
Here we were propagating wrapped coders
Codecov Report
@@ Coverage Diff @@
## main #4690 +/- ##
==========================================
- Coverage 60.64% 60.63% -0.01%
==========================================
Files 286 286
Lines 10453 10456 +3
Branches 694 669 -25
==========================================
+ Hits 6339 6340 +1
- Misses 4114 4116 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
RustedBones
requested review from
a team and
lcguerrerocovo
and removed request for
a team
| case c: RecordCoder[(K, V)] => | ||
| ( | ||
| c.cs.find(_._1 == "_1").get._2.asInstanceOf[beam.Coder[K]], |
There was a problem hiding this comment.
Prefer to rely on type position that on the field name
| case _ => | ||
| throw new IllegalArgumentException( | ||
| s"Failed to extract key-value coders from Coder[(K, V)]: $coder" | ||
| ) | ||
| } | ||
| (Coder.beam(k), Coder.beam(v)) | ||
| (Coder.beam(unwrap(k)), Coder.beam(unwrap(v))) |
There was a problem hiding this comment.
Is calling unwrap required everywhere Coder.beam is called?
There was a problem hiding this comment.
No. Most of the usage of Coder.beam are not with unwrap like here.
We have to unwrap when moving back from materilalized coder back to scio coders. (reuse of internal coders in partial transformation with keyBy, mapValues, ...)
When
NullableCodersis tuned on, users could get the following errorThis happens when
keyByis used. It only requires the key coder as implicit. The value coder is propagated for the materialized coder set on the internal collection.When using pre-materialized coders, We should make sure those are completely unwrapped.