damccorm commented on code in PR #17432:
URL: https://github.com/apache/beam/pull/17432#discussion_r859936695
##########
sdks/go/pkg/beam/core/graph/fn.go:
##########
@@ -847,9 +886,15 @@ func validateSdfSigNumbers(fn *Fn, num int) error {
func validateSdfSigTypes(fn *Fn, num int) error {
restrictionT := fn.methods[createInitialRestrictionName].Ret[0].T
rTrackerT := reflect.TypeOf((*sdf.RTracker)(nil)).Elem()
-
- for _, name := range requiredSdfNames {
- method := fn.methods[name]
+ bRTrackerT := fn.methods[createTrackerName].Ret[0].T
+ optionalSdfs := optionalSdfNameMap()
+
+ for _, name := range sdfNames {
+ method, ok := fn.methods[name]
+ if !ok && optionalSdfs[name] {
+ // skip validating unimplemented optional sdf methodsß
+ continue
+ }
Review Comment:
Ah, I missed that this would throw if the method is unimplemented before
your changes - I understand why you're doing this now.
With that said, I think it might be cleaner to move the optional sdfs into
their own switch statement rather than doing this kind of filtering. That also
lets us keep it so that this function will throw if a required sdf name isn't
present (I think other validation does check that, so its probably not a big
deal either way). That would let you get rid of this map entirely and you
wouldn't have to do any special filtering.
I'll leave that as an optional suggestion
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
