feat(ibis): enhance the function list of snowflake#1350
feat(ibis): enhance the function list of snowflake#1350douenergy merged 2 commits intoCanner:mainfrom
Conversation
WalkthroughAdds Snowflake VARIANT column type support by introducing a new enum member and type mapping, updates remote function count expectations, enables Snowflake data source queries locally, and improves error handling for remote functions with invalid return types. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wren-core/core/src/mdl/function/remote_function.rs (1)
187-199: Critical: Inconsistent error handling across function types.The
From<RemoteFunction>implementation forByPassScalarUDFnow falls back toDataType::Utf8for invalid return types, but the analogous implementations forByPassAggregateUDF(line 274) andByPassWindowFunction(line 337) still use.unwrap()and will panic. This creates inconsistent behavior where invalid return types are silently tolerated for scalar functions but cause panics for aggregate and window functions.Additionally, the comment on line 189 states "just panic if the return type is not valid" but the code now does the opposite.
Apply this diff to make error handling consistent across all three function types:
impl From<RemoteFunction> for ByPassScalarUDF { fn from(func: RemoteFunction) -> Self { - // just panic if the return type is not valid to avoid we input invalid type + // Fall back to Utf8 if the return type is not recognized let return_type = ReturnType::from_str(&func.return_type) .unwrap_or(ReturnType::Specific(DataType::Utf8)); ByPassScalarUDF {impl From<RemoteFunction> for ByPassAggregateUDF { fn from(func: RemoteFunction) -> Self { - // just panic if the return type is not valid to avoid we input invalid type - let return_type = ReturnType::from_str(&func.return_type).unwrap(); + // Fall back to Utf8 if the return type is not recognized + let return_type = ReturnType::from_str(&func.return_type) + .unwrap_or(ReturnType::Specific(DataType::Utf8)); ByPassAggregateUDF {impl From<RemoteFunction> for ByPassWindowFunction { fn from(func: RemoteFunction) -> Self { - // just panic if the return type is not valid to avoid we input invalid type - let return_type = ReturnType::from_str(&func.return_type).unwrap(); + // Fall back to Utf8 if the return type is not recognized + let return_type = ReturnType::from_str(&func.return_type) + .unwrap_or(ReturnType::Specific(DataType::Utf8)); ByPassWindowFunction {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ibis-server/resources/function_list/snowflake.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
ibis-server/app/model/metadata/dto.py(1 hunks)ibis-server/app/model/metadata/snowflake.py(1 hunks)ibis-server/tests/routers/v3/connector/snowflake/test_functions.py(1 hunks)ibis-server/tools/query_local_run.py(2 hunks)wren-core/core/src/mdl/function/remote_function.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ibis-server/app/model/metadata/snowflake.py (1)
ibis-server/app/model/metadata/dto.py (1)
RustWrenEngineColumnType(13-58)
ibis-server/tools/query_local_run.py (2)
ibis-server/app/model/__init__.py (1)
SnowflakeConnectionInfo(329-360)ibis-server/app/model/data_source.py (1)
get_snowflake_connection(366-397)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: ci
- GitHub Check: cargo check
🔇 Additional comments (4)
ibis-server/app/model/metadata/dto.py (1)
53-53: LGTM! Clean addition to support Snowflake VARIANT type.The new enum member follows the existing pattern and enables proper type mapping for Snowflake's semi-structured data types.
ibis-server/app/model/metadata/snowflake.py (1)
51-52: Good addition for semi-structured type support.The mappings enable proper handling of Snowflake's semi-structured types:
variant→VARIANT: Direct mapping to the newly added enum memberobject→JSON: Treats Snowflake's OBJECT (key-value pairs) as JSON, which is semantically appropriateibis-server/tools/query_local_run.py (2)
18-18: LGTM! Proper import for Snowflake support.The import follows the existing pattern and is used correctly in the conditional branch below.
108-110: LGTM! Snowflake integration follows established patterns.The implementation correctly:
- Validates connection info using
SnowflakeConnectionInfo.model_validate_json()- Obtains the connection via
DataSourceExtension.get_snowflake_connection()- Follows the same structure as other data source branches
|
There are some issues about pulling the Oracle testing image (always timeout 🤔), but it doesn't matter for this PR. |
|
Thanks @goldmedal |
Summary by CodeRabbit
New Features
Bug Fixes