[GitHub] [beam] youngoli commented on a change in pull request #11179: [BEAM-3301] Bugfix in DoFn validation.
youngoli commented on a change in pull request #11179: [BEAM-3301] Bugfix in DoFn validation. URL: https://github.com/apache/beam/pull/11179#discussion_r395850295 ## File path: sdks/go/pkg/beam/core/graph/fn.go ## @@ -446,23 +444,16 @@ func validateMainInputs(fn *Fn, method *funcx.Fn, methodName string, numMainIn m return err } - // Check that the first numMainIn inputs are not side inputs (Iters or - // ReIters). We aren't able to catch singleton side inputs here since - // they're indistinguishable from main inputs. - mainInputs := method.Param[pos : pos+int(numMainIn)] - for i, p := range mainInputs { - if p.Kind != funcx.FnValue { - err := errors.Errorf("expected main input parameter but found "+ - "side input parameter in position %v", - pos+i) - err = errors.SetTopLevelMsgf(err, - "Method %v in DoFn %v should have all main inputs before side inputs, "+ - "but a side input (as Iter or ReIter) appears as parameter %v when a "+ - "main input was expected.", - methodName, fn.Name(), pos+i) - err = errors.WithContextf(err, "method %v", methodName) - return err - } + // Check that the first input is not an Iter or ReIter (those aren't valid + // as the first main input). + first := method.Param[pos].Kind + if first != funcx.FnValue { + err := errors.New("first main input parameter must be value type") Review comment: I'll just add it in real quick while squashing the commits. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [beam] youngoli commented on a change in pull request #11179: [BEAM-3301] Bugfix in DoFn validation.
youngoli commented on a change in pull request #11179: [BEAM-3301] Bugfix in DoFn validation. URL: https://github.com/apache/beam/pull/11179#discussion_r395836071 ## 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: I was originally picturing this as a helper function for callers of NewDoFn. It seems easy for future callers to make a mistake and only check if the PCollection is a KV and forget to check for CoGBK, so I thought a helper method would be useful in the future. 1. I missed that pardo.go is in the same package as pcollection.go. I'm also leaning to not expanding the user surface if it's not necessary. 2 & 3. Yeah I was unsure about the name, since it's not technically checking for KVs, I just couldn't think of anything better. IsKeyed sounds good though. 4. That's the other part I was debating. My goal was to make it easy to avoid the mistake in the future, but thinking about it... It seems unlikely that anyone else would even be using this code, and I would expect that if they were they were an advanced user doing something tricky. I think I'll go with just putting the conditional in pardo.go and adding a comment. We can always turn it into a helper later if it does get used in multiple places. 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: us...@infra.apache.org With regards, Apache Git Services