feat(low-code cdk): add dynamic stream name to stream model #441
Conversation
|
/autofix
|
📝 WalkthroughWalkthroughThis pull request introduces new properties to support dynamic stream naming. Changes were made to the declarative schema definitions in both the YAML and Python model files by adding a string property ( Changes
Sequence Diagram(s)sequenceDiagram
participant DS as DeclarativeSource
participant SC as Stream Configs
participant DD as Dynamic Definitions
DS->>SC: Invoke _stream_configs
SC-->>DS: Return stream configs with dynamic_stream_name = None
DS->>DD: Invoke _dynamic_stream_configs (using enumerate)
Note right of DD: For each dynamic definition,<br>assign dynamic_stream_name (either provided or default: "dynamic_stream_i")
DD-->>DS: Return updated dynamic stream configs
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 0
🧹 Nitpick comments (5)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1430-1437: Review Comment: Newdynamic_stream_nameProperty in DeclarativeStreamYou've added the
dynamic_stream_nameproperty with a string type, a default ofNone, and an example value of"Tables". This addition fits the PR objective of supporting dynamic stream naming nicely. Would you consider reviewing if a default ofNone(versus, say, an explicit empty string or YAML null) is consistent with how similar properties (likenamein DynamicDeclarativeStream) are handled in our schema? wdyt?airbyte_cdk/sources/declarative/models/declarative_component_schema.py (3)
2052-2057: Is "None" the right default as a string?The default value for
dynamic_stream_nameis set as the string "None" rather than the PythonNonevalue. This might cause unexpected behavior if code elsewhere checks for this field beingNoneusing equality (is Noneor== None). Would changing this to actualNonemake more sense, wdyt?dynamic_stream_name: Optional[str] = Field( - "None", + None, description="The dynamic stream name that create current stream, if None is static stream.", example=["Tables"], title="Dynamic Stream Name", )
2052-2057: Consider improving the description grammarThe description has a minor grammatical issue. Would you mind reviewing it for clarity?
dynamic_stream_name: Optional[str] = Field( "None", description="The dynamic stream name that create current stream, if None is static stream.", - example=["Tables"], + examples=["Tables"], title="Dynamic Stream Name", )I noticed
exampleis used instead ofexamples(which is used in other similar fields throughout the file). Also, the description could be clearer about what this field represents.
2498-2500: Consistency in examples vs exampleFor consistency with the rest of the file, should this be
examplesinstead ofexample?name: Optional[str] = Field( "", description="The dynamic stream name.", - example=["Tables"], + examples=["Tables"], title="Name" )airbyte_cdk/sources/declarative/manifest_declarative_source.py (1)
397-399: Consider consistent naming patternsThe fallback pattern
dynamic_stream_{dynamic_definition_index}works, but would a more descriptive pattern be better? Perhaps including some identifier from the stream_template if available?For example:
{template_name}_{dynamic_definition_index}might give more context about what kind of stream this is. Just a thought for consideration!
📜 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/manifest_declarative_source.py(3 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(2 hunks)unit_tests/sources/declarative/resolvers/test_http_components_resolver.py(3 hunks)unit_tests/sources/declarative/test_concurrent_declarative_source.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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)
🔇 Additional comments (8)
unit_tests/sources/declarative/test_concurrent_declarative_source.py (1)
712-714: The new assertion verifies that dynamic_stream_name is None by defaultGood addition! This test ensures that every stream in the configuration has the
dynamic_stream_nameproperty initialized toNoneby default, which establishes a clear baseline behavior for the property.unit_tests/sources/declarative/resolvers/test_http_components_resolver.py (3)
60-60: Nice addition of the name property for dynamic streamsAdding the
nameproperty to theDynamicDeclarativeStreamconfiguration in the test manifest helps establish the expected behavior for dynamic stream naming. This is essential for testing the new functionality.
491-493: Good test coverage for the dynamic stream nameThis assertion verifies that the dynamic stream name is correctly propagated from the manifest to the stream configuration. I like how you're validating that the value matches "TestDynamicStream" exactly as defined in the manifest.
579-581: Nice verification for index-based dynamic stream namesThis assertion checks that dynamic stream names follow the expected naming convention when not explicitly provided in the manifest. The assertion for "dynamic_stream_0" suggests an index-based default naming strategy, which provides good backwards compatibility.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
3776-3782: Review Comment: AddednameProperty in DynamicDeclarativeStreamThe new
nameproperty for DynamicDeclarativeStream has been introduced with typestring, a default of an empty string, and an example"Tables". This change aligns well with the dynamic stream naming feature outlined in the PR. Do you feel that the use of an empty string here (versus using a similar default as in DeclarativeStream) provides the clear distinction you intended between static and dynamic streams? wdyt?airbyte_cdk/sources/declarative/manifest_declarative_source.py (3)
346-346: Good addition of dynamic_stream_name to static streams!Setting
dynamic_stream_nametoNonefor static streams is a clean approach to differentiate them from dynamic streams. This will help with stream identification and make the codebase more maintainable.
358-358: Good use of enumerate for index trackingUsing
enumerate()is a clean way to get both the index and value in Python. This change allows you to create default names for dynamic streams based on their position in the configuration.
397-399: Nice fallback mechanism for dynamic stream namingI like how you've implemented a fallback mechanism that uses the explicit name if provided, or generates a default using the index. This ensures every dynamic stream has a unique identifier even if names aren't explicitly provided.
|
will be added in #442 |
Fixed: https://github.com/airbytehq/airbyte-internal-issues/issues/12171
Summary by CodeRabbit
New Features
Tests