[GitHub] [beam] lostluck commented on a change in pull request #15400: [BEAM-11928] Implement stand-alone Combine Globally operation

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

lostluck commented on a change in pull request #15400:
URL: https://github.com/apache/beam/pull/15400#discussion_r703651332



##########
File path: sdks/go/pkg/beam/core/graph/edge.go
##########
@@ -450,8 +456,8 @@ func NewCombine(g *Graph, s *Scope, u *CombineFn, in *Node, ac *coder.Coder, typ
 	}
 
 	inT := in.Type()
-	if !typex.IsCoGBK(inT) {
-		return nil, addContext(errors.Errorf("Combine requires CoGBK type: %v", inT), s)
+	if !typex.IsCoGBK(inT) && s.Label == CombinePerKeyScope {
+		return nil, addContext(errors.Errorf("Combine Per Krequires CoGBK type: %v", inT), s)

Review comment:
       typo. Also, unless it helps reading, keeping the error wording to match terms the user writes helps debugging. Errors should be written in terms the user who runs into them can understand and handle. In this case, no spaces for "CombinePerKey". 
   
   ```suggestion
   		return nil, addContext(errors.Errorf("CombinePerKey requires CoGBK type: %v", inT), s)
   ```




-- 
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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org