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

Render sql_header for dbt show #8436

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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/Fixes-20230817-164722.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Fixes
body: Render sql_header before prepending for 'dbt show'
time: 2023-08-17T16:47:22.030206+02:00
custom:
Author: jtcohen6
Issue: "8417"
23 changes: 17 additions & 6 deletions core/dbt/task/show.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
from dbt.task.compile import CompileTask, CompileRunner
from dbt.task.seed import SeedRunner

from dbt.context.providers import generate_runtime_model_context
from dbt.clients.jinja import get_rendered, _HAS_RENDER_CHARS_PAT


class ShowRunner(CompileRunner):
def __init__(self, config, adapter, node, node_index, num_nodes):
Expand All @@ -24,12 +27,20 @@ def execute(self, compiled_node, manifest):
limit = None if self.config.args.limit < 0 else self.config.args.limit

if "sql_header" in compiled_node.unrendered_config:
compiled_node.compiled_code = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we were mutating the compiled_code attribute on compiled_node directly in addition to executing it, whereas the changes here just execute it.

I imagine modifying compiled_node.compiled_code had some downstream effects such as surfacing the sql_header in the target/compiled files. Is the motivation for this change to be more consistent with what target/compiled files typically contain for run models?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the motivation for this change to be more consistent with what target/compiled files typically contain for run models?

Yes, this is what I was thinking. The sql_header isn't part of a node's compiled_code when it's compiled or materialized, so it feels inconsistent that it would be included just for the manifest produced by show.

compiled_node.unrendered_config["sql_header"] + compiled_node.compiled_code
)

adapter_response, execute_result = self.adapter.execute(
compiled_node.compiled_code, fetch=True, limit=limit
sql_header = compiled_node.unrendered_config["sql_header"]
# Does the sql_header contain Jinja and need to be rendered?
# Generating the context will be slower if we don't actually need to render the sql_header (if it contains no Jinja)
if _HAS_RENDER_CHARS_PAT.search(sql_header):
# Currently, we only render sql_header at *parse* time while *running* models
# See dbt-core issues #2793, #3264, #7151
# For simplicity, we will use "generate_runtime_model_context" instead of "generate_parser_model_context"
context = generate_runtime_model_context(compiled_node, self.config, manifest)
import ipdb; ipdb.set_trace()
sql_header = get_rendered(compiled_node.unrendered_config["sql_header"], context)
compiled_code = sql_header + compiled_code

adapter_response, execute_result = self.adapter.execute_macro(
compiled_code, fetch=True, limit=limit
)
end_time = time.time()

Expand Down
9 changes: 8 additions & 1 deletion tests/functional/show/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,20 @@
from {{ ref('sample_model') }}
"""

models__sql_header = """
models__sql_header_no_rendering = """
{% call set_sql_header(config) %}
set session time zone 'Asia/Kolkata';
{%- endcall %}
select current_setting('timezone') as timezone
"""

models__sql_header_yes_rendering = """
{% call set_sql_header(config) %}
set session time zone '{{ var("timezone", "Asia/Kolkata") }}';
{%- endcall %}
select current_setting('timezone') as timezone
"""

private_model_yml = """
groups:
- name: my_cool_group
Expand Down
19 changes: 12 additions & 7 deletions tests/functional/show/test_show.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
models__second_model,
models__ephemeral_model,
schema_yml,
models__sql_header,
models__sql_header_no_rendering,
models__sql_header_yes_rendering,
private_model_yml,
)

Expand All @@ -25,7 +26,8 @@ def models(self):
"sample_number_model_with_nulls.sql": models__sample_number_model_with_nulls,
"second_model.sql": models__second_model,
"ephemeral_model.sql": models__ephemeral_model,
"sql_header.sql": models__sql_header,
"sql_header_no_rendering.sql": models__sql_header_no_rendering,
"sql_header_yes_rendering.sql": models__sql_header_yes_rendering,
}

@pytest.fixture(scope="class")
Expand Down Expand Up @@ -164,13 +166,16 @@ def test_seed(self, project):
(_, log_output) = run_dbt_and_capture(["show", "--select", "sample_seed"])
assert "Previewing node 'sample_seed'" in log_output


class TestShowSqlHeader(ShowBase):
def test_sql_header(self, project):
run_dbt(["build"])
(_, log_output) = run_dbt_and_capture(["show", "--select", "sql_header"])
def test_sql_header_no_rendering(self, project):
(_, log_output) = run_dbt_and_capture(["show", "--select", "sql_header_no_rendering"])
assert "Asia/Kolkata" in log_output

def test_sql_header_yes_rendering(self, project):
(_, log_output) = run_dbt_and_capture(
["show", "--select", "sql_header_yes_rendering", "--vars", "timezone: Europe/Paris"]
)
assert "Europe/Paris" in log_output


class TestShowModelVersions:
@pytest.fixture(scope="class")
Expand Down