You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "lostluck (via GitHub)" <gi...@apache.org> on 2023/02/15 06:01:02 UTC

[GitHub] [beam] lostluck commented on a diff in pull request #25437: [Go SDK]: Allow SDF methods to have context param and error return value

lostluck commented on code in PR #25437:
URL: https://github.com/apache/beam/pull/25437#discussion_r1106682555


##########
sdks/go/pkg/beam/core/runtime/exec/sdf_invokers.go:
##########
@@ -39,61 +43,45 @@ import (
 
 // cirInvoker is an invoker for CreateInitialRestriction.
 type cirInvoker struct {
-	fn   *funcx.Fn
-	args []any // Cache to avoid allocating new slices per-element.
-	call func(elms *FullValue) (rest any)
+	fn     *funcx.Fn
+	args   []any // Cache to avoid allocating new slices per-element.
+	ctxIdx int
+	call   func() (rest any, err error)
 }
 
 func newCreateInitialRestrictionInvoker(fn *funcx.Fn) (*cirInvoker, error) {
 	n := &cirInvoker{
 		fn:   fn,
 		args: make([]any, len(fn.Param)),
 	}
+
+	var ok bool
+	if n.ctxIdx, ok = fn.Context(); !ok {
+		n.ctxIdx = -1
+	}
+
 	if err := n.initCallFn(); err != nil {
 		return nil, errors.WithContext(err, "sdf CreateInitialRestriction invoker")
 	}
 	return n, nil
 }
 
-func (n *cirInvoker) initCallFn() error {
-	// Expects a signature of the form:
-	// (key?, value) restriction
-	// TODO(BEAM-9643): Link to full documentation.
-	switch fnT := n.fn.Fn.(type) {
-	case reflectx.Func1x1:

Review Comment:
   Up until this point, the SDF methods all had known "arities" of Input Parameters and output parameters. So, the normal Beam DoFn Bundle Lifecycle methods (StartBundle, ProcessElement, FinishBundle) could have fairly arbitrary inputs and up to 5 outputs (ignoring emitters).
   
   The idea behind splitting out the SDF invokers was because they had a limited set of "arities" they could have, so they could take a shorter less abstract path. The main trick is they aren't exactly the same as the other methods (dealing with emitters, iterators, etc).
   
   There's no great reason to split them out now if the general handling works for them though.



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