[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
