[GitHub] [beam] lostluck commented on a change in pull request #11179: [BEAM-3301] Bugfix in DoFn validation.

[GitBox <gi...@apache.org>]

lostluck commented on a change in pull request #11179: [BEAM-3301] Bugfix in DoFn validation.
URL: https://github.com/apache/beam/pull/11179#discussion_r395444326
 
 

 ##########
 File path: sdks/go/pkg/beam/pcollection.go
 ##########
 @@ -60,6 +60,12 @@ func (p PCollection) Type() FullType {
 	return p.n.Type()
 }
 
+// OutputsKV returns whether the output of this PCollection are single value
+// elements or KV pairs.
+func (p PCollection) OutputsKV() bool {
 
 Review comment:
   1. No need to have this exported right now, since we can't make a breaking change later. It's only used in the same package. Let's not expand the user surface unless there are good user usages, as a broad API get confusing.
   
   2. IsKV would be a more precise name, since PCollections are a logical representation of all their data, not actually a source or a sink. They can represent KV type or they aren't.
   
   3. This isn't checking if it's a KV type, it's checking if it's a Keyed type or not, since it's also checking if it's a CoGBK.
   
   4. Since this is only used in the one place, it's reasonable to move the conditional there instead of adding the one off helper method.
   
   I'm always willing to hear other opinions!

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services