feat(low-code): added key_transformation to DpathFlattenFields #472
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new definition for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser as ModelToComponentFactory
participant Transformer as DpathFlattenFields
Client->>Parser: Call create_dpath_flatten_fields(model, config, key_transformation)
Parser->>Transformer: Instantiate DpathFlattenFields with key_transformation
Transformer->>Transformer: Check if key_transformation is provided
alt key_transformation exists
Transformer->>Transformer: Apply prefix/suffix to keys
else
Transformer->>Transformer: Use original keys
end
Transformer-->>Client: Return transformed fields
Possibly related PRs
Suggested reviewers
Would you like me to suggest adding lazebnyi or darynaishchenko as well, or keep it focused on these two? Wdyt? Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 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 (9)
🪧 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: 4
🔭 Outside diff range comments (1)
unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py (1)
1-188:⚠️ Potential issueMinor formatting issue detected in pipeline.
There's a Ruff formatting error reported in the pipeline. Could you run the formatter to fix this issue? The test looks great functionality-wise!
#!/bin/bash # Check which files would be reformatted by Ruff ruff format --check .🧰 Tools
🪛 GitHub Actions: Linters
[error] Ruff formatting check failed. 2 files would be reformatted.
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (1)
19-19: The docstring type doesn't match the actual type annotation on line 28.The docstring indicates
string = Nonebut the implementation usesUnion[InterpolatedString, str] = None. These should match for clarity and consistency.- key_transformation: string = None how to transform extracted object keys + key_transformation: Optional[Union[InterpolatedString, str]] = None how to transform extracted object keysWhat do you think about using the proper type annotation in the docstring?
📜 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(1 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(1 hunks)airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py(4 hunks)unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py(13 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py (2)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (1)
DpathFlattenFields(12-72)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
DpathFlattenFields(882-908)
🪛 GitHub Actions: Linters
unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py
[error] Ruff formatting check failed. 2 files would be reformatted.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
[error] 800-800: Argument 'key_transformation' to 'DpathFlattenFields' has incompatible type 'str | None'; expected 'InterpolatedString | str'
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py
[error] 28-28: Incompatible types in assignment (expression has type 'None', variable has type 'InterpolatedString | str')
[error] 39-39: 'DpathFlattenFields' has no attribute 'parameters'
[error] 60-60: 'DpathFlattenFields' has no attribute 'parameters'
⏰ 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: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (8)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
900-907: Looks good! The newkey_transformationfield is well-documented.The addition of the
key_transformationfield to theDpathFlattenFieldsclass will enable customizing the keys of extracted objects, which directly addresses the need mentioned in the PR objective for supporting the source-hubspot integration. The field is properly typed asOptional[str]with a clear description and example.This will be useful for transforming field names during the flattening process, especially when migrating integrations to the low-code approach. wdyt?
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
2338-2344: This new addition looks great! Enabling customization of flattened field keys.The
key_transformationproperty is a nice enhancement to theDpathFlattenFieldscomponent, allowing users to apply custom transformations to object keys. The example with"flattened_{{ key }}"makes the intended usage clear. This feature will be particularly useful for the source-hubspot integration migration mentioned in the PR objectives.unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py (6)
10-10: Good constant name for the default value.The constant name
_NO_KEY_TRANSFORMATIONSclearly expresses the absence of a transformation, and usingNoneas the value aligns with the implementation.🧰 Tools
🪛 GitHub Actions: Linters
[error] Ruff formatting check failed. 2 files would be reformatted.
19-19: Perfect test parameterization!Adding the
key_transformationparameter to the test parameterization allows for comprehensive testing of the new feature.🧰 Tools
🪛 GitHub Actions: Linters
[error] Ruff formatting check failed. 2 files would be reformatted.
29-29: Good backward compatibility testing approach.Adding
_NO_KEY_TRANSFORMATIONSto existing test cases ensures the new parameter doesn't break existing functionality.Also applies to: 39-39, 52-52, 69-69, 79-79, 89-89, 99-99, 109-109, 119-119, 129-129, 139-139
🧰 Tools
🪛 GitHub Actions: Linters
[error] Ruff formatting check failed. 2 files would be reformatted.
143-172: Great test coverage for the new functionality!These three new test cases comprehensively cover the key transformation feature with different combinations of the
delete_origin_valueandreplace_recordparameters:
- Transformation without deleting origin value
- Transformation with deleted origin value
- Transformation with record replacement
The expected results accurately reflect how the transformed keys should appear in the output.
🧰 Tools
🪛 GitHub Actions: Linters
[error] Ruff formatting check failed. 2 files would be reformatted.
176-176: Function signature properly updated.The test function signature has been updated to include the new parameter.
🧰 Tools
🪛 GitHub Actions: Linters
[error] Ruff formatting check failed. 2 files would be reformatted.
184-184: Constructor call properly updated.The
DpathFlattenFieldsconstructor now includes thekey_transformationparameter.🧰 Tools
🪛 GitHub Actions: Linters
[error] Ruff formatting check failed. 2 files would be reformatted.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py (1)
144-173: New test cases for key transformation featureGreat addition of test cases covering the key transformation feature! You've effectively covered three important scenarios:
- Transformation without deleting origin value
- Transformation with origin value deletion
- Transformation with record replacement
The expected records look correct, showing
field3transformed tofield2_field3as per the template string.Would it be worth adding a test case with a more complex transformation template or with nested objects that have multiple keys to transform? Just a thought - wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py(3 hunks)unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py (2)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (1)
DpathFlattenFields(12-73)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
DpathFlattenFields(882-908)
⏰ 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: Analyze (python)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
🔇 Additional comments (5)
unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py (5)
10-10: New constant for key transformationsThe addition of the
_NO_KEY_TRANSFORMATIONSconstant provides a clear way to specify when no key transformation should be applied. This aligns well with the other constants in the file. Nice approach to maintain readability and consistency!
20-20: Added key_transformation parameter to test parameterizationGood addition of the
key_transformationparameter to the test parameterization list. This allows testing different transformation scenarios effectively.
30-30: Added _NO_KEY_TRANSFORMATIONS to existing test casesI see you've added the
_NO_KEY_TRANSFORMATIONSvalue to all existing test cases, ensuring they continue to function as before with the new parameter in place. This is a good approach to maintain backward compatibility.Also applies to: 40-40, 53-53, 70-70, 80-80, 90-90, 100-100, 110-110, 120-120, 130-130, 140-140
182-182: Added key_transformation parameter to function signatureGood addition of the
key_transformationparameter to the test function signature, maintaining consistency with the parametrize decorator.
191-191: Added key_transformation to DpathFlattenFields constructorThe
key_transformationparameter is now properly passed to theDpathFlattenFieldsconstructor, ensuring that the test can verify the transformation functionality.
There was a problem hiding this comment.
Mainly just one comment around how we can try to move away from some jinja interpolation magic that the user has to understand, but this is a good feature for the CDK and I image other connectors might use this so this makes sense to have here instead on a hubspot custom component
airbyte_cdk/sources/declarative/models/declarative_component_schema.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
2310-2337: New Definition "KeyTransformation" Added
This block introduces a well-structured definition for key transformations that can be applied within the DpathFlattenFields component. The inclusion of properties forprefix,suffix, andcustomprovides users flexible options to modify keys during the flattening process. The use of examples is clear and instructive. wdyt?airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
882-907: The KeyTransformation class looks good, but should it support interpolation like the one in dpath_flatten_fields.py?I see you've implemented the new
KeyTransformationclass with a nice clean structure. However, I noticed that there's a similar class inairbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.pythat usesUnion[InterpolatedString, str, None]as field types instead ofOptional[Union[str, None]].For consistency, should these two classes have the same type handling approach? Also,
Optional[Union[str, None]]is a bit redundant sinceOptional[str]would be equivalent. Not a big deal though, wdyt?airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (1)
48-71: Well-structured key transformation methodThe implementation of the key transformation logic is clean and follows a good separation of concerns. However, I notice that prefix, suffix, and custom transformations are applied sequentially. This might lead to unexpected behavior if multiple transformations are specified.
For example, if both prefix and suffix are specified, the prefix is applied first, then the suffix is applied to already-prefixed keys. If custom is then specified, it's applied to keys that already have both prefix and suffix.
Is this the intended behavior? Would it make more sense to apply custom transformation as an alternative to prefix/suffix rather than in addition to them? Or perhaps document this sequential application clearly? 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(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/dpath_flatten_fields.py(4 hunks)unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py(13 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
KeyTransformation(882-906)airbyte_cdk/sources/declarative/interpolation/interpolated_string.py (1)
InterpolatedString(13-79)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (1)
KeyTransformation(12-15)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (2)
KeyTransformation(12-15)DpathFlattenFields(19-97)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
KeyTransformation(882-906)DpathFlattenFields(909-932)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (16)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
2365-2370: Integration of "key_transformation" in DpathFlattenFields
The newkey_transformationproperty correctly references the "KeyTransformation" definition using the$refpointer. This change enhances the component by enabling custom key modifications as part of the flattening process. Everything appears consistent and clear. Would you consider adding a note in the docs about its default behavior (i.e. using original keys if not provided)? wdyt?airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
927-931: The integration of key_transformation into DpathFlattenFields looks solid!This looks like a well-structured addition to the
DpathFlattenFieldsclass. It properly aligns with the PR objectives of enabling key customization during flattening, which will help with migrating the hubspot integration to a low-code approach.I see from the past review comments that there was a discussion about potentially using both
key_prefix/key_suffixandkey_transformationapproaches. The implementation chosen here with the fullKeyTransformationobject is more flexible - good choice.airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
501-501: Import looks good!Clean addition of the KeyTransformation import which is used in the create_dpath_flatten_fields method.
794-802: Thoughtful implementation of KeyTransformation handling!I like how you've carefully handled the case where model.key_transformation might be None, creating a KeyTransformation object only when needed.
810-810: Good parameter passing!Properly passes the key_transformation parameter to the DpathFlattenFields constructor.
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (5)
11-16: Nice clean dataclass for KeyTransformation!Good choice making the dataclass frozen (immutable). The types look appropriate for supporting both string literals and InterpolatedString objects.
26-26: Good docstring updateYou've documented the new parameter appropriately.
35-35: Proper type annotationYour Union type annotation correctly reflects that the parameter can be either a KeyTransformation object or None.
38-38: Fixed parameter storageStoring parameters in
self._parametersresolves the previous issues with accessing parameters.
87-88: Good integration of key transformationCleanly integrates the key transformation into the existing transform method.
unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py (6)
3-6: Good import additionClean import of the KeyTransformation class.
13-16: Helpful constants for testsThese constants make the test cases more readable and maintainable.
26-26: Good test parameter additionAdded key_transformation to the test parameters appropriately.
150-219: Comprehensive test cases for new functionality!I really like how thoroughly you've tested the key transformation functionality. Your tests cover:
- Custom transformations with and without origin deletion
- Prefix transformations
- Suffix transformations
- Combined prefix and suffix
- All transformations together with a string operation
These test cases will ensure the feature works as expected and help prevent future regressions.
231-233: Clean test setup for KeyTransformationGood conditional creation of the KeyTransformation object from the test parameters.
240-240: Proper test constructionCorrectly passes the key_transformation to the DpathFlattenFields constructor.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (1)
30-30: Missing Optional in class docstring.The docstring parameter description for
key_transformationshould indicate it's optional to match the implementation.- key_transformation: KeyTransformation = None how to transform extracted object keys + key_transformation: Optional[KeyTransformation] = None how to transform extracted object keys
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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/dpath_flatten_fields.py(4 hunks)unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- airbyte_cdk/sources/declarative/declarative_component_schema.yaml
- airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (1)
KeyTransformation(12-19)
unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py (2)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (2)
DpathFlattenFields(23-89)KeyTransformation(12-19)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
DpathFlattenFields(901-924)KeyTransformation(882-898)
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py
[error] 18-19: Argument 1 to 'create' of 'InterpolatedString' has incompatible type 'InterpolatedString | str | None'; expected 'InterpolatedString | str' [arg-type]
[error] 55-59: Item 'str' of 'InterpolatedString | str' has no attribute 'eval' [union-attr]
⏰ 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 (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: SDM Docker Image Build
🔇 Additional comments (6)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
882-898: Nice addition of the KeyTransformation class!The implementation looks clean with good documentation. The examples provided are helpful for users to understand how to use the prefix and suffix options.
919-923: Good integration of key_transformation into DpathFlattenFields.The field definition clearly indicates that it's optional and includes a helpful description of its purpose.
unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py (4)
3-6: Updated imports to include KeyTransformation.Clean update to import the new KeyTransformation class.
13-15: Good addition of constants for key transformation testing.These constants make the test cases easier to read and understand.
149-178: Comprehensive test cases for key transformations.I like how you've covered all the combinations of prefix, suffix, and both together. The test cases are well structured and provide good coverage of the new functionality.
190-192: Good handling of KeyTransformation instantiation in tests.This creates the KeyTransformation object with the correct parameters for testing.
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py
Outdated
Show resolved
Hide resolved
|
/auto-fix |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (1)
1-91: Fix needed for formatting issue.The pipeline failure indicates a formatting issue. Could you run 'poetry run ruff format' to fix the code style issues in this file?
🧰 Tools
🪛 GitHub Actions: Linters
[error] 1-1: Ruff formatting check failed. 1 file would be reformatted. Please run 'poetry run ruff format' to fix code style issues in this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(2 hunks)airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py(4 hunks)unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py(13 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py (2)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (2)
DpathFlattenFields(26-90)KeyTransformation(12-22)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
DpathFlattenFields(901-924)KeyTransformation(882-898)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (2)
KeyTransformation(12-22)DpathFlattenFields(26-90)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
KeyTransformation(882-898)DpathFlattenFields(901-924)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (17)
KeyTransformation(882-898)Config(132-133)Config(146-147)Config(160-161)Config(174-175)Config(192-193)Config(206-207)Config(220-221)Config(234-235)Config(248-249)Config(262-263)Config(276-277)Config(290-291)Config(306-307)Config(320-321)Config(334-335)Config(368-369)airbyte_cdk/sources/declarative/interpolation/interpolated_string.py (1)
InterpolatedString(13-79)
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py
[error] 1-1: Ruff formatting check failed. 1 file would be reformatted. Please run 'poetry run ruff format' to fix code style issues in this file.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (14)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
500-502: Looks good with the KeyTransformation import.Adding import for the KeyTransformation class which will be used in the create_dpath_flatten_fields method.
794-803: Good implementation of key_transformation initialization.The conditional creation of KeyTransformation is well-implemented, ensuring that it's only created when model.key_transformation is not None. This handles the optional nature of the parameter correctly.
811-812: Proper parameter passing to DpathFlattenFields.The key_transformation parameter is correctly passed to the DpathFlattenFields constructor.
unit_tests/sources/declarative/transformations/test_dpath_flatten_fields.py (5)
3-6: Good addition of KeyTransformation import.Importing KeyTransformation from the module for use in the tests.
13-15: Well-defined constants for key transformation tests.Adding constants for key transformations is a good approach for maintaining test readability.
25-36: Properly updated parametrize decorator with key_transformation parameter.The test parametrization has been updated to include the key_transformation parameter, and _NO_KEY_TRANSFORMATIONS is correctly set for existing tests.
149-178: Good coverage of key transformation test cases.The additional test cases cover the important scenarios:
- Prefix-only transformation
- Suffix-only transformation
- Both prefix and suffix transformation
This ensures comprehensive testing of the new functionality.
187-200: Well-implemented test function modifications.The test function is properly updated to handle the key_transformation parameter, conditionally creating a KeyTransformation object when needed.
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (6)
11-23: Good implementation of KeyTransformation class.The KeyTransformation class is well-designed with appropriate handling of prefix and suffix. The post_init method efficiently evaluates interpolated strings at initialization time rather than during each transform operation.
33-34: Updated docstring to include key_transformation.The docstring has been updated to document the new key_transformation parameter.
42-43: Proper type annotation for key_transformation parameter.The key_transformation parameter is correctly annotated as Optional[KeyTransformation].
44-53: Updated parameter handling in post_init.The parameters are now stored in self._parameters which is a better practice for internal instance variables.
55-64: Well-implemented key transformation method.The _apply_key_transformation method correctly applies prefix and suffix transformations to the keys in the extracted dictionary.
79-81: Properly integrated key transformation in transform method.The _apply_key_transformation method is called at the right point in the transform process, before updating the record.
|
/auto-fix |
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (4)
11-28: The KeyTransformation class evaluates interpolated strings during initialization rather than at transformation time.The
__post_init__method immediately evaluates bothprefixandsuffixwith the config at initialization time (lines 19-26). This means any dynamic values in the interpolated strings will only be evaluated once when the class is initialized, not each time a transformation is applied. Is this the intended behavior?If these values should be dynamic during the transformation process, would it make more sense to delay this evaluation until the transformation is actually applied? wdyt?
def __post_init__(self, parameters: Mapping[str, Any]) -> None: if self.prefix is not None: - self.prefix = InterpolatedString.create(self.prefix, parameters=parameters).eval( - self.config - ) + self.prefix = InterpolatedString.create(self.prefix, parameters=parameters) if self.suffix is not None: - self.suffix = InterpolatedString.create(self.suffix, parameters=parameters).eval( - self.config - ) + self.suffix = InterpolatedString.create(self.suffix, parameters=parameters)Then, in
_apply_key_transformation:def _apply_key_transformation(self, extracted: Mapping[str, Any]) -> Mapping[str, Any]: if self.key_transformation: if self.key_transformation.prefix: + prefix = self.key_transformation.prefix.eval(self.config) + if prefix: extracted = { - f"{self.key_transformation.prefix}{key}": value + f"{prefix}{key}": value for key, value in extracted.items() }(Similar change for suffix)
59-73: The key transformation method could be optimized to reduce dictionary recreations.The current implementation creates a new dictionary for each transformation type (prefix and suffix). For large dictionaries, this could impact performance.
Would it be more efficient to apply both transformations in a single pass? Also, could we add a check to avoid unnecessary transformations when either prefix or suffix evaluates to an empty string? wdyt?
def _apply_key_transformation(self, extracted: Mapping[str, Any]) -> Mapping[str, Any]: if self.key_transformation: + has_prefix = bool(self.key_transformation.prefix) + has_suffix = bool(self.key_transformation.suffix) + + if has_prefix or has_suffix: + updated_extracted = {} + for key, value in extracted.items(): + new_key = key + if has_prefix: + new_key = f"{self.key_transformation.prefix}{new_key}" + if has_suffix: + new_key = f"{new_key}{self.key_transformation.suffix}" + updated_extracted[new_key] = value + return updated_extracted - if self.key_transformation.prefix: - extracted = { - f"{self.key_transformation.prefix}{key}": value - for key, value in extracted.items() - } - - if self.key_transformation.suffix: - extracted = { - f"{key}{self.key_transformation.suffix}": value - for key, value in extracted.items() - } return extracted
46-46: Should the default value be None or Field.default_factory?I notice that
key_transformationhas a default value ofNonehere, which means it won't be initialized until used. This is correct, but for consistency with the implementation, should we consider making this more explicit?- key_transformation: Optional[KeyTransformation] = None + key_transformation: Optional[KeyTransformation] = None # Default is None to skip transformation
89-91: Consider adding logging for key transformations.For debugging purposes, it might be helpful to add some logging when key transformations are applied, especially since this is a new feature.
if isinstance(extracted, dict): + original_keys = set(extracted.keys()) extracted = self._apply_key_transformation(extracted) + transformed_keys = set(extracted.keys()) + if original_keys != transformed_keys: + # Using a logger or similar mechanism + print(f"Applied key transformation: {original_keys} -> {transformed_keys}")What do you think about adding some form of logging? This could help with troubleshooting any issues that might arise with the new feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (17)
KeyTransformation(882-898)Config(132-133)Config(146-147)Config(160-161)Config(174-175)Config(192-193)Config(206-207)Config(220-221)Config(234-235)Config(248-249)Config(262-263)Config(276-277)Config(290-291)Config(306-307)Config(320-321)Config(334-335)Config(368-369)airbyte_cdk/sources/declarative/interpolation/interpolated_string.py (1)
InterpolatedString(13-79)
⏰ 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: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Analyze (python)
airbyte_cdk/sources/declarative/transformations/dpath_flatten_fields.py
Outdated
Show resolved
Hide resolved
darynaishchenko
deleted the
daryna/low-code/update-flatten-fields-transformation
branch
What
neede for source-hubspot migration to low code.
We already have
DpathFlattenFieldsto extract object fields, but with current implementation we can't customize extracted object keys.Current implementation in hubspot with RecordUnnester.
How
added
key_transformationoption toDpathFlattenFields.Example:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests