feat(low-code cdk): add condition to add fields #409
Conversation
|
/autofix
|
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Suggested reviewers
Would you like to add these reviewers, wdyt? 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/transformations/add_fields.py (1)
141-143: Conditional field addition logic is well implemented.The transform method now checks if the condition is empty or evaluates to true before adding fields. This maintains backward compatibility (empty condition works like before) while adding new functionality.
One thought - should we consider adding debug logging when fields are skipped due to conditions not being met? This might help users troubleshoot their configurations, wdyt?
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
1471-1480: Review on the newconditionfield inAddFields:
You've added a new optionalconditionfield to control whether fields should be added, which aligns with similar patterns elsewhere in the schema (e.g., inRecordFilterandRemoveFields). A couple of quick questions:
- Are you comfortable with using an empty string (
"") as the default instead of perhaps usingNone? (I see consistency with other parts of the code, so this might be fine.)- The description string ends with an extra comma. Would you consider cleaning it up for clarity?
Here's a small diff suggestion to polish up the formatting:
- condition: Optional[str] = Field( - "", - description="Fields will be added if expression is evaluated to True.,", - examples=[ - "{{ property|string == '' }}", - "{{ property is integer }}", - "{{ property|length > 5 }}", - "{{ property == 'some_string_to_match' }}", - ], - ) + condition: Optional[str] = Field( + "", + description="Fields will be added if the expression evaluates to True.", + examples=[ + "{{ property|string == '' }}", + "{{ property is integer }}", + "{{ property|length > 5 }}", + "{{ property == 'some_string_to_match' }}", + ], + )wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(10 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(1 hunks)airbyte_cdk/sources/declarative/transformations/add_fields.py(3 hunks)unit_tests/sources/declarative/transformations/test_add_fields.py(16 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
[error] 709-709: Argument "condition" to "AddFields" has incompatible type "str | None"; expected "str"
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (7)
airbyte_cdk/sources/declarative/transformations/add_fields.py (3)
10-10: New import InterpolatedBoolean looks good!Nice addition of the
InterpolatedBooleanimport to support conditional field addition, aligning with the new feature.
90-90: Condition attribute is well-defined.The new
conditionattribute with default empty string provides good backward compatibility. This allows for optional conditional field addition without breaking existing implementations, wdyt?
96-98: Initializing filter interpolator looks good.Clean implementation of the condition evaluation using the
InterpolatedBooleanclass in the__post_init__method.airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
113-125: Well-defined condition property with helpful examples.The schema definition for the new
conditionproperty is clear and thorough:
- Description clearly explains the purpose
- Default empty string maintains backward compatibility
- Interpolation context includes the necessary variables
- Examples cover common use cases for filtering
The examples are particularly useful, showing different ways to use the condition:
- Checking for empty strings
- Type validation
- Length-based conditions
- Exact matching
This will be valuable for users implementing conditional field addition. Nice work!
unit_tests/sources/declarative/transformations/test_add_fields.py (3)
161-178: LGTM! Good test cases for the new condition parameterThe new test cases effectively validate the conditional behavior - verifying that fields are only added when the condition evaluates to True and skipped when False. This is a solid addition to ensure the new feature works as expected.
185-186: LGTM! Function signature updated correctlyThe function signature has been properly updated to include the new
conditionparameter.
193-193: LGTM! Correctly passing the condition parameterThe
AddFieldsconstructor now properly receives the condition parameter, ensuring the tests validate the new conditional functionality.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
1463-1472: The addition of theconditionfield looks good!This new feature adds flexibility to the
AddFieldstransformation by allowing it to conditionally add fields based on evaluated expressions. The examples provided are comprehensive and illustrate various use cases.Just noticed a tiny typo in line 1465 - there's an extra comma after "True" in the phrase "True.,". Nothing major, but might be worth removing for clarity, wdyt?
- description="Fields will be added if expression is evaluated to True.,", + description="Fields will be added if expression is evaluated to True.",unit_tests/sources/declarative/transformations/test_add_fields.py (1)
161-178: Good test cases for the new condition functionality!These test cases clearly demonstrate the behavior when the condition is explicitly set to
False(no fields added) orTrue(fields added).Would it also be valuable to test some of the more complex condition examples from your Field description, like string comparison or length checks? Something like
"{{ property|length > 5 }}"or"{{ property is integer }}", wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py(1 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(1 hunks)unit_tests/sources/declarative/transformations/test_add_fields.py(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
unit_tests/sources/declarative/transformations/test_add_fields.py (4)
14-16: LGTM! Good update to the parametrize decorator.The test parameter list has been correctly updated to include the new
conditionparameter.
21-22: Solid backward compatibility approach.Adding an empty string condition to existing test cases ensures backward compatibility while testing the new functionality. The empty string is correctly treated as a default "true" condition where fields are added.
Also applies to: 30-31, 39-40, 48-49, 57-58, 66-67, 75-76, 84-85, 93-94, 102-103, 111-112, 120-121, 129-130, 138-139, 147-148, 156-157
185-186: LGTM! Function signature updated correctly.The test function signature has been properly updated to include the new
conditionparameter.
193-195: Constructor updated appropriately.The
AddFieldsconstructor call now includes theconditionparameter, ensuring it's properly passed to the transformation. The line formatting maintains good readability despite the added parameter.
|
/autofix
|
Fixed : https://github.com/airbytehq/airbyte-internal-issues/issues/11938
Summary by CodeRabbit