-
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
[YAML] Allow windowing to be done in Java or Python. #30055
Conversation
R: @Polber |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
ad3ac14
to
062442a
Compare
class TranslatingProvider(Provider): | ||
def __init__( | ||
self, | ||
transforms: Mapping[str, Callable[..., beam.PTransform]], | ||
underlying_provider: Provider): | ||
self._transforms = transforms | ||
self._underlying_provider = underlying_provider | ||
|
||
def provided_transforms(self): | ||
return self._transforms.keys() | ||
|
||
def available(self): | ||
return self._underlying_provider.available() | ||
|
||
def cache_artifacts(self): | ||
return self._underlying_provider.cache_artifacts() | ||
|
||
def underlying_provider(self): | ||
return self._underlying_provider | ||
|
||
def to_json(self): | ||
return {'type': "TranslatingProvider"} | ||
|
||
def create_transform( | ||
self, typ: str, config: Mapping[str, Any], | ||
yaml_create_transform: Any) -> beam.PTransform: | ||
return self._transforms[typ](self._underlying_provider, **config) |
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.
Does there need to be a new provider? Seems as though RenamingProvider
has all the relevant functionality and would allow mappings and defaults should there be Java built-in transforms that expose those in the future
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.
This does a deeper rewriting of the arguments than renaming, though I was thinking that the renaming transform could be defined as a special case of this one. (The SQL-based one possibly as well.)
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 think they could be merged in either direction, but not a blocker. LGTM
class TranslatingProvider(Provider): | ||
def __init__( | ||
self, | ||
transforms: Mapping[str, Callable[..., beam.PTransform]], | ||
underlying_provider: Provider): | ||
self._transforms = transforms | ||
self._underlying_provider = underlying_provider | ||
|
||
def provided_transforms(self): | ||
return self._transforms.keys() | ||
|
||
def available(self): | ||
return self._underlying_provider.available() | ||
|
||
def cache_artifacts(self): | ||
return self._underlying_provider.cache_artifacts() | ||
|
||
def underlying_provider(self): | ||
return self._underlying_provider | ||
|
||
def to_json(self): | ||
return {'type': "TranslatingProvider"} | ||
|
||
def create_transform( | ||
self, typ: str, config: Mapping[str, Any], | ||
yaml_create_transform: Any) -> beam.PTransform: | ||
return self._transforms[typ](self._underlying_provider, **config) |
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 think they could be merged in either direction, but not a blocker. LGTM
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with 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 or the workflows README to see a list of phrases to trigger workflows.