[BEAM-5354] Add side input nodes to their consumer's parent-scope. #8713
Conversation
|
Run Go Postcommit |
| @@ -128,7 +128,7 @@ func (m *marshaller) addScopeTree(s *ScopeTree) string { | |||
|
|
|||
| var subtransforms []string | |||
| for _, edge := range s.Edges { | |||
| subtransforms = append(subtransforms, m.addMultiEdge(edge)) | |||
| subtransforms = append(subtransforms, m.addMultiEdge(edge)...) | |||
There was a problem hiding this comment.
Learning question, does the addition of ... here prevents creation of a new slice?
There was a problem hiding this comment.
... is the operator that says "unroll this slice into the variadic parameter"
The built-in function append([]T, T...) is special in that it's got additional compiler support for optimizing the behavior in cases like this. In all cases if "subtransforms" has extra capacity, no new backing array for the slice will be allocated.
Other than ensuring we're appending strings, rather than []string to a []string, so the types work out, IIRC a single append call will only lead to at most a single allocation of a new slice.
eg. The runtime will check the length of the parameter being ...'d, and only increase it by at most once.
Since append is special, I wouldn't expect the returned slice from addMultiEdge to be copied.
For other uses, in user defined functions, functionally variadics (...string) operate identically to having a slice ([]string) on the receiver side, but the caller has the option to pass in disparate parameters as well. The main restriction is variadics must be the final parameter.
I don't know if the slice is copied for those, but I do believe it woul be the slice header (the pointer to the backing array, length, and capacity), and not the whole backing array that would be copied.
There's likely nuance going on here that I'm missing, but the above should be the right shape of it.
Some transforms require additional nodes in the Portable Pipeline Proto representation, and in particular, side inputs. The node representing the source of the Side input was previously being added as a root transform, which is OK as far as the model is concerned, but for ordering dependent runners (like Dataflow is currently), this can cause issues, since it requires nodes be specified before they're depended on (topological ordering).
This change ensures that new side input nodes are added as siblings to their consumer's scope, rather than as unique root transforms, which fits things into the pipeline hypergraph naturally.
Added additional tests to the translation from the Go SDK's internal representation, to the PPP, and modified one of the integration tests to trigger this behavior.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.