-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
[Low-code CDK] Add ability to remove fields #14402
Conversation
""" | ||
for pointer in self._field_pointers: | ||
try: | ||
dpath.util.delete(record, pointer) |
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.
very cool! can we make sure there's a test for removing nested fields?
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
airbyte-cdk/python/airbyte_cdk/sources/declarative/requesters/http_requester.py
Outdated
Show resolved
Hide resolved
# | ||
|
||
|
||
# The order of imports matters, so we add the split directive below to tell isort to sort imports while keeping RecordTransformation |
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.
why does the order matter?
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.
because every other class in this package implements RecordTransformation
, so if you try to import RemoveFields
first, it'll try importing RecordTransformation
which goes through this init.py
. Added a comment to clarify.
During transformation, if a field or any of its parents does not exist in the record, no error is thrown. | ||
|
||
If an input field pointer references an item in a list (e.g: ["k", 0] in the object {"k": ["a", "b", "c"]}) then | ||
the object at that index is set to None rather than being not entirely removed from the list. TODO change this behavior. |
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.
+1 for removing the None
value from the list
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 agree this is the better behavior but given that dpath
implements it this way I hesitated on adding a custom implementation. After thinking about it, this seemed like an OK thing to leave as a TODO
for the following reasons: this case (removing an element in an array) seems like an uncommon use case. The 98% case is removing the whole field. Plus, for some reasons outlined in this issue I opened in dpath I think implementing this correctly is tricky, because it will require shifting array indices in a stateful way.
WDYT?
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.
agreed that this is not a common use case. sounds good to me!
@@ -0,0 +1,55 @@ | |||
# | |||
# Copyright (c) 2022 Airbyte, Inc., all rights reserved. | |||
# |
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 think CI will complain about a missing new line between the header and the imports
…http_requester.py Co-authored-by: Alexandre Girard <alexandre@airbyte.io>
…l-cdk-field-transforms
…hq/airbyte into sherif/yaml-cdk-field-transforms
airbyte-cdk/python/airbyte_cdk/sources/declarative/transformations/transformation.py
Show resolved
Hide resolved
airbyte-cdk/python/unit_tests/sources/declarative/transformations/test_remove_fields.py
Show resolved
Hide resolved
…ions/transformation.py
…ons/test_remove_fields.py
…hq/airbyte into sherif/yaml-cdk-field-transforms
What
Closes #12988
The main differences from the issue as spec'd by Alex:
Recommended reading order
DeclarativeStream
RecordTransformation
RemoveFieldTransformation