test(low code cdk): dynamic streams changes #132
Conversation
…hq/airbyte-python-cdk into lazebnyi/add-components-resolver
…tehq/airbyte-python-cdk into lazebnyi/add-dynamic-schema-loader
…tehq/airbyte-python-cdk into lazebnyi/add-dynamic-schema-loader
📝 WalkthroughWalkthroughThe pull request introduces significant changes across multiple files related to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Hey there! Do you think these changes will cover all the necessary aspects for the new features? Wdyt? 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: 1
🧹 Outside diff range and nitpick comments (11)
airbyte_cdk/sources/declarative/resolvers/components_resolver.py (3)
27-37: Consider combining similar classes to reduce duplicationNoticing that both
ComponentMappingDefinitionandResolvedComponentMappingDefinitionhave very similar structures, perhaps we could refactor the code to reduce duplication, maybe by having one inherit from the other or by creating a shared base class. Wdyt?
39-41: Consider using an experimental warning instead of deprecationUsing the
@deprecateddecorator to markComponentsResolveras experimental might be a bit misleading since deprecation typically implies that the feature is outdated or will be removed. Would it make sense to use a custom experimental decorator or warning to indicate that this class is experimental? Wdyt?
46-55: Enhance the docstring forresolve_componentsmethodThe docstring for the
resolve_componentsmethod is currently quite brief. Could we provide more detailed information about how subclasses should implement this method and what the expected behavior is? Maybe including an example would be helpful. Wdyt?airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
1624-1649: Consider refactoring duplicate logic increate_dynamic_schema_loaderandcreate_http_components_resolver.Both methods contain similar code for building stream slicers and retrievers. Extracting this common logic into a helper function could improve maintainability and reduce duplication. Wdyt?
2317-2332: Address the type annotation forfield_pathincreate_components_mapping_definition.There's a
# type: ignore[arg-type]comment due tofield_pathpotentially beingstrorInterpolatedString. Would it be possible to adjust the type annotations to avoid usingtype: ignore, perhaps by specifyingUnion[str, InterpolatedString]in theComponentMappingDefinition? Wdyt?
2335-2336: Specify the return type forcreate_http_components_resolver.Currently, the method's return type is annotated as
Any. Would it be better to specify the exact return typeHttpComponentsResolverto enhance type checking and readability? Wdyt?unit_tests/sources/declarative/resolvers/test_http_components_resolver.py (2)
114-148: Consider adding more test cases totest_http_components_resolver.Currently, the parameterized test includes a single test case. Would it be helpful to add additional cases to cover more scenarios, such as different component mappings, varied retriever data, and edge cases? Wdyt?
150-189: Consider enhancing assertions intest_dynamic_streams_read.The test verifies the number of streams and records but doesn't assert the content of the records. Including assertions that check the actual data in the records could ensure they match expected values. Wdyt?
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py (2)
34-39: Consider grouping related mappings together?The new mappings for DynamicDeclarativeStream and HttpComponentsResolver are logically related. What do you think about grouping them together under a common section with a comment to improve readability? wdyt?
# DeclarativeStream "DeclarativeStream.retriever": "SimpleRetriever", "DeclarativeStream.schema_loader": "JsonFileSchemaLoader", + # Dynamic Stream Components # DynamicDeclarativeStream "DynamicDeclarativeStream.stream_template": "DeclarativeStream", "DynamicDeclarativeStream.components_resolver": "HttpComponentsResolver", # HttpComponentsResolver "HttpComponentsResolver.retriever": "SimpleRetriever", "HttpComponentsResolver.components_mapping": "ComponentMappingDefinition",
67-70: Consider grouping schema-related mappings together?These new mappings are related to schema handling. Would it make sense to group them with other schema-related mappings if any exist? wdyt?
+ # Schema Components # DynamicSchemaLoader "DynamicSchemaLoader.retriever": "SimpleRetriever", # SchemaTypeIdentifier "SchemaTypeIdentifier.types_map": "TypesMap",airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1687-1707: Consider adding validation examples for TypesMap?The TypesMap component is well-defined but might benefit from examples showing valid type mappings. Would you like to add some examples to help users understand the expected format? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py(1 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(9 hunks)airbyte_cdk/sources/declarative/manifest_declarative_source.py(4 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(8 hunks)airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py(2 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(12 hunks)airbyte_cdk/sources/declarative/partition_routers/__init__.py(1 hunks)airbyte_cdk/sources/declarative/resolvers/__init__.py(1 hunks)airbyte_cdk/sources/declarative/resolvers/components_resolver.py(1 hunks)airbyte_cdk/sources/declarative/resolvers/http_components_resolver.py(1 hunks)airbyte_cdk/sources/declarative/schema/__init__.py(1 hunks)airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py(1 hunks)unit_tests/sources/declarative/resolvers/__init__.py(1 hunks)unit_tests/sources/declarative/resolvers/test_http_components_resolver.py(1 hunks)unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py(1 hunks)unit_tests/sources/declarative/test_manifest_declarative_source.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- unit_tests/sources/declarative/resolvers/init.py
- airbyte_cdk/sources/declarative/resolvers/init.py
🔇 Additional comments (19)
airbyte_cdk/sources/declarative/resolvers/components_resolver.py (1)
21-23: Consistency in value type annotations
In ComponentMappingDefinition, the value field is typed as Union[InterpolatedString, str], whereas in ResolvedComponentMappingDefinition, it's just InterpolatedString. Is this difference intentional? Ensuring consistent type annotations might improve clarity. What do you think?
Also applies to: 33-35
airbyte_cdk/sources/declarative/resolvers/http_components_resolver.py (1)
1-106: LGTM!
The HttpComponentsResolver class is well-implemented and follows best practices. The methods are clearly defined, and the logic appears sound.
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (1)
1-219: LGTM!
The DynamicSchemaLoader class is thoughtfully designed, and the methods for schema construction are comprehensive. The handling of data extraction and type mapping is robust.
airbyte_cdk/sources/declarative/manifest_declarative_source.py (3)
337-339: Should we verify the existence of 'requester' before setting 'use_cache'?
Currently, we set use_cache on components_resolver_config["retriever"]["requester"] without checking if 'requester' exists. Would it be safer to check if 'requester' is present within 'retriever' before setting use_cache? WDYT?
345-347: Consider adding a check for 'stream_template' in 'dynamic_definition'.
We access dynamic_definition["stream_template"] directly without verifying its existence. Should we add a check to ensure 'stream_template' is present in dynamic_definition to prevent potential KeyError exceptions? WDYT?
Line range hint 1-356: LGTM!
The integration of dynamic stream configurations enhances the source's flexibility. The additional validations and error handling improve robustness.
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
185-190: LGTM!
Combining static and dynamic stream configurations in _group_streams is a logical enhancement and should support concurrent processing effectively.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (3)
653-677: LGTM!
The introduction of TypesMap and SchemaTypeIdentifier classes adds valuable functionality for dynamic schema loading.
1187-1216: LGTM!
The ComponentMappingDefinition class is a useful addition for defining component mappings, and the fields are well-documented.
Line range hint 1515-1576: LGTM!
The restructuring of DeclarativeSource into multiple classes to handle dynamic streams is well-executed. This change should provide greater flexibility in source configurations.
airbyte_cdk/sources/declarative/schema/__init__.py (1)
9-11: Imports and update to __all__ look appropriate.
The added imports and changes to __all__ correctly expose the new classes.
airbyte_cdk/sources/declarative/partition_routers/__init__.py (1)
9-11: Adding PartitionRouter to imports and __all__ looks good.
The module now correctly includes PartitionRouter in its public interface.
unit_tests/sources/declarative/test_manifest_declarative_source.py (4)
74-82: LGTM! Well-structured base fixture.
The _base_manifest fixture provides a clean base configuration for tests. Good practice using descriptive docstring and minimal configuration.
83-134: LGTM! Comprehensive stream configuration fixture.
The _declarative_stream fixture is well-designed with:
- Flexible configuration through parameters
- Clear docstring
- Realistic test data
135-182: LGTM! Well-structured dynamic stream fixture.
The _dynamic_declarative_stream fixture effectively reuses the _declarative_stream fixture and provides a complete dynamic stream configuration.
629-659: LGTM! Thorough test coverage.
The test effectively verifies:
- Failure case with missing streams and dynamic streams
- Success case with regular streams
- Success case with dynamic streams
Good job on the clear test organization and comprehensive coverage!
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (3)
11-15: LGTM! Clear stream configuration requirement.
The anyOf condition clearly specifies that either streams or dynamic_streams must be present. Good job on making this requirement explicit in the schema.
26-29: LGTM! Well-structured dynamic_streams definition.
The dynamic_streams array property is well-defined with a clear reference to DynamicDeclarativeStream.
2992-3081: LGTM! Well-documented experimental components.
The new experimental components are thoroughly documented with:
- Clear descriptions and experimental status warnings
- Required properties
- Comprehensive property descriptions
- Proper schema references
| with pytest.raises(ValueError, match="Expected key to be a string. Got None"): | ||
| dynamic_schema_loader.get_json_schema() |
There was a problem hiding this comment.
Check the error message in test_dynamic_schema_loader_invalid_type.
The ValueError is expected when an invalid type is provided, but the match string is "Expected key to be a string. Got None", which appears to relate to the key instead of the type. Should this be updated to match the error message for invalid types, such as "Invalid type specified"? Wdyt?
branch for testing changes from #104 and #88
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests