feat(low-code): add default type to dynamic schema loader#335
feat(low-code): add default type to dynamic schema loader#335Daryna Ishchenko (darynaishchenko) wants to merge 3 commits intomainfrom
Conversation
|
/autofix
|
📝 WalkthroughWalkthroughThe pull request introduces a new optional Changes
Sequence Diagram(s)sequenceDiagram
participant MCF as ModelToComponentFactory
participant STI as SchemaTypeIdentifier
participant DSL as DynamicSchemaLoader
MCF->>STI: Create identifier (with default_type if provided)
STI-->>MCF: Return identifier object
MCF->>DSL: Pass identifier for schema resolution
DSL->>DSL: Check if field type is in AIRBYTE_DATA_TYPES
alt Field type is invalid
DSL->>STI: Return default_type (if defined)
else Field type is valid
DSL->>STI: Return original field type
end
Does this diagram capture the updated control flow clearly, wdyt? Possibly related PRs
✨ 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: 0
🧹 Nitpick comments (3)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (1)
256-258: Consider optimizing the default type check.The current implementation checks if the field_type is not in AIRBYTE_DATA_TYPES after the types_mapping check. What do you think about combining these checks to avoid multiple lookups? wdyt?
- if self.schema_type_identifier.default_type and field_type not in AIRBYTE_DATA_TYPES: + if field_type not in AIRBYTE_DATA_TYPES and self.schema_type_identifier.default_type: return self.schema_type_identifier.default_type return field_typeairbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
2048-2055: LGTM! The default_type parameter is correctly added.The implementation looks good and aligns with the PR objectives. The default_type is properly passed to the SchemaTypeIdentifier constructor with a fallback to None if not provided.
One small suggestion: Would you consider adding a docstring to explain the purpose of the default_type parameter and when it should be used? This would help future developers understand its role in schema type identification, wdyt?
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
2050-2053: LGTM! The newdefault_typeproperty is well-defined.The addition of the
default_typeproperty with clear title and description aligns well with the PR objectives to handle unknown types from Airtable. The property is appropriately defined as an optional string field.Just a thought - would it make sense to add some examples of common default types (like "string") in the schema definition to help guide users? wdyt?
default_type: title: Default Type description: Default to be set if field type wasn't found in the types_mapping. type: string + examples: + - "string" + - "integer" + - "boolean"
📜 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/schema/dynamic_schema_loader.py(2 hunks)unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py(1 hunks)
⏰ 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 (3)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (1)
90-90: LGTM! Nice addition of the default_type field.The optional default_type field provides a good fallback mechanism for handling unknown field types.
unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py (1)
408-475: Great test coverage!The test thoroughly validates the default_type behavior across different scenarios:
- Unknown type defaulting to string
- Type in types map but condition is False
- Airbyte type handling
- Multiple records with different types
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
815-819: LGTM! Well-documented model field.The default_type field is properly added to the model with clear description and title.
Daryna Ishchenko (darynaishchenko)
requested a review
from Serhii Lazebnyi (lazebnyi)
1 participant
What
For source airtable we already have types mapping, but airtable also have types that "unknown" and python airtable version handled it as strings. but Dynamic Schema loader fails with value error if type of field not in airbyte types map or in schema identifier types map.
How
Added
default_typetoschema_type_identifier, When provided default type will be used if field not it airbyte types map or in schema identifier types map.Summary by CodeRabbit
New Features
Tests