Skip to content
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
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Features-20231110-154255.yaml
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"
3 changes: 3 additions & 0 deletions core/dbt/contracts/graph/model_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from dbt.exceptions import DbtInternalError, CompilationError
from dbt import hooks
from dbt.node_types import NodeType, AccessType
from dbt_semantic_interfaces.type_enums.export_destination_type import ExportDestinationType
from mashumaro.jsonschema.annotations import Pattern


Expand Down Expand Up @@ -407,6 +408,8 @@ class SavedQueryConfig(BaseConfig):
default_factory=dict,
metadata=MergeBehavior.Update.meta(),
)
export_as: Optional[ExportDestinationType] = None
schema: Optional[str] = None
Copy link
Contributor

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?

Copy link
Contributor Author

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

We didn't add the ExportConfig alias property to the SavedQueryConfig. This
is because alias will always be specific to a single export, and thus it doesn't
make sense to allow defining it on the SavedQueryConfig to then apply to all
Exports belonging to the SavedQuery

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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 🙃

Copy link
Contributor

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

Copy link
Contributor

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



@dataclass
Expand Down
12 changes: 1 addition & 11 deletions core/dbt/contracts/graph/unparsed.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
from dbt.exceptions import CompilationError, ParsingError, DbtInternalError

from dbt.dataclass_schema import dbtClassMixin, StrEnum, ExtensibleDbtClassMixin, ValidationError
from dbt_semantic_interfaces.type_enums.export_destination_type import ExportDestinationType

from dataclasses import dataclass, field
from datetime import timedelta
Expand Down Expand Up @@ -729,21 +728,12 @@ class UnparsedQueryParams(dbtClassMixin):
where: Optional[Union[str, List[str]]] = None


@dataclass
class UnparsedExportConfig(dbtClassMixin):
"""Nested configuration attributes for exports."""

export_as: ExportDestinationType
schema: Optional[str] = None
alias: Optional[str] = None


@dataclass
class UnparsedExport(dbtClassMixin):
"""Configuration for writing query results to a table."""

name: str
config: UnparsedExportConfig
config: Dict[str, Any] = field(default_factory=dict)


@dataclass
Expand Down
31 changes: 20 additions & 11 deletions core/dbt/parser/schema_yaml_readers.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
UnparsedDimensionTypeParams,
UnparsedEntity,
UnparsedExport,
UnparsedExportConfig,
UnparsedExposure,
UnparsedGroup,
UnparsedMeasure,
Expand All @@ -19,6 +18,7 @@
UnparsedSavedQuery,
UnparsedSemanticModel,
)
from dbt.contracts.graph.model_config import SavedQueryConfig
from dbt.contracts.graph.nodes import (
Exposure,
Group,
Expand Down Expand Up @@ -57,7 +57,7 @@
MetricType,
TimeGranularity,
)
from typing import List, Optional, Union
from typing import Any, Dict, List, Optional, Union


def parse_where_filter(
Expand Down Expand Up @@ -669,16 +669,25 @@

return config

def _get_export_config(self, unparsed: UnparsedExportConfig) -> ExportConfig:
return ExportConfig(
export_as=unparsed.export_as,
schema_name=unparsed.schema,
alias=unparsed.alias,
def _get_export_config(
self, unparsed: Dict[str, Any], saved_query_config: SavedQueryConfig
MichelleArk marked this conversation as resolved.
Show resolved Hide resolved
) -> ExportConfig:
# Combine the two dictionaries using dictionary unpacking
# the second dictionary is the one whose keys take priority
combined = {**saved_query_config.__dict__, **unparsed}

Check warning on line 677 in core/dbt/parser/schema_yaml_readers.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/parser/schema_yaml_readers.py#L677

Added line #L677 was not covered by tests
# `schema` is the user facing attribute, but for DSI protocol purposes we track it as `schema_name`
if combined.get("schema") is not None and combined.get("schema_name") is None:
combined["schema_name"] = combined["schema"]

Check warning on line 680 in core/dbt/parser/schema_yaml_readers.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/parser/schema_yaml_readers.py#L679-L680

Added lines #L679 - L680 were not covered by tests

return ExportConfig.from_dict(combined)

Check warning on line 682 in core/dbt/parser/schema_yaml_readers.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/parser/schema_yaml_readers.py#L682

Added line #L682 was not covered by tests

def _get_export(
self, unparsed: UnparsedExport, saved_query_config: SavedQueryConfig
) -> Export:
return Export(

Check warning on line 687 in core/dbt/parser/schema_yaml_readers.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/parser/schema_yaml_readers.py#L687

Added line #L687 was not covered by tests
name=unparsed.name, config=self._get_export_config(unparsed.config, saved_query_config)
)

def _get_export(self, unparsed: UnparsedExport) -> Export:
return Export(name=unparsed.name, config=self._get_export_config(unparsed.config))

def _get_query_params(self, unparsed: UnparsedQueryParams) -> QueryParams:
return QueryParams(
group_by=unparsed.group_by,
Expand Down Expand Up @@ -721,7 +730,7 @@
resource_type=NodeType.SavedQuery,
unique_id=unique_id,
query_params=self._get_query_params(unparsed.query_params),
exports=[self._get_export(export) for export in unparsed.exports],
exports=[self._get_export(export, config) for export in unparsed.exports],
config=config,
unrendered_config=unrendered_config,
group=config.group,
Expand Down
67 changes: 67 additions & 0 deletions tests/functional/saved_queries/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,70 @@
export_as: table
schema: my_export_schema_name
"""

saved_query_with_extra_config_attributes_yml = """
version: 2

saved_queries:
- name: test_saved_query
description: "{{ doc('saved_query_description') }}"
label: Test Saved Query
query_params:
metrics:
- simple_metric
group_by:
- "Dimension('user__ds')"
where:
- "{{ Dimension('user__ds', 'DAY') }} <= now()"
- "{{ Dimension('user__ds', 'DAY') }} >= '2023-01-01'"
exports:
- name: my_export
config:
my_random_config: 'I have this for some reason'
export_as: table
"""

saved_query_with_export_configs_defined_at_saved_query_level_yml = """
version: 2

saved_queries:
- name: test_saved_query
description: "{{ doc('saved_query_description') }}"
label: Test Saved Query
config:
export_as: table
schema: my_default_export_schema
query_params:
metrics:
- simple_metric
group_by:
- "Dimension('user__ds')"
where:
- "{{ Dimension('user__ds', 'DAY') }} <= now()"
- "{{ Dimension('user__ds', 'DAY') }} >= '2023-01-01'"
exports:
- name: my_export
config:
export_as: view
schema: my_custom_export_schema
- name: my_export2
"""

saved_query_without_export_configs_defined_yml = """
version: 2

saved_queries:
- name: test_saved_query
description: "{{ doc('saved_query_description') }}"
label: Test Saved Query
query_params:
metrics:
- simple_metric
group_by:
- "Dimension('user__ds')"
where:
- "{{ Dimension('user__ds', 'DAY') }} <= now()"
- "{{ Dimension('user__ds', 'DAY') }} >= '2023-01-01'"
exports:
- name: my_export
"""
186 changes: 186 additions & 0 deletions tests/functional/saved_queries/test_configs.py
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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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