-
Notifications
You must be signed in to change notification settings - Fork 499
dbt-materialize: Support connection overrides #36127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,3 +41,5 @@ prompts: | |
| cluster: | ||
| hint: 'dev cluster' | ||
| default: 'quickstart' | ||
| options: | ||
| hint: 'overrides the PostgreSQL `options` connection parameter' | ||
|
Comment on lines
+44
to
+45
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I totally hear you that this is tricky to unit-test, but do you think we could add an integration test in |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| # Copyright Materialize, Inc. and contributors. All rights reserved. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License in the LICENSE file at the | ||
| # root of this repository, or online at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| class TestConnectionOptionsOverride: | ||
| """Verify that `options` in the dbt profile reaches the server.""" | ||
|
|
||
| @pytest.fixture(scope="class") | ||
| def dbt_profile_target(self): | ||
| return { | ||
| "type": "materialize", | ||
| "threads": 1, | ||
| "host": "{{ env_var('DBT_HOST', 'localhost') }}", | ||
| "user": "materialize", | ||
| "pass": "password", | ||
| "database": "materialize", | ||
| "port": "{{ env_var('DBT_PORT', 6875) }}", | ||
| "options": { | ||
| "welcome_message": "on", | ||
| "auto_route_catalog_queries": "off", | ||
| }, | ||
| } | ||
|
|
||
| def test_options_override(self, project): | ||
| # Override these session variables opposite to their default values | ||
| result = project.run_sql( | ||
| "SELECT current_setting('welcome_message')", fetch="one" | ||
| ) | ||
| assert result[0] == "on" | ||
|
|
||
| result = project.run_sql( | ||
| "SELECT current_setting('auto_route_catalog_queries')", fetch="one" | ||
| ) | ||
| assert result[0] == "off" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new open method copies a chunk of
PostgresConnectionManager.openfromdbt-postgres. Did you consider overriding a smaller surface instead (e.g. just the options-string construction and thensuper().open())? My worry is that whendbt-postgresadds new kwargs or changes retry logic, we'll silently fall behind. Totally fine if you already looked at this and it wasn't workable, just curious.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a valid concern :( I tried to but because kwargs is defined internally and there's no way to override options through the
credentialsobject, we can't use super().open(). The best way I could think of is extending off the previous approach of monkey patching psycopg, then doing some weird python thread blocking such that the monkey patch can accept input from our adapter's.open, but that seems worse than copying their implementation. FWIW prior art to this is something like Clickhouse https://github.com/ClickHouse/dbt-clickhouse/blob/main/dbt/adapters/clickhouse/connections.pyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the correct implementation here however is inheriting off SQLConnectionManager instead of PostgresqlConnectionManager?