-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Support hierarchical config setting for SavedQueryExport configs #9065
Merged
QMalcolm
merged 6 commits into
main
from
qmalcolm--8956-hierarchical-saved-query-export-config-support
Nov 14, 2023
Merged
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
1eaccb7
Add test asserting `SavedQuery` configs can be set from `dbt_project.…
QMalcolm 8e1b4b3
Allow extraneous properties in Export configs
QMalcolm 3fd6f82
Add `ExportConfig` options to `SavedQueryConfig` options
QMalcolm b67a38b
Begin inheriting configs from saved query config, and transitively fr…
QMalcolm dd3be16
Correct conditional in export config building for map schema to schem…
QMalcolm 5ec2363
Update parameter names in `_get_export_config` to be more verbose
QMalcolm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: Features | ||
body: Support setting export configs hierarchically via saved query and project configs | ||
time: 2023-11-10T15:42:55.042317-08:00 | ||
custom: | ||
Author: QMalcolm | ||
Issue: "8956" |
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,186 @@ | ||
import pytest | ||
|
||
from dbt.contracts.graph.manifest import Manifest | ||
from dbt.tests.util import update_config_file | ||
from dbt_semantic_interfaces.type_enums.export_destination_type import ExportDestinationType | ||
from tests.functional.assertions.test_runner import dbtTestRunner | ||
from tests.functional.configs.fixtures import BaseConfigProject | ||
from tests.functional.saved_queries.fixtures import ( | ||
saved_queries_yml, | ||
saved_query_description, | ||
saved_query_with_extra_config_attributes_yml, | ||
saved_query_with_export_configs_defined_at_saved_query_level_yml, | ||
saved_query_without_export_configs_defined_yml, | ||
) | ||
from tests.functional.semantic_models.fixtures import ( | ||
fct_revenue_sql, | ||
metricflow_time_spine_sql, | ||
schema_yml, | ||
) | ||
|
||
|
||
class TestSavedQueryConfigs(BaseConfigProject): | ||
@pytest.fixture(scope="class") | ||
def project_config_update(self): | ||
return { | ||
"saved-queries": { | ||
"test": { | ||
"test_saved_query": { | ||
"+enabled": True, | ||
"+export_as": ExportDestinationType.VIEW.value, | ||
"+schema": "my_default_export_schema", | ||
} | ||
}, | ||
}, | ||
} | ||
|
||
@pytest.fixture(scope="class") | ||
def models(self): | ||
return { | ||
"saved_queries.yml": saved_query_with_extra_config_attributes_yml, | ||
"schema.yml": schema_yml, | ||
"fct_revenue.sql": fct_revenue_sql, | ||
"metricflow_time_spine.sql": metricflow_time_spine_sql, | ||
"docs.md": saved_query_description, | ||
} | ||
|
||
def test_basic_saved_query_config( | ||
self, | ||
project, | ||
): | ||
runner = dbtTestRunner() | ||
|
||
# parse with default fixture project config | ||
result = runner.invoke(["parse"]) | ||
assert result.success | ||
assert isinstance(result.result, Manifest) | ||
assert len(result.result.saved_queries) == 1 | ||
saved_query = result.result.saved_queries["saved_query.test.test_saved_query"] | ||
assert saved_query.config.export_as == ExportDestinationType.VIEW | ||
assert saved_query.config.schema == "my_default_export_schema" | ||
|
||
# disable the saved_query via project config and rerun | ||
config_patch = {"saved-queries": {"test": {"test_saved_query": {"+enabled": False}}}} | ||
update_config_file(config_patch, project.project_root, "dbt_project.yml") | ||
result = runner.invoke(["parse"]) | ||
assert result.success | ||
assert len(result.result.saved_queries) == 0 | ||
|
||
|
||
class TestExportConfigsWithAdditionalProperties(BaseConfigProject): | ||
@pytest.fixture(scope="class") | ||
def models(self): | ||
return { | ||
"saved_queries.yml": saved_queries_yml, | ||
"schema.yml": schema_yml, | ||
"fct_revenue.sql": fct_revenue_sql, | ||
"metricflow_time_spine.sql": metricflow_time_spine_sql, | ||
"docs.md": saved_query_description, | ||
} | ||
|
||
def test_extra_config_properties_dont_break_parsing(self, project): | ||
runner = dbtTestRunner() | ||
|
||
# parse with default fixture project config | ||
result = runner.invoke(["parse"]) | ||
assert result.success | ||
assert isinstance(result.result, Manifest) | ||
assert len(result.result.saved_queries) == 1 | ||
saved_query = result.result.saved_queries["saved_query.test.test_saved_query"] | ||
assert len(saved_query.exports) == 1 | ||
assert saved_query.exports[0].config.__dict__.get("my_random_config") is None | ||
|
||
|
||
class TestInheritingExportConfigFromSavedQueryConfig(BaseConfigProject): | ||
@pytest.fixture(scope="class") | ||
def models(self): | ||
return { | ||
"saved_queries.yml": saved_query_with_export_configs_defined_at_saved_query_level_yml, | ||
"schema.yml": schema_yml, | ||
"fct_revenue.sql": fct_revenue_sql, | ||
"metricflow_time_spine.sql": metricflow_time_spine_sql, | ||
"docs.md": saved_query_description, | ||
} | ||
|
||
def test_export_config_inherits_from_saved_query(self, project): | ||
runner = dbtTestRunner() | ||
|
||
# parse with default fixture project config | ||
result = runner.invoke(["parse"]) | ||
assert result.success | ||
assert isinstance(result.result, Manifest) | ||
assert len(result.result.saved_queries) == 1 | ||
saved_query = result.result.saved_queries["saved_query.test.test_saved_query"] | ||
assert len(saved_query.exports) == 2 | ||
|
||
# assert Export `my_export` has its configs defined from itself because they should take priority | ||
export1 = next( | ||
(export for export in saved_query.exports if export.name == "my_export"), None | ||
) | ||
assert export1 is not None | ||
assert export1.config.export_as == ExportDestinationType.VIEW | ||
assert export1.config.export_as != saved_query.config.export_as | ||
assert export1.config.schema_name == "my_custom_export_schema" | ||
assert export1.config.schema_name != saved_query.config.schema | ||
|
||
# assert Export `my_export` has its configs defined from the saved_query because they should take priority | ||
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. ✨ |
||
export2 = next( | ||
(export for export in saved_query.exports if export.name == "my_export2"), None | ||
) | ||
assert export2 is not None | ||
assert export2.config.export_as == ExportDestinationType.TABLE | ||
assert export2.config.export_as == saved_query.config.export_as | ||
assert export2.config.schema_name == "my_default_export_schema" | ||
assert export2.config.schema_name == saved_query.config.schema | ||
|
||
|
||
class TestInheritingExportConfigsFromProject(BaseConfigProject): | ||
@pytest.fixture(scope="class") | ||
def project_config_update(self): | ||
return { | ||
"saved-queries": { | ||
"test": { | ||
"test_saved_query": { | ||
"+export_as": ExportDestinationType.VIEW.value, | ||
} | ||
}, | ||
}, | ||
} | ||
|
||
@pytest.fixture(scope="class") | ||
def models(self): | ||
return { | ||
"saved_queries.yml": saved_query_without_export_configs_defined_yml, | ||
"schema.yml": schema_yml, | ||
"fct_revenue.sql": fct_revenue_sql, | ||
"metricflow_time_spine.sql": metricflow_time_spine_sql, | ||
"docs.md": saved_query_description, | ||
} | ||
|
||
def test_export_config_inherits_from_project( | ||
self, | ||
project, | ||
): | ||
runner = dbtTestRunner() | ||
|
||
# parse with default fixture project config | ||
result = runner.invoke(["parse"]) | ||
assert result.success | ||
assert isinstance(result.result, Manifest) | ||
assert len(result.result.saved_queries) == 1 | ||
saved_query = result.result.saved_queries["saved_query.test.test_saved_query"] | ||
assert saved_query.config.export_as == ExportDestinationType.VIEW | ||
|
||
# change export's `export_as` to `TABLE` via project config | ||
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. ✨ |
||
config_patch = { | ||
"saved-queries": { | ||
"test": {"test_saved_query": {"+export_as": ExportDestinationType.TABLE.value}} | ||
} | ||
} | ||
update_config_file(config_patch, project.project_root, "dbt_project.yml") | ||
result = runner.invoke(["parse"]) | ||
assert result.success | ||
assert isinstance(result.result, Manifest) | ||
assert len(result.result.saved_queries) == 1 | ||
saved_query = result.result.saved_queries["saved_query.test.test_saved_query"] | ||
assert saved_query.config.export_as == ExportDestinationType.TABLE |
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.
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.
are we dropping support for alias?
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 intentionally left out alias because including it felt weird 🙃 If a saved query has multiple exports, I don't think we'd want them to all get aliased to to same thing.
I didn't feel there was a great place to leave a comment about it (feels weird to comment in a class about why it doesn't have an attribute), so I put the informartion in the commit message
Happy to add a comment to the class if that feels like the correct approach. Alternatively, if we do want to add alias we can, although I do worry that could cause other problems.
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.
Makes sense. Have we documented
alias
in user documentation yet? If so it might make sense to support some no-op backwards-compatible interface here given we are backporting this change.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 will run through it with @graciegoheen / @jtcohen6. As of right now exports as a whole don't appear to be documented https://docs.getdbt.com/docs/build/saved-queries 🙃
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.
That's right! We closed out this docs issue until exports is fully ready to go dbt-labs/docs.getdbt.com#4381
Relevant slack thread here -> https://dbt-labs.slack.com/archives/C05K4R7KZ5Z/p1699478482262759
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.
@QMalcolm Here's one new PR for saved queries:
dbt-labs/docs.getdbt.com#4469