feat(low-code): add items mappings to dynamic schemas #256
Conversation
lazebnyi
changed the title
|
/autofix
|
lazebnyi
changed the title
📝 WalkthroughWalkthroughThis pull request introduces a new definition, Changes
Sequence DiagramsequenceDiagram
participant SchemaLoader as Dynamic Schema Loader
participant TypeMapper as Types Mapper
participant ComponentFactory as Model to Component Factory
SchemaLoader->>TypeMapper: Request Type Mapping
TypeMapper-->>SchemaLoader: Return Mapped Types
SchemaLoader->>ComponentFactory: Create Component with Mapped Types
ComponentFactory-->>SchemaLoader: Return Configured Component
Possibly related PRs
Suggested reviewers
Hey! 🌟 Do you think the introduction of ✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (1)
291-291: Remove debugThere's a
print(additional_types)statement that seems intended for debugging. Should we remove it to avoid unintended console output? WDYT?Apply this diff:
- print(additional_types)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
2007-2007: Question aboutproperty_nameinItemsTypeMapIn the
ItemsTypeMapclass, there's an optionalproperty_namefield. Since this class represents item type mappings, isproperty_nameneeded here, or could it be removed for clarity? WDYT?airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
1907-1919: Consider adding type hints for better code clarity.The implementation of
create_property_types_maplooks good, but would you consider adding type hints for the local variables to improve code clarity? For example:def create_property_types_map( self, model: PropertyTypesMapModel, config: Config, **kwargs: Any ) -> PropertyTypesMap: - type_mapping = self._create_component_from_model(model=model.type_mapping, config=config) + type_mapping: TypesMap = self._create_component_from_model(model=model.type_mapping, config=config) - model_property_type_pointer: List[Union[InterpolatedString, str]] = ( + model_property_type_pointer: List[Union[InterpolatedString, str]] = ( [x for x in model.property_type_pointer] if model.property_type_pointer else [] )
1920-1928: Consider adding type hints for better code clarity.Similar to the previous comment, would you consider adding type hints in the
create_items_type_mapmethod? The implementation looks good otherwise.def create_items_type_map( self, model: ItemsTypeMapModel, config: Config, **kwargs: Any ) -> ItemsTypeMap: - type_mapping = self._create_component_from_model(model=model.type_mapping, config=config) + type_mapping: TypesMap = self._create_component_from_model(model=model.type_mapping, config=config) - model_items_type_pointer: List[Union[InterpolatedString, str]] = ( + model_items_type_pointer: List[Union[InterpolatedString, str]] = ( [x for x in model.items_type_pointer] if model.items_type_pointer else [] )airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1865-1870: Consider enhancing documentation for new fields.The new fields
items_typeandproperties_typescould benefit from more detailed descriptions. What do you think about adding:items_type: + title: Items Type Mapping + description: Mapping configuration for array item types. Used when the field being mapped is an array. "$ref": "#/definitions/ItemsTypeMap" properties_types: + title: Properties Type Mappings + description: List of mapping configurations for object property types. Used when the field being mapped is an object with properties that need type mapping. type: array items: - "$ref": "#/definitions/PropertyTypesMap"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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(5 hunks)airbyte_cdk/sources/declarative/schema/__init__.py(2 hunks)airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py(7 hunks)unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- airbyte_cdk/sources/declarative/schema/init.py
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
[error] 84-84: Function is missing a return type annotation. Use '-> None' if function does not return a value
[error] 250-250: Missing type parameters for generic type 'List'
[error] 286-286: Missing type parameters for generic type 'List'
[error] 301-301: Unsupported target for indexed assignment ('Mapping[str, Any]')
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
[error] 1949-1949: Argument 'items_type' to 'TypesMap' has incompatible type 'Any | airbyte_cdk.sources.declarative.models.declarative_component_schema.ItemsTypeMap | None'
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
247-249: LGTM! The imports are well-organized.The new imports for
ItemsTypeMapandPropertyTypesMapare properly added in both the model imports and the schema imports sections, maintaining consistency with the existing code structure.Also applies to: 316-318, 441-443
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
Outdated
Show resolved
Hide resolved
unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/declarative_component_schema.yaml
Outdated
Show resolved
Hide resolved
airbyte_cdk/sources/declarative/declarative_component_schema.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (1)
Line range hint
227-282: Consider using a more specific type foradditional_typesThe implementation looks good, but would you consider making the type annotation more specific? WDYT?
- ) -> Tuple[Union[List[str], str], List]: + ) -> Tuple[Union[List[str], str], List[str]]:🧰 Tools
🪛 GitHub Actions: Linters
[error] 279-279: List item 0 has incompatible type 'list[str] | str'; expected 'str'
[error] 286-286: Missing type parameters for generic type 'List'
[error] 300-300: Unsupported target for indexed assignment ('Mapping[str, Any]')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(5 hunks)airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py(7 hunks)unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte_cdk/sources/declarative/declarative_component_schema.yaml
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
[error] 84-84: Function is missing a return type annotation. Use '-> None' if function does not return a value
[error] 250-250: Missing type parameters for generic type 'List'
[error] 279-279: List item 0 has incompatible type 'list[str] | str'; expected 'str'
[error] 286-286: Missing type parameters for generic type 'List'
[error] 300-300: Unsupported target for indexed assignment ('Mapping[str, Any]')
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (10)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (5)
48-58: LGTM! The experimental class definition looks good.The
PropertyTypesMapclass is well-documented and properly marked as experimental with appropriate deprecation warning.
60-69: LGTM! The experimental class definition looks good.The
ItemsTypeMapclass is well-documented and properly marked as experimental with appropriate deprecation warning.
84-84:⚠️ Potential issueAdd return type annotation to
__post_init__Would you consider adding
-> Noneas the return type annotation to comply with type checking requirements? WDYT?- def __post_init__(self): + def __post_init__(self) -> None:Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: Linters
[error] 84-84: Function is missing a return type annotation. Use '-> None' if function does not return a value
285-287:⚠️ Potential issueAdd type parameters to the
ListannotationWould you consider adding type parameters to the
Listannotation in the method signature? WDYT?- field_type: str, additional_types: Optional[List] = None + field_type: str, additional_types: Optional[List[str]] = NoneLikely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: Linters
[error] 286-286: Missing type parameters for generic type 'List'
297-306:⚠️ Potential issueUse MutableMapping for dictionary assignment
The code attempts to modify
airbyte_typewhich is typed asMapping, but we need a mutable type for item assignment. Would you consider this change? WDYT?- airbyte_type = deepcopy(AIRBYTE_DATA_TYPES[field_type]) + airbyte_type: MutableMapping[str, Any] = deepcopy(AIRBYTE_DATA_TYPES[field_type])Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: Linters
[error] 300-300: Unsupported target for indexed assignment ('Mapping[str, Any]')
unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py (2)
87-121: LGTM! Comprehensive test coverage for type mappings.The test configuration covers both simple and complex type mappings, including array and object types with nested properties.
403-408: LGTM! Good test case for custom integer type mapping.The test case properly validates the conversion of custom integer types in the schema.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
1907-1918: LGTM! Well-implemented factory method for PropertyTypesMap.The implementation correctly handles optional property_type_pointer and creates the component with all required fields.
1920-1927: LGTM! Well-implemented factory method for ItemsTypeMap.The implementation correctly handles optional items_type_pointer and creates the component with all required fields.
1929-1950: LGTM! Updated TypesMap creation with new fields.The implementation properly handles the new optional fields items_type and properties_types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py (2)
87-104: LGTM! Consider improving readability with comments?The types_mapping configuration effectively demonstrates both simple and complex type mappings, particularly the new array type mapping with nested items_type. Would you consider adding comments to explain the purpose of each mapping type for better maintainability? WDYT?
types_mapping: [ + # Simple string type mapping { "target_type": "string", "current_type": "singleLineText" }, + # Array type mapping with nested integer items { "target_type": "array", "current_type": "formula", "items_type": {🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
Line range hint
1-391: Fix formatting issues.The linter reported formatting issues. Could you run
ruff formatto fix them? This will ensure consistent code style across the codebase.🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
2010-2018: LGTM! Consider adding field descriptions?The ItemsTypeMap class is well-structured for handling nested type mappings. Would you consider adding descriptions for each field to improve the API documentation? For example:
class ItemsTypeMap(BaseModel): - property_name: Optional[str] = None + property_name: Optional[str] = Field( + None, + description="Optional name of the property containing the items type information.", + title="Property Name", + ) items_type_pointer: List[str] = Field( ..., description="List of potentially nested fields describing the full path of the items type to extract.", title="Items Type Path", ) type_mapping: TypesMap🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
2027-2045: Consider consistent default value handling?The SchemaTypeIdentifier class looks great! However, I noticed a small inconsistency in default value handling. The schema_pointer uses an empty list as default via Field(..., default=[]), while type_pointer uses None. Would you consider making this consistent? WDYT?
type_pointer: Optional[List[str]] = Field( - None, + [], description="List of potentially nested fields describing the full path of the field type to extract.", title="Type Path", )🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
1902-1909: Consider adding error handling for type_mapping.The implementation looks good! However, what do you think about adding validation to ensure
type_mappingis not None before creating the component? This would prevent potential runtime errors and provide clearer error messages. wdyt?def create_items_type_map( self, model: ItemsTypeMapModel, config: Config, **kwargs: Any ) -> ItemsTypeMap: + if not model.type_mapping: + raise ValueError("type_mapping is required for ItemsTypeMap") type_mapping = self._create_component_from_model(model=model.type_mapping, config=config) model_items_type_pointer: List[Union[InterpolatedString, str]] = ( [x for x in model.items_type_pointer] if model.items_type_pointer else [] ) return ItemsTypeMap(items_type_pointer=model_items_type_pointer, type_mapping=type_mapping)airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1803-1819: Consider improving the component description and field names for clarity.The description mentions "property" but this component is about array items. Also, the field name
property_nameseems unrelated to items mapping. Here are a few suggestions to improve consistency:
- Update the description to focus on array items:
- description: (This component is experimental. Use at your own risk.) Represents a mapping between a current type and its corresponding target type for property. + description: (This component is experimental. Use at your own risk.) Represents a mapping between a current type and its corresponding target type for array items.
- Consider renaming or removing the
property_namefield since it seems unrelated to items mapping, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(11 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(4 hunks)airbyte_cdk/sources/declarative/schema/__init__.py(2 hunks)airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py(7 hunks)unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- airbyte_cdk/sources/declarative/schema/init.py
- airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
🧰 Additional context used
🪛 GitHub Actions: Linters
unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL
- 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)
unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py (2)
344-344: LGTM! Well-structured test case.The test case effectively validates the new items_type mapping functionality by expecting an array of integers for the avg_salary field, which aligns perfectly with the types_mapping configuration.
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
386-391: LGTM! Comprehensive test data.The test data effectively sets up the scenario for testing the new array type mapping with nested items_type by providing a formula field with customInteger result type.
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
2020-2024: LGTM! Clean integration of the new field.The TypesMap class cleanly integrates the new items_type field while maintaining backward compatibility. The use of Optional[ItemsTypeMap] is appropriate for the nested type mapping feature.
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
247-249: LGTM! Clean import addition.The new import follows the existing pattern and is properly organized within the import block.
1911-1922: Fix the type error as suggested in the previous review.Based on the past review comments, there's a type error in the pipeline failures. Would you consider updating the implementation to handle the type error by returning
Noneinstead ofmodel.items_typewhenitems_typeis not provided? This aligns with the fix suggested in the previous review.def create_types_map(self, model: TypesMapModel, config: Config, **kwargs: Any) -> TypesMap: items_type = ( self._create_component_from_model(model=model.items_type, config=config) if model.items_type - else model.items_type + else None ) return TypesMap( target_type=model.target_type, current_type=model.current_type, condition=model.condition if model.condition is not None else "True", items_type=items_type, )
438-438: LGTM! Clean import addition.The import is properly placed within the existing import statement.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1845-1846: LGTM! The TypesMap extension looks good.The addition of the
items_typefield to TypesMap enables handling array item type mappings, which aligns well with the PR objectives.
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
1996-2004: Consider removing unused field.The
property_namefield appears to be unused in the implementation. Should we remove it or is it intended for future use? 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(2 hunks)airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py(7 hunks)unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
[error] 254-254: List item 0 has incompatible type "list[str] | str"; expected "str"
[error] 274-274: Unsupported target for indexed assignment ("Mapping[str, Any]")
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (6)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (2)
229-229:⚠️ Potential issueFix type annotation in return type.
The return type annotation has a type error. The second element of the tuple is annotated as
List[str], but it's used to store a single string value fromtypes_map.items_type.type_mapping.target_type.Apply this diff to fix the type annotation:
- ) -> Tuple[Union[List[str], str], List[str]]: + ) -> Tuple[Union[List[str], str], List[Any]]:Likely invalid or redundant comment.
271-274:⚠️ Potential issueFix immutable mapping assignment.
The code attempts to modify an immutable
Mapping[str, Any]by assigning toairbyte_type["items"]. This will raise a runtime error.Apply this diff to fix the immutable mapping issue:
- airbyte_type = deepcopy(AIRBYTE_DATA_TYPES[field_type]) + airbyte_type: MutableMapping[str, Any] = deepcopy(AIRBYTE_DATA_TYPES[field_type])Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: Linters
[error] 274-274: Unsupported target for indexed assignment ("Mapping[str, Any]")
unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py (2)
341-341: LGTM! Good test coverage.The test case effectively validates the new array type mapping functionality with nested integer items.
383-388: LGTM! Clear schema definition.The schema field definition correctly maps the formula type with customInteger result type to an array of integers.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
2006-2011: LGTM! Well-structured type mapping.The TypesMap class is cleanly extended to support item type mapping while maintaining backward compatibility.
2013-2031: LGTM! Excellent documentation.The SchemaTypeIdentifier class is well-documented and provides a clear structure for schema type identification.
…f github.com:airbytehq/airbyte-python-cdk into lazebnyi/add-array-items-handling-to-dynamic-schemas
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (1)
270-270:⚠️ Potential issueConsider using
MutableMappingfor mutable operations.We're modifying
airbyte_typewhich is typed asMapping, but performing mutable operations (airbyte_type["items"] = items_type). Should we explicitly type it asMutableMapping? WDYT?-airbyte_type = deepcopy(AIRBYTE_DATA_TYPES[field_type]) +airbyte_type: MutableMapping[str, Any] = deepcopy(AIRBYTE_DATA_TYPES[field_type])Also applies to: 288-288
🧹 Nitpick comments (4)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (4)
21-21: Consider using a more precise type annotation forAIRBYTE_DATA_TYPES.The current type
Mapping[str, MutableMapping[str, Any]]could be more specific. Since we know the structure of the values, we could define a TypedDict for better type safety. WDYT?from typing import TypedDict, Literal class AirbyteDataType(TypedDict, total=False): type: List[str] format: str airbyte_type: str AIRBYTE_DATA_TYPES: Mapping[str, AirbyteDataType] = { # ... existing definitions ... }
50-57: Enhance documentation for better clarity.The class documentation could be more descriptive. Would you consider adding:
- Example usage
- Description of each field
- Explanation of how it integrates with
TypesMapExample:
""" Represents a mapping between a current type and its corresponding target type for array items. Example: ItemsTypeMap( items_type_pointer=["data", "type"], type_mapping=TypesMap(current_type="string", target_type="integer") ) Fields: items_type_pointer: Path to extract the item type from the schema type_mapping: Type conversion rules for array items """
71-77: Consider additional validations in__post_init__.The current validation checks if
items_typeis used only with arrays. Should we also validate:
- That
target_typeis a valid type fromAIRBYTE_DATA_TYPES?- That
current_typeis not empty or None?Example:
def __post_init__(self) -> None: if self.target_type not in AIRBYTE_DATA_TYPES: raise ValueError(f"Invalid target_type: {self.target_type}") if not self.current_type: raise ValueError("current_type cannot be empty") if self.items_type and self.target_type != "array": raise ValueError("'items_type' can only be used when 'target_type' is an array.")
272-286: Consider more descriptive error messages.The error handling in the array type processing could be more informative. Would you consider adding more context to help with debugging? For example:
- raise ValueError( - f"Invalid data type. Available string or two items list of string. Got {additional_types[0]}." - ) + raise ValueError( + f"Invalid array items type. Expected either a string (e.g., 'string') or a " + f"two-item list of strings (e.g., ['null', 'string']). Got {additional_types[0]}." + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (1)
Line range hint
242-260: Update function return type to matchComplexFieldTypeusage.
Currently,_replace_type_if_not_validyields a linter error at line 260, complaining that the function returnslist[str],str, orComplexFieldTypebut is declared to return onlylist[str]orstr. Would you consider updating the return annotation to includeComplexFieldType? WDYT?-def _replace_type_if_not_valid( - ... -) -> Union[List[str], str]: +def _replace_type_if_not_valid( + ... +) -> Union[List[str], str, ComplexFieldType]: ...🧰 Tools
🪛 GitHub Actions: Linters
[error] 224-224: Need type annotation for 'resolved_type' (hint: 'resolved_type: dict[, ] = ...')
[error] 229-229: Incompatible types in assignment (expression has type 'MutableMapping[str, Any]', variable has type 'dict[Any, Any]')
[error] 239-239: Incompatible types in assignment (expression has type 'MutableMapping[str, Any]', variable has type 'dict[Any, Any]')
🧹 Nitpick comments (6)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (1)
210-216: Conditional branch forComplexFieldType.
Nicely introduces a separate logic path for complex fields. No issues spotted. Would you consider adding a brief comment that clarifies how this fallback is triggered? WDYT?airbyte_cdk/sources/declarative/models/declarative_component_schema.py (3)
607-609: Good example usage for scopes.
These lines clearly depict how multiple scopes could be set. Would you consider including a small comment that clarifies if partial scopes are supported? WDYT?🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
1045-1059: Shorten or clarify docstring.
These lines are quite verbose. Would you consider referencing an external documentation link or summarizing for brevity? WDYT?🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
1672-1674:schema_normalizationusage.
The optional union withCustomSchemaNormalizationis well-defined. If additional checks are needed, consider them here. WDYT?🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
1902-1912:create_complex_field_typemethod.
Great approach, referencingitemsif it’s a nestedComplexFieldTypeModel. Might be helpful to confirm there's no recursion limit. WDYT?airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1803-1815: Consider adding usage examples for clarity.Would you like to include a simple example in the schema doc showing how "ComplexFieldType" might be nested, so users can see how "items" could reference another "ComplexFieldType"? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(10 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(4 hunks)airbyte_cdk/sources/declarative/schema/__init__.py(2 hunks)airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py(4 hunks)unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- airbyte_cdk/sources/declarative/schema/init.py
- unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
[error] 224-224: Need type annotation for 'resolved_type' (hint: 'resolved_type: dict[, ] = ...')
[error] 229-229: Incompatible types in assignment (expression has type 'MutableMapping[str, Any]', variable has type 'dict[Any, Any]')
[error] 239-239: Incompatible types in assignment (expression has type 'MutableMapping[str, Any]', variable has type 'dict[Any, Any]')
[error] 260-260: Incompatible return value type (got 'list[str] | str | ComplexFieldType', expected 'list[str] | str')
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
[warning] 5-490: Import block is un-sorted or un-formatted
airbyte_cdk/sources/declarative/models/declarative_component_schema.py
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Validate PR title
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (12)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (3)
21-21: Looks good!
No concerns about this updated type annotation at the top-level dictionary.
48-66: Neat addition ofComplexFieldType.
It provides a helpful structure for arrays with potential nested items. Great job clarifying the usage constraints for theitemsattribute.
264-264: Confirmed accurate type signature.
ReturningMutableMapping[str, Any]is consistent and should resolve potential immutability issues.airbyte_cdk/sources/declarative/models/declarative_component_schema.py (4)
1084-1086: Nice example block.
This snippet makes it clear how to pass server input specs. Seems straightforward.🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
1848-1856: Added newnamefield for the stream.
Thanks for providing a default of an empty string. That helps avoid undefined behavior.🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
2285-2290: Clear naming forcomponents_resolver.
This clarifies the approach to dynamic instantiation. No apparent concerns.🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
2294-2294: Rerun format to resolve lint warnings.
The pipeline complains that the file needs formatting. Would you consider runningruff formator an equivalent tool to fix style issues? WDYT?🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting. Run 'ruff format' to fix formatting issues.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (4)
247-249: ImportingComplexFieldTypeModelis consistent.
No concerns found with these newly introduced imports.
579-579: Clear mapping extension.
You've neatly listedComplexFieldTypeModelin the_init_mappings. Thanks for maintaining consistency.
1913-1921:create_types_mapmethod improvement.
Nice logic for bridgingComplexFieldTypewith standardstrorList[str].
Line range hint
5-490: Reformat imports.
The pipeline complains this import block is unsorted. Would you consider runningruff formator a similar tool to organize them? WDYT?airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1830-1830: Validate integration with the new "ComplexFieldType".Do we need further validation or documentation ensuring that other references in this file (or the code) handle "ComplexFieldType" correctly? Maybe we could add a short note in the doc to make users aware of the new type. wdyt?
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
Outdated
Show resolved
Hide resolved
…f github.com:airbytehq/airbyte-python-cdk into lazebnyi/add-array-items-handling-to-dynamic-schemas
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
1902-1912: Consider adding a clarifying docstring forcreate_complex_field_type.
For example, you could describe how items might be a string or nestedComplexFieldTypeModel. Would that be helpful, wdyt?
1913-1919: Only handlingtarget_typeas aComplexFieldTypeModelmight be limiting.
If you ever need a complex type forcurrent_type, would you consider extending similar logic to that property as well, wdyt?airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (2)
48-66: Neat introduction ofComplexFieldTypewith__post_init__validation.
Would you consider raising a more descriptive error message if someone incorrectly setsitemswith a non-array field? For instance, clarifying thefield_typemismatch. wdyt?
222-242: Consider handling potential circular references in_resolve_complex_type.
If there's a cycle (though presumably disallowed by the schema), it could cause an infinite loop. Would an explicit cycle check or a note in docs help? wdyt?airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
739-743: IntroducingComplexFieldTypeis a great addition.
Would you add a usage example to show howitemscan be nested? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py(2 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(4 hunks)airbyte_cdk/sources/declarative/schema/__init__.py(2 hunks)airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py(4 hunks)unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- airbyte_cdk/sources/declarative/schema/init.py
- unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (10)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
136-138: Thanks for introducing the new import.
It aligns with the usage ofComplexFieldTypeModelbelow. No concerns here.
435-435: IncludingComplexFieldTypeis consistent with the new logic.
This import usage appears correct and clearly references the newly introduced class.
579-579: RegisteringComplexFieldTypeModelin the mappings looks excellent.
This will properly delegate complex field creation tocreate_complex_field_type.airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (5)
21-21: Validating the switch to aMapping[str, MutableMapping[str, Any]].
Since you later mutate the dictionary, this is consistent. However, might it be safer to store a fresh copy each time rather than mutating the globalAIRBYTE_DATA_TYPES? wdyt?
74-74: AllowingTypesMap.target_typeto beComplexFieldTypebroadens usage.
This approach seems appropriate for supporting nested structures. Nicely done.
210-210: Good job addingelif isinstance(mapped_field_type, ComplexFieldType).
This fallback ensures complex types get routed to_resolve_complex_type. The structure looks solid.Also applies to: 214-216
247-247: No issues with this adjusted signature.
Everything is consistent for returningComplexFieldType.
264-264: Double-check the mutable return in_get_airbyte_type.
Returning aMutableMapping[str, Any]might lead to external mutations. Would you consider returning an immutable copy if unintentional changes are a concern? wdyt?airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
745-745: Expandingtarget_typeto acceptComplexFieldTypeincreases flexibility.
This looks ready for nested schema support.
2268-2268: Thank you for callingupdate_forward_refs()onComplexFieldType.
This helps ensure referencing nested classes works properly. Looks good.
|
@lazebnyi - FYI - I'm updating from |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
136-138: Unify the imports forComplexFieldType?
It looks like you're importingComplexFieldTypeboth here (asComplexFieldTypeModel) and again at line 435 (directly fromairbyte_cdk.sources.declarative.schema). Would you consider consolidating them into a single import to avoid confusion, wdyt?Here's a possible diff showing how you might remove the reference in line 435 if it's not needed elsewhere:
- ComplexFieldType,
579-579: Add a direct test forcreate_complex_field_type?
Would you consider creating a dedicated unit test for"create_complex_field_type"in order to ensure that complex nesting scenarios are properly handled, wdyt?
1913-1921: Clarify or documenttarget_typeusage.
Your logic for handlingComplexFieldTypeModelintarget_typelooks great, but it may help future maintainers if you briefly document when and howtarget_typecan be aComplexFieldTypeModelvs. a simple type, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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 (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
435-435: Duplicate import forComplexFieldType.
This line seems to duplicate what’s imported at lines 136-138. Would you consider removing one of them, wdyt?
1902-1912: Validate the type ofmodel.items?
While you gracefully handle the scenario in whichmodel.itemsis itself aComplexFieldTypeModel, there’s no clear check for unexpected types. Would you consider adding a type validation or fail-safe check ifmodel.itemsis neitherNonenor aComplexFieldTypeModel, wdyt?
…f github.com:airbytehq/airbyte-python-cdk into lazebnyi/add-array-items-handling-to-dynamic-schemas
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (2)
48-65: Consider adding field_type validationThe class looks good! Would you consider adding validation to ensure field_type is one of the valid AIRBYTE_DATA_TYPES? WDYT?
def __post_init__(self) -> None: """ Enforces that `items` is only used when `field_type` is a array """ + if self.field_type not in AIRBYTE_DATA_TYPES: + raise ValueError(f"Invalid field_type. Must be one of {list(AIRBYTE_DATA_TYPES.keys())}. Got {self.field_type}") # `items_type` is valid only for array target types if self.items and self.field_type != "array": raise ValueError("'items' can only be used when 'field_type' is an array.")🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
223-231: Add type annotation and error handlingThe method looks good but could use some improvements. WDYT about these changes?
-def _resolve_complex_type(self, complex_type: ComplexFieldType) -> Mapping[str, Any]: +def _resolve_complex_type(self, complex_type: ComplexFieldType) -> MutableMapping[str, Any]: if not complex_type.items: return self._get_airbyte_type(complex_type.field_type) - field_type = self._get_airbyte_type(complex_type.field_type) + field_type: MutableMapping[str, Any] = self._get_airbyte_type(complex_type.field_type) + if not isinstance(complex_type.items, (str, ComplexFieldType)): + raise ValueError(f"Invalid items type. Expected str or ComplexFieldType, got {type(complex_type.items)}") field_type["items"] = self._get_airbyte_type(complex_type.items) if isinstance(complex_type.items, str) else self._resolve_complex_type( complex_type.items) return field_type🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py (1)
377-382: Remove duplicate Currency fieldThere appears to be a duplicate Currency field in the test data. Would you like to remove one of them? WDYT?
{ "name": "AvgSalary", "type": "formula", "result": {"type": "customInteger"}, }, {"name": "Currency", "type": "formula", "result": {"type": "currency"}}, - {"name": "Currency", "type": "formula", "result": {"type": "currency"}}, {"name": "WorkingDays", "type": "formula"},🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py(5 hunks)unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py
[warning] File requires formatting using Ruff formatter
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
[warning] File requires formatting using Ruff formatter
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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 (Fast)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (3)
21-21: LGTM! Type annotation improvementThe change from
Mapping[str, Any]toMapping[str, MutableMapping[str, Any]]better reflects the mutable nature of the type definitions.🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
74-75: LGTM! Type annotation updateThe addition of ComplexFieldType to target_type Union is consistent with the new feature.
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
157-159: LGTM! Security and flexibility improvementsGood improvements:
- Using HTTPS for the schema URL enhances security
- Adding additionalProperties makes the schema more flexible for future extensions
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py (2)
87-94: LGTM! Good test coverage for complex typesThe test case properly covers the new ComplexFieldType feature with nested arrays and conditional mapping.
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
324-325: LGTM! Comprehensive test coverageThe test case properly validates:
- HTTPS schema URL
- additionalProperties flag
- Complex nested array types
Also applies to: 335-335
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (4)
21-21: Consider using a concrete dict type.Currently,
AIRBYTE_DATA_TYPESis set toMapping[str, MutableMapping[str, Any]]. Would usingdict[str, Any]in place ofMutableMapping[str, Any]simplify the code, since we're always dealing with dictionaries anyway? wdyt?
48-66: Add some unit tests for this new class.
ComplexFieldTypeis a valuable addition. Would you like to add a quick unit test scenario to confirm thatitemsonly applies tofield_type="array"and otherwise raises aValueError? wdyt?
157-159: Assess whether allowing additional properties is desired.The schema sets
"additionalProperties": True. Do we want to consider stricter validation by restricting unknown properties, or is this flexibility intentional? wdyt?
257-257: Consider usingdict[str, Any]for_get_airbyte_type.Using
MutableMapping[str, Any]is correct given the current approach. However, for consistency, would you prefer simply usingdict[str, Any]since we know it's a dictionary? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py(5 hunks)unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py
⏰ 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 (4)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (4)
74-74: Verify thorough usage and tests for complex target types.Adding
ComplexFieldTypesupport inTypesMapis great. Have we tested scenarios with nested or multi-level complex types to confirm they resolve correctly? wdyt?
211-214: Good coverage for multiple type definitions.The explicit checks for
strand the subsequent handling ofComplexFieldTypemake sense. Do we need to account for other edge cases, or is this logic sufficient? wdyt?
215-217: Ensure deep testing for nested arrays.Handling
ComplexFieldTyperecursively is powerful. Would you like to add more tests to confirm it works for multiple nested levels? wdyt?
240-240: Type extension looks consistent.Adding
ComplexFieldTypeto the return union in_replace_type_if_not_validaligns nicely with the rest of the code. This looks correct. wdyt?
lazebnyi
deleted the
lazebnyi/add-array-items-handling-to-dynamic-schemas
branch
lazebnyi
changed the title
What
For type mapping in dynamic streams, array type require an option to parse item types to ensure correct normalization.
How
Added
ComplexFieldTypeto target types inTypesMapand handle them in the core logic.Summary by CodeRabbit
New Features
ComplexFieldTypefor defining complex field structures.TypesMapto support complex type mappings.arraytype conditions in schema loading.Improvements
Technical Updates
ComplexFieldTypeand its integration into the schema.