-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BEAM-3221, BEAM-4180] Clarify documentation for StandardTransforms.Primitives, Pipeline, and PTransform. #10779
Conversation
@@ -78,31 +78,45 @@ message Components { | |||
// A Pipeline is a hierarchical graph of PTransforms, linked | |||
// by PCollections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add something like "a typical graph may look like:"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, just some minor suggestions and questions.
// PCollections, SDK environments, UDF, etc., for | ||
// supporting compact reuse and arbitrary graph structure. | ||
// Impulse -> PCollection -> ParDo -> PCollection -> GroupByKey -> ... | ||
// \> PCollection -> ParDo -> ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? I thought it would look like:
Impulse -> PCollection -> ParDo -> PCollection -> GroupByKey -> ...
\> ParDo -> ...
Or maybe the ParDo you're documenting is a multi-output one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a multi output one.
// elements that are ultimately added to the final output PCollection that the | ||
// transform produces. | ||
// | ||
//The Beam SDKs contain a number of different transforms that you can apply to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a space here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// is a ParDoPayload, and so on. | ||
// | ||
// TODO: document the standardized URNs and payloads | ||
// TODO: separate standardized payloads into a separate proto file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we didn't actually do this second TODO, ParDoPayload et al are still here. I guess we don't want to move them anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I closed the related BEAM-4180 since the effort isn't worth it and the Payload
term does provide meaning that its meant to be embedded.
Retest this please |
Run Python2_PVR_Flink PreCommit |
Run Java PreCommit |
Run Python2_PVR_Flink PreCommit |
Run Java11 PreCommit |
Run JavaPortabilityApiJava11 PreCommit |
Run Java_Examples_Dataflow_Java11 PreCommit |
Run Java11 PreCommit |
Run JavaPortabilityApiJava11 PreCommit |
Run Java_Examples_Dataflow_Java11 PreCommit |
retest this please |
…Pipeline, and PTransform.
Run Python PreCommit |
5 similar comments
Run Python PreCommit |
Run Python PreCommit |
Run Python PreCommit |
Run Python PreCommit |
Run Python PreCommit |
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-XXX
with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
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.