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

[Adap-207] remove subshell around get show sql with limit predicate #249

Merged
merged 10 commits into from
Jul 3, 2024
7 changes: 7 additions & 0 deletions .changes/unreleased/Under the Hood-20240624-231955.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: Under the Hood
body: --limit flag no longer subshells the query. This resolves the dbt Cloud experience
issue where limit prevents ordering elements..
time: 2024-06-24T23:19:55.611142-07:00
custom:
Author: versusfacit
Issue: "207"
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,6 @@ cython_debug/

# PyCharm
.idea/

# Vim
*.swp
17 changes: 17 additions & 0 deletions dbt-tests-adapter/dbt/tests/adapter/dbt_show/test_dbt_show.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import pytest

from dbt_common.exceptions import DbtRuntimeError
from dbt.tests.adapter.dbt_show import fixtures
from dbt.tests.util import run_dbt

Expand Down Expand Up @@ -47,9 +48,25 @@ def test_sql_header(self, project):
run_dbt(["show", "--select", "sql_header", "--vars", "timezone: Asia/Kolkata"])


class BaseShowDoesNotHandleDoubleLimit:
"""see issue: https://github.com/dbt-labs/dbt-adapters/issues/207"""

DATABASE_ERROR_MESSAGE = 'syntax error at or near "limit"'

def test_double_limit_throws_syntax_error(self, project):
with pytest.raises(DbtRuntimeError) as e:
run_dbt(["show", "--limit", "1", "--inline", "select 1 limit 1"])

assert self.DATABASE_ERROR_MESSAGE in str(e)


class TestPostgresShowSqlHeader(BaseShowSqlHeader):
pass


class TestPostgresShowLimit(BaseShowLimit):
pass


class TestShowDoesNotHandleDoubleLimit(BaseShowDoesNotHandleDoubleLimit):
pass
34 changes: 19 additions & 15 deletions dbt/include/global_project/macros/adapters/show.sql
Original file line number Diff line number Diff line change
@@ -1,22 +1,26 @@
{#
We expect a syntax error if dbt show is invoked both with a --limit flag to show
and with a limit predicate embedded in its inline query. No special handling is
provided out-of-box.
#}
{% macro get_show_sql(compiled_code, sql_header, limit) -%}
{%- if sql_header -%}
{%- if sql_header is not none -%}
{{ sql_header }}
{%- endif -%}
{%- if limit is not none -%}
{%- endif %}
{{ get_limit_subquery_sql(compiled_code, limit) }}
{%- else -%}
{{ compiled_code }}
{%- endif -%}
{% endmacro %}

{% macro get_limit_subquery_sql(sql, limit) %}
mikealfare marked this conversation as resolved.
Show resolved Hide resolved
{{ adapter.dispatch('get_limit_subquery_sql', 'dbt')(sql, limit) }}
{% endmacro %}
{#
Not necessarily a true subquery anymore. Now, merely a query subordinate
to the calling macro.
#}
{%- macro get_limit_subquery_sql(sql, limit) -%}
{{ adapter.dispatch('get_limit_sql', 'dbt')(sql, limit) }}
{%- endmacro -%}

{% macro default__get_limit_subquery_sql(sql, limit) %}
select *
from (
{{ sql }}
) as model_limit_subq
limit {{ limit }}
{% macro default__get_limit_sql(sql, limit) %}
{{ compiled_code }}

Choose a reason for hiding this comment

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

@VersusFacit and @mikealfare Looks like this line should be using the function parameter {{ sql }}. It works with {{ compiled_code }} I assume because of the context being set in the get_show_sql macro, but that's likely hiding a future bug.

I could be wrong, but ran into this when providing a custom version of get_show_sql in my project.

{% if limit is not none %}
limit {{ limit }}
{%- endif -%}
{% endmacro %}
Loading