feat(low-code cdk): add overwrite flag to dpath flatten #410
Conversation
|
/autofix
|
📝 WalkthroughWalkthroughThis PR adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant T as DpathFlattenFields
participant R as Record
participant D as dpath
Note over T: Begin transformation
T->>T: Extract fields from R
alt replace_record is True and extracted data exists
T->>D: dpath.delete(R, "**")
T->>T: Update R with extracted data
else replace_record is False or no extracted data
T->>T: Check for key conflicts
alt No conflicts & delete_origin_value is True
T->>D: dpath.delete(R, specified path)
T->>T: Merge extracted data into R
else
T->>T: Merge extracted data into R without deletion
end
end
Note over T: Return updated record
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(2 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(2 hunks)airbyte_cdk/sources/declarative/transformations/add_fields.py(3 hunks)airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py(2 hunks)unit_tests/sources/declarative/transformations/test_add_fields.py(16 hunks)unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py(11 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
[error] 711-711: Argument "condition" to "AddFields" has incompatible type "str | None"; expected "str"
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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: Pytest (Fast)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (19)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (3)
18-18: Documentation update aligns with new feature - nice!I see you've added documentation for the new
replace_recordparameter in the class docstring. This clearly explains the purpose and default value of the parameter, which is essential for developers using this class.
26-26: Parameter declaration looks goodThe
replace_recordparameter is properly initialized with a default value ofFalse, which ensures backward compatibility with existing code that doesn't specify this parameter.
53-61:❓ Verification inconclusive
The record replacement logic looks clear and efficient
The implementation handles the record replacement nicely. I like how you've:
- First checked if
replace_recordis enabled andextractedhas content- Used
dpath.delete(record, "**")to clear all fields in the record- Preserved the existing conflict-checking logic for the non-replacement case
I'm curious though - have you considered handling potential edge cases where
extractedmight contain unexpected types? For instance, what ifextractedis a list or a primitive value instead of a dict? The code assumes it's a dict in the non-replacement case.
🏁 Script executed:
#!/bin/bash # Let's check if there are any callers that might pass non-dict values to extracted rg -A 2 "extracted\s*=" airbyte_cdk/sourcesLength of output: 1513
Record replacement logic verified – type checks are in place!
I ran a grep search and noted that before reaching this snippet, the code already ensures that
extractedis a dict (using anisinstance(extracted, dict)check) before performing anyupdateoperations. It looks like returning a list or a primitive value (via defaults such as[]or from other code paths) is handled differently in the extractor logic, so the record update flow here is safe.
- The previous check guarantees that only dicts reach this update logic.
- The default value and guard ensure non-dict types are processed elsewhere.
Would you consider documenting this intentional safeguard for clarity, or do you feel additional checks aren’t necessary? wdyt?
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
2275-2278: Schema definition for new property is well structuredThe
replace_recordproperty has been properly defined in the schema with:
- Clear title
- Detailed description that matches the code documentation
- Correct type (boolean)
- Default value that matches the code implementation
This ensures consistency between the schema and implementation, which is excellent for maintainability.
airbyte_cdk/sources/declarative/transformations/add_fields.py (3)
90-90: Newconditionparameter looks goodI see you've added a
conditionstring parameter with an empty default value. This is a nice enhancement that allows for conditional field addition, and the empty default maintains backward compatibility.
96-98: InterpolatedBoolean initialization looks correctCreating the
_filter_interpolatorin__post_init__matches the pattern used elsewhere in the codebase. The instance is correctly initialized with the condition and parameters.
141-143: Condition evaluation logic is clean and efficientThe implementation:
- First checks if the condition is empty (fast path)
- Only evaluates the interpolator when needed
- Passes the necessary context (
config,value, and**kwargs)This approach is efficient and maintains the expected behavior for existing code. Good work!
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
880-884: Great addition of thereplace_recordoption to enhance flattening control!This new flag gives users more flexibility in how they handle data transformation. The default value of
None(which I assume defaults toFalsein the implementation) ensures backward compatibility with existing configurations.
1468-1477: Nice implementation of conditional field addition!The
conditionparameter will make the AddFields transformation much more flexible. The examples provided in the description are helpful for users to understand how to use this feature. The default empty string value maintains backward compatibility with existing configurations.unit_tests/sources/declarative/transformations/test_add_fields.py (4)
14-15: Good update to include condition parameter in the test signature.You've properly updated the test parameter list to include the new
conditionparameter. This ensures the tests will validate the new functionality.
161-178: Excellent test cases for the new condition functionality!These test cases clearly validate both the positive and negative conditions, ensuring that fields are only added when the condition evaluates to True. This provides good coverage for the new feature.
185-186: Good addition of condition parameter to the test function signature.The function signature is properly updated to accept the new parameter.
193-195: Properly passing the condition parameter to the AddFields transformation.You've correctly updated the AddFields initialization to include the new condition parameter, ensuring it's passed through to the transformation logic.
unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py (5)
7-9: Good addition of constants for the replace_record functionality.Adding these constants makes the test cases more readable and maintainable. They clearly indicate the purpose of the boolean values.
18-19: Well-structured update to include replace_record in the parameter list.You've properly updated the parameter list to include the new functionality.
113-130: Great test cases to validate the replace_record functionality!These new test cases specifically test the behavior when
replace_recordis True, ensuring that:
- When
replace_recordis True, only the flattened fields remain in the output- The value of
delete_origin_valuedoesn't affect the result whenreplace_recordis TrueThis comprehensively covers the new functionality.
134-135: Good update to the test function signature.You've correctly updated the function signature to include the new
replace_recordparameter.
136-142: Properly passing the replace_record parameter to DpathFlattenFields.The instantiation of DpathFlattenFields now includes the new parameter, ensuring it's used in the transformation process.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
739-750: Good implementation of the replace_record featureThe new
replace_recordparameter is properly implemented with a default value when it'sNone. This parameter aligns with the PR objective of adding an "overwrite" flag to the dpath flatten functionality.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
Outdated
Show resolved
Hide resolved
|
/autofix
|
Fixed: https://github.com/airbytehq/airbyte-internal-issues/issues/11937
Summary by CodeRabbit
New Features
Chores