[DBA-297] Fix Snowflake adapter to use secret in ATTACH instead of connection string#274
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes the Snowflake → DuckDB registration path so DuckDB’s Snowflake SECRET is actually used by ATTACH, instead of embedding credentials in the connection string (which made the secret unused).
Changes:
- Switch Snowflake DuckDB registration from connection-string
ATTACHtoCREATE OR REPLACE SECRET+ATTACH ... SECRET. - Refactor secret creation to return params dict (
_create_secret_params) and add_format_sql_paramsfor SQL rendering. - Update Snowflake adapter tests and the Snowflake example script to match the new flow.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
databao/agent/databases/snowflake_adapter.py |
Wire DuckDB secret into Snowflake ATTACH; refactor secret param generation/formatting. |
tests/test_snowflake_adapter.py |
Update tests to assert secret params dict instead of parsing generated SQL. |
examples/snowflake-example.py |
Update connection URL and simplify domain creation to match current usage. |
Comments suppressed due to low confidence (1)
databao/agent/databases/snowflake_adapter.py:146
config.additional_propertiesis no longer propagated when registering the Snowflake connection in DuckDB. Previously_create_connection_stringmerged**config.additional_properties, so extra Snowflake connector options (e.g., session/timeout settings) would be honored; with the secret-based flow they’re silently dropped, which is a functional regression for users relying on them. Consider mergingconfig.additional_propertiesinto the secret params (casting values to strings as needed) or passing them via theATTACHoptions if DuckDB expects them there.
def _create_secret_params(config: SnowflakeConnectionProperties) -> dict[str, str]:
params: dict[str, str] = {
ACCOUNT_KEY: config.account,
}
if config.user:
params[USER_KEY] = config.user
if config.database:
params[DATABASE_KEY] = config.database
if config.warehouse:
params[WAREHOUSE_KEY] = config.warehouse
if config.role:
params["role"] = config.role
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…arams and add corresponding test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Snowflake adapter created a DuckDB secret via
CREATE OR REPLACE SECRETbut theATTACHstatement ignored it, passing credentials directly via a connection string instead. The secret was effectively dead code.Changes
Fix: Wire DuckDB secret into ATTACH statement
Replaced the connection-string-based
ATTACHflow with a secret-based one:Before (buggy):
After (fixed):
Affected files
databao/agent/databases/snowflake_adapter.pyexamples/snowflake-example.pytests/test_snowflake_adapter.pyRefactoring
_create_connection_string(no longer needed)_create_secret_sql→_create_secret_params(returns a dict instead of raw SQL)_format_sql_paramshelper_create_secret_paramsdirectlyTest Plan
YouTrack
DBA-297