-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat(ingest): Add GenericAspectTransformer #7994
feat(ingest): Add GenericAspectTransformer #7994
Conversation
With these changes, our aim is to create the infrastructure that will allow creating transformers to consume generic aspects.
BaseTransformer has been extended to support MetadataChangeProposalClass |
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.
Thanks for your contribution to datahub! I think GenericAspectTransformer
may be more appropriate as another mixin (like LegacyMCETransformer
and SingleAspectTransformer
, but this works as is.
elif isinstance(envelope.record, MetadataChangeEventClass): | ||
self._record_mce(envelope.record) | ||
elif isinstance(envelope.record, MetadataChangeProposalWrapper): | ||
self._record_mcp(envelope.record) |
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 can handle both these cases by:
- Pulling the appropriate aspect out of the MCE
- Converting the MCPW into a MCP with
make_mcp
def transform_aspect( | ||
self, entity_urn: str, aspect_name: str, aspect: Optional[Aspect] | ||
) -> Optional[Aspect]: | ||
"""Do not implement.""" | ||
pass |
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.
I can see for simplicity why you inherited from SimpleAspectTransformer
, but I think it's awkward we're essentially ignoring the main method of that class.
Checklist