feat(low-code): added keys replace transformation #183
Conversation
|
/autofix
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Stream as DeclarativeStream
participant Factory as ModelToComponentFactory
participant Transformation as KeysReplaceTransformation
Stream->>Factory: Request transformation
Factory-->>Transformation: Create KeysReplaceTransformation
Stream->>Transformation: Apply transform
Transformation->>Transformation: Replace keys in record
Possibly Related PRs
Suggested Reviewers
Hey team! 👋 Quick question - would you be interested in seeing some example use cases for the new 📜 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 (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
718-733: LGTM! Consider adding value validation, wdyt?The
KeysReplaceschema is well-structured and properly integrated. The fields have clear descriptions and examples.One suggestion: Consider adding validation to ensure
oldandnewvalues are not empty strings, as empty replacements could lead to unexpected behavior. Something like:old: str = Field( ..., description="Old value to replace.", examples=[" ", "_"], title="Old value", min_length=1 )unit_tests/sources/declarative/transformations/test_keys_replace_transformation.py (2)
9-10: Consider using a more descriptive constant nameThe constant
_ANY_VALUE = -1could be more descriptive. What about something like_DUMMY_VALUEor_PLACEHOLDER_VALUE, wdyt?-_ANY_VALUE = -1 +_DUMMY_VALUE = -1 # Used as a placeholder in test records
12-15: Consider adding more test cases for better coverageThe current test only covers a basic case. Would you consider adding tests for:
- Empty strings in keys
- Multiple occurrences of the pattern
- Nested dictionaries
- Special characters in keys
- Edge cases where no replacement is needed
Here's a suggested expansion of the test cases:
def test_transform(): test_cases = [ # Basic case ({"date time": _DUMMY_VALUE}, {"date_time": _DUMMY_VALUE}), # Multiple spaces ({"date time": _DUMMY_VALUE}, {"date__time": _DUMMY_VALUE}), # Nested dictionary ({"outer space": {"inner space": _DUMMY_VALUE}}, {"outer_space": {"inner_space": _DUMMY_VALUE}}), # No replacement needed ({"date_time": _DUMMY_VALUE}, {"date_time": _DUMMY_VALUE}), # Empty string ({"": _DUMMY_VALUE}, {"": _DUMMY_VALUE}), ] for input_record, expected_output in test_cases: record = input_record.copy() KeysReplaceTransformation(old=" ", new="_").transform(record) assert record == expected_outputairbyte_cdk/sources/declarative/transformations/keys_replace_transformation.py (1)
12-16: Consider adding docstring and input validationThe class could benefit from documentation and parameter validation. What do you think about:
@dataclass class KeysReplaceTransformation(RecordTransformation): + """Transforms record keys by replacing occurrences of a substring with another. + + Args: + old: The substring to replace in record keys + new: The substring to replace with + + Example: + >>> transform = KeysReplaceTransformation(old=" ", new="_") + >>> record = {"date time": 123} + >>> transform.transform(record) + >>> assert record == {"date_time": 123} + """ old: str new: str + + def __post_init__(self): + if not isinstance(self.old, str) or not isinstance(self.new, str): + raise ValueError("Both 'old' and 'new' must be strings")airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1883-1911: LGTM! The KeysReplace transformation component looks well-structured.The component definition is clear and includes all necessary fields. I particularly like the descriptive examples provided for both
oldandnewvalues. Just a minor suggestion - would you consider adding a more complex example showing how this could be used with config parameters? Something like:old: "{{ config['old_key_pattern'] }}" new: "{{ config['new_key_pattern'] }}"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(3 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(3 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(4 hunks)airbyte_cdk/sources/declarative/transformations/keys_replace_transformation.py(1 hunks)unit_tests/sources/declarative/transformations/test_keys_replace_transformation.py(1 hunks)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
1692-1692: LGTM! KeysReplace properly integrated
The KeysReplace transformation is correctly added to both transformation lists in DeclarativeStream and DynamicSchemaLoader.
Also applies to: 1867-1867
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
239-241: LGTM! The factory implementation looks good
The changes to ModelToComponentFactory follow the established patterns and are consistent with other transformation implementations.
Also applies to: 497-497, 619-623
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1239-1239: LGTM! The KeysReplace transformation is properly integrated.
The component is correctly added to both transformation lists in DeclarativeStream and DynamicSchemaLoader. The integration maintains schema consistency and allows the transformation to be used in both contexts.
Also applies to: 1784-1784
airbyte_cdk/sources/declarative/transformations/keys_replace_transformation.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
619-623: Consider adding validation for the old/new parameters.The implementation looks good, but what do you think about adding validation to ensure that
oldandnewparameters are not empty strings or None? This could help catch configuration issues early. wdyt?def create_keys_replace_transformation( self, model: KeysReplaceModel, config: Config, **kwargs: Any ) -> KeysReplaceTransformation: + if not model.old or not model.new: + raise ValueError("Both 'old' and 'new' parameters must be non-empty strings") return KeysReplaceTransformation(old=model.old, new=model.new)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(3 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte_cdk/sources/declarative/declarative_component_schema.yaml
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
239-241: LGTM! Clean import additions.
The imports are well-organized and follow the existing pattern.
Also applies to: 405-407
497-497: LGTM! Clean mapping addition.
The mapping is correctly placed in alphabetical order.
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/transformations/keys_replace_transformation.py (1)
35-61: Consider adding error handling for non-string keysThe transformation assumes all dictionary keys are strings. What do you think about adding error handling for non-string keys? Something like this maybe?
def _transform(data: Dict[str, Any]) -> Dict[str, Any]: result = {} for key, value in data.items(): + if not isinstance(key, str): + result[key] = value + continue updated_key = key.replace(old_key, new_key) if isinstance(value, dict): result[updated_key] = _transform(value) else: result[updated_key] = value return resultairbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1888-1930: The KeysReplace transformation definition looks good, but should we enhance the examples?The component is well-defined with clear descriptions and proper type constraints. However, we could make the examples more comprehensive to better illustrate common use cases. What do you think about adding examples that show:
- Replacing spaces with underscores in all keys
- Using config values for dynamic replacements
- Using record values for conditional replacements
Here's a suggested enhancement to the examples:
examples: - - " " - - "{{ record.id }}" - - "{{ config['id'] }}" - - "{{ stream_slice['id'] }}" + - " " # Replace spaces with underscores in keys + - "." # Replace dots with underscores in keys + - "{{ config['old_prefix'] }}" # Replace dynamic prefix from config + - "{{ record.old_key_pattern }}" # Replace pattern from record interpolation_context: - config - record - stream_state - stream_slice new: type: string title: New value description: New value to set. examples: - - "_" - - "{{ record.id }}" - - "{{ config['id'] }}" - - "{{ stream_slice['id'] }}" + - "_" # Convert spaces to underscores + - "_" # Convert dots to underscores + - "{{ config['new_prefix'] }}" # Use new prefix from config + - "{{ record.new_key_pattern }}" # Use new pattern from record
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(3 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(3 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(4 hunks)airbyte_cdk/sources/declarative/transformations/keys_replace_transformation.py(1 hunks)unit_tests/sources/declarative/transformations/test_keys_replace_transformation.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- unit_tests/sources/declarative/transformations/test_keys_replace_transformation.py
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/transformations/keys_replace_transformation.py
[error] 31-31: Function is missing a type annotation for one or more arguments
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (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)
🔇 Additional comments (8)
airbyte_cdk/sources/declarative/transformations/keys_replace_transformation.py (2)
1-10: LGTM! Clean imports and proper license header.The imports are well-organized and include only what's needed.
13-25: Great docstring with clear example!The docstring effectively illustrates the transformation with a practical example. The before/after demonstration makes it very clear how the transformation works.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
724-739: LGTM! Well-structured model definitionThe
KeysReplacemodel follows the established patterns with:
- Clear field descriptions and examples
- Proper type constraints
- Optional parameters field
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
257-259: LGTM! Clean import additionThe import follows the established pattern of the file.
518-518: LGTM! Proper mapping registrationThe
KeysReplaceModelis correctly mapped to its factory method.
640-646: LGTM! Factory method implementationThe factory method follows the established pattern and correctly passes all required parameters.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)
1244-1244: LGTM! The KeysReplace transformation is properly added to the DeclarativeStream's transformations list.The addition follows the same pattern as other transformations in the list.
1789-1789: LGTM! The KeysReplace transformation is properly added to the DynamicSchemaLoader's schema_transformations list.The addition follows the same pattern as other transformations in the list.
airbyte_cdk/sources/declarative/transformations/keys_replace_transformation.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/transformations/keys_replace_transformation.py
Outdated
Show resolved
Hide resolved
|
Changes makes sense to me, just left annoying nit comment, |
* main: fix(airbyte-cdk): unable to create custom retriever (airbytehq#198) feat(low-code): added keys replace transformation (airbytehq#183) feat: add `min` macros (airbytehq#203) fix(low-code cdk pagination): Fix the offset strategy so that it resets back to 0 when a stream is an incremental data feed (airbytehq#202)
Added
KeysReplaceTransformationsto be able to replace symbols in record keys.It's used for connectors where we need to transform record fields to replace symbols in it. For example
source-airtablewe need to replace spaces to underscores: fromuser idtouser_id, for example.Summary by CodeRabbit
New Features
KeysReplace, for replacing symbols in record keys.DeclarativeStreamto include theKeysReplacetransformation.KeysReplaceTransformation, for processing key replacements in records.Tests
KeysReplaceTransformationto validate its functionality.