feat(low-code): added condition to TypesMap of DynamicSchemaLoader #224
Conversation
|
/autofix
|
|
Warning Rate limit exceeded@darynaishchenko has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Factory as ModelToComponentFactory
participant SchemaLoader as DynamicSchemaLoader
participant TypesMap as TypesMap
Factory->>TypesMap: Create with condition
TypesMap-->>Factory: TypesMap instance
SchemaLoader->>TypesMap: Evaluate condition
TypesMap-->>SchemaLoader: Condition result
SchemaLoader->>SchemaLoader: Replace type if condition is true
Possibly related PRs
Suggested reviewers
Hey there! Quick question - wdyt about how the new 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: 2
🔭 Outside diff range comments (1)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (1)
Line range hint
6-21: Would you mind sorting the imports to fix the linter error?The linter reports an error (Ruff I001) indicating that the import block is un-sorted or un-formatted. Sorting the imports should resolve this issue. You can run the linter with the
--fixoption to automatically sort the imports. wdyt?
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1766-1771: LGTM! The schema definition is well-structured.The addition of the
conditionproperty toTypesMapis well-defined with:
- Support for both string and boolean values through
anyOf- Proper interpolation context for
raw_schemaWhat do you think about adding some examples to help users understand how to use the condition property effectively? Something like:
examples: - "{{ raw_schema['type'] == 'string' }}" - true
📜 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(4 hunks)unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
[error] 6-21: Import block is un-sorted or un-formatted (Ruff I001). This can be fixed automatically using the --fix option.
⏰ 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/models/declarative_component_schema.py (1)
702-702: Addition ofconditionattribute looks good.The
conditionattribute inTypesMapenhances flexibility. Good addition!airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
1689-1693: LGTM! The implementation looks clean and handles the default case well.The changes to
create_types_mapmethod look good. I particularly like how you've handled the default case by settingcondition=Truewhenmodel.conditionisNone. This maintains backward compatibility while adding the new functionality.
unit_tests/sources/declarative/schema/test_dynamic_schema_loader.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
[error] 208-208: Argument "condition" to "InterpolatedBoolean" has incompatible type "str | bool | None"; expected "str"
⏰ 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 (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (4)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (4)
13-13: LGTM! The new import and TypesMap condition field look good.The addition of the optional condition field to TypesMap aligns well with the PR objective of supporting conditional type mapping.
Also applies to: 57-57
182-182: LGTM! Good addition of raw_schema parameter.The change properly propagates the schema context needed for condition evaluation.
199-201: LGTM! Method signature update looks good.The addition of raw_schema parameter with proper type annotation maintains type safety.
208-210: Should we pass the actualconfigandparametersinstead of empty dicts?Currently,
configandparametersare set to empty dictionaries when evaluating theInterpolatedBooleancondition. Passing the actualconfigandparametersmight be necessary for correct interpolation. wdyt?🧰 Tools
🪛 GitHub Actions: Linters
[error] 208-208: Argument "condition" to "InterpolatedBoolean" has incompatible type "str | bool | None"; expected "str"
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1766-1769: LGTM! Consider adding documentation for the condition property.The addition of the
conditionproperty toTypesMaplooks good. Would you like to add a description field to document its purpose and usage? For example:condition: type: string + description: Optional condition to evaluate against the raw schema to determine if the type mapping should be applied. interpolation_context: - raw_schemaairbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (1)
208-214: LGTM! Consider adding error handling for condition evaluation.The condition evaluation logic looks good. Would you like to add error handling for potential evaluation failures? For example:
condition = InterpolatedBoolean( condition=types_map.condition if types_map.condition is not None else "true", parameters={}, + ).eval(config=self.config, raw_schema=raw_schema) - ).eval(config=self.config, raw_schema=raw_schema) + except Exception as e: + logger.warning(f"Failed to evaluate condition: {e}. Defaulting to True.") + condition = True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
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/schema/dynamic_schema_loader.py(4 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 (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py (2)
13-13: LGTM! Import added for new functionality.The import of
InterpolatedBooleanis required for the condition evaluation feature.
57-57: LGTM! Field matches schema definition.The
conditionfield is correctly defined asOptional[str], matching the schema definition.airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
702-702: LGTM! Field matches schema definition.The
conditionfield is correctly defined asOptional[str], maintaining consistency with the schema.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
713-713: LGTM! Consider adding field documentation?The addition of the optional
conditionfield toTypesMaplooks good and aligns with the PR objective of handling complex type determination scenarios. Would you like to add some documentation using Field(..., description="...") to clarify its purpose and provide usage examples? wdyt?Here's a suggestion for the field definition:
- condition: Optional[str] + condition: Optional[str] = Field( + None, + description="Optional condition to evaluate when determining if this type mapping should be applied. Useful when the type depends on additional fields beyond just the 'type' field.", + examples=['{{ raw_schema["type"] == "formula" and raw_schema["result_type"] == "number" }}'], + title="Type Mapping Condition" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
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)
🚧 Files skipped from review as they are similar to previous changes (2)
- airbyte_cdk/sources/declarative/declarative_component_schema.yaml
- airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
⏰ 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 (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
airbyte_cdk/sources/declarative/schema/dynamic_schema_loader.py
Outdated
Show resolved
Hide resolved
darynaishchenko
deleted the
daryna/low-code/add-conditions-to-schema-type-map
branch
What
Dynamic schema loader: there can be a case when type of the field doesn't depend on "type" field value only. Example:
source airtable.field_1
field_2
How
To properly identify field type in cases like above
conditionoption was added toTypesMapobject. Now providing TypesMap object condition we can create 2 separate cases for string values and number values.See
unit_tests/sources/declarative/schema/test_dynamic_schema_loader.pyfor more examples.Summary by CodeRabbit
New Features
conditionproperty to support more flexible type mapping and record filtering in declarative components.Improvements
Tests