Update the PTransform and associated APIs to be less class-based. #17699
Conversation
robertwb
force-pushed
the
javascript-no-classes
branch
from
5b6cc3b to
c8e387f
Compare
|
R: @damccorm This should be fairly straightforwards, mostly class -> function, capitalization, and deleting new. |
robertwb
force-pushed
the
javascript-no-classes
branch
from
c8e387f to
b0dbc37
Compare
| } | ||
| } | ||
|
|
||
| register(urn: string, constructorOrClass: Class<Coder<any>>) { |
There was a problem hiding this comment.
This is called constructorOrClass, but it has to just be the class, right? Its actually just generally unclear to me why we need this function (vs directly calling registerClass) - are you intending to do some branching here depending on the input type?
There was a problem hiding this comment.
The intent was to accept both here, but I wasn't able to figure out how to cleanly distinguish between the two. Dropping a TODO for now.
| } | ||
|
|
||
| flatten.urn = "beam:transform:flatten:v1"; |
There was a problem hiding this comment.
What are your thoughts on moving this (and other similar urns) into some constant urn file in this or a future PR? I saw this referenced in an earlier file and was confused since you don't see functions with fields assigned to them too often. I'm not a JS expert, but I haven't seen this pattern before (admittedly I also can't find any style guides that mention it positively or negatively)
There was a problem hiding this comment.
These URNs are defined in the proto files, but I wasn't sure how to extract them out. This seemed the most natural place to stick it as a convention.
| // TODO: (Typescript) Is it possible to type that this takes | ||
| // PCollection<{a: T, b: U, ...}> to {a: PCollection<T>, b: PCollection<U>, ...} | ||
| // Seems to requires a cast inside expandInternal. But at least the cast is contained there. | ||
| export class Split2<T extends { [key: string]: unknown }> extends PTransform< |
There was a problem hiding this comment.
Its not totally clear to me why we had this initially, or why we're now removing Spit - could you help me understand what the split vs split2 distinction was for and why its good to now coallesce the behavior?
There was a problem hiding this comment.
I consolidated them because having two primitives that do almost the same thing in slightly different was undesirable. The original Split can be easily implemented in terms of Split2 which is more flexible (and, IMHO, easier to understand).
| } | ||
|
|
||
| return withName("flatten", expandInternal); |
There was a problem hiding this comment.
I think you're missing the withName import
Co-authored-by: Danny McCormick <[email protected]>
Co-authored-by: Danny McCormick <[email protected]>
|
Thanks. Addressed your comments and fixed the build. PTAL. |
This gets us closer to stabilizing the API.
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.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.