-
Notifications
You must be signed in to change notification settings - Fork 208
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: Multi-yield transformers #396
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
b8eaba9
to
cc3037d
Compare
Signed-off-by: Joseph Atkins-Turkish <[email protected]>
cc3037d
to
4826ce2
Compare
Signed-off-by: Joseph Atkins-Turkish <[email protected]>
will take a look this pr early next week. |
Signed-off-by: Joseph Atkins-Turkish <[email protected]>
48abf60
to
fb38cb2
Compare
@feng-tao I just fixed the conflicts and sorted the imports so this passes CI again. Anything else I can do to help get it in? |
sorry for the wait, will look at this week. |
feng-tao
approved these changes
Jan 23, 2021
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
Wonong
pushed a commit
to Wonong/amundsendatabuilder
that referenced
this pull request
Mar 4, 2021
* Implement multi-yield transformers Signed-off-by: Joseph Atkins-Turkish <[email protected]> * add license Signed-off-by: Joseph Atkins-Turkish <[email protected]> * isort Signed-off-by: Joseph Atkins-Turkish <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of Changes
This makes it possible for transformers to yield multiple records. This allows transformers to enrich records by yielding additional data.
For example, if my company has a convention for documenting owners in table descriptions using
@owner: name
, a transformer in the table extraction pipeline could yieldTableOwner
(as well as the original record).A small knock-on effect of this approach was that, since
ChainedTransformer
now always returns an iterator, I code was expecting a ChainedTransformer to return a value, I had to wrap it withnext(..., None)
.Tests
I added an integration test which exercises the task and chained transformer changes end-to-end. This is pretty much copied out of the test I wrote for this in my own codebase. I know it doesn't quite fit the existing convention, but, I do think having end-to-end tests like this can be useful anyway. Would be interesting to get thoughts.
Documentation
I updated the README.
CheckList
Make sure you have checked all steps below to ensure a timely review.
make test