-
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
[Feature] Avoid wrapping in subqueries where possible and respect order by
clauses with dbt show --limit
#9600
Comments
Thanks for raising this @joellabes ! Agreed, it makes sense that we'd want to respect We'd also want to respect WorkaroundThe standard definition is: {% macro default__get_limit_subquery_sql(sql, limit) %}
select *
from (
{{ sql }}
) as model_limit_subq
limit {{ limit }}
{% endmacro %} You can get the behavior you're after by creating a file within your project named something like {% macro default__get_limit_subquery_sql(sql, limit) %}
{{ sql }}
limit {{ limit }}
{% endmacro %} |
dbt show --limit
doesn't respect order by clauses and should avoid wrapping in subqueries where possibleorder by
clauses with dbt show --limit
ooooh i like that workaround. on my way to propose a PR in internal-analytics 👀
@dbeatty10 can you clarify what you mean here? Aren't preview queries by definition ephemeral (not in the materialization sense) and unbreakable? |
Like you mentioned originally, any query that uses a limit won't work with the macro override above. Here's an example that doesn't work: dbt show --inline "select 1 as id limit 3" |
OK got it! I guess my pitch is that previews don't have to be protected from that happening - if you accidentally wind up issuing |
Hi @dbeatty10 splitting hairs here :-) I don't know if I would say this is a |
Very good points @dsmdavid -- we could reasonably categorize it as any of those. We've taken a couple swings at this:
The latter has interactions with IdealIn the most ideal scenario, we'd just do the following: Add a limit if none exists in the SQL. If a limit already exists in the SQL, and it is less than the provided CLI/Cloud limit, then there's nothing to do. Otherwise, replace the preexisting SQL limit with the CLI/Cloud limit. The devil would be in the implementation details due to the query re-writing, so this could be a large lift. We'd probably opt for a more practical option that is more easy to implement, like below. Less ideal, but more practicalSee below for a not-quite-perfect workaround that seems reasonably practical. There's two variants we could choose between when applying
Is there any way that we have the best of both words without re-writing the query? I don't think so. But we can do better than just choosing one to apply to all scenarios. Rather, we can dynamically choose one or other depending on the situation. ProposalI'd propose that we choose between these two (imperfect!) strategies:
Both variants works great in all scenarios except the following edge case: select 1 as id
order by id
limit 3 Either one will break down in this scenario, but each in a different way: Using variant 1 will lead to a syntax error, whereas for variant 2, the underlying database may clobber the order by clause. CodeCode to override
{% macro get_show_sql(compiled_code, sql_header, limit) -%}
{%- if sql_header -%}
{{ sql_header }}
{%- endif -%}
{%- if limit is not none -%}
{%- if use_limit_subquery(compiled_code) is sameas true -%}
{{ get_limit_subquery_sql(compiled_code, limit) }}
{%- else -%}
{{ get_limit_sql(compiled_code, limit) }}
{%- endif -%}
{%- else -%}
{{ compiled_code }}
{%- endif -%}
{% endmacro %}
{% macro get_limit_subquery_sql(sql, limit) %}
{{ adapter.dispatch('get_limit_subquery_sql', 'dbt')(sql, limit) }}
{% endmacro %}
{% macro default__get_limit_subquery_sql(sql, limit) %}
select *
from (
{{ sql }}
) as model_limit_subq
limit {{ limit }}
{% endmacro %}
{% macro get_limit_sql(sql, limit) %}
{{ adapter.dispatch('get_limit_sql', 'dbt')(sql, limit) }}
{% endmacro %}
{% macro default__get_limit_sql(sql, limit) %}
{{ sql }}
limit {{ limit }}
{% endmacro %}
{% macro use_limit_subquery(compiled_code) %}
{{ adapter.dispatch('use_limit_subquery', 'dbt')(compiled_code) }}
{% endmacro %}
{% macro default__use_limit_subquery(compiled_code) %}
{%- do return('order by' not in compiled_code.lower()) -%}
{% endmacro %} Examples for variant 1Please bear with the "LaTeX" casing I adopted in the example below. This mixed-casing is included to ensure the proposed solution is case-insensitive. The example below demonstrates variant 1. To switch these examples to variant 2, just replace the {% macro use_limit_subquery(compiled_code) %}
{%- do return('limit' in compiled_code.lower()) -%}
{% endmacro %} Happy path 👍The following models work as intended when using
select 1 as id
select *
from (
select 1 as id
) as model_limit_subq
limit 5
select 1 as id
LiMiT 3
select *
from (
select 1 as id
LiMiT 3
) as model_limit_subq
limit 5
select 1 as id
OrDeR bY id
select 1 as id
OrDeR bY id
limit 5
select 1 as id
OrDeR bY id
-- LiMiT 3
select 1 as id
OrDeR bY id
-- LiMiT 3
limit 5 Unhappy path / edge case 👎
select 1 as id
OrDeR bY id
LiMiT 3
select 1 as id
OrDeR bY id
LiMiT 3
limit 5
select 1 as id
-- OrDeR bY id
LiMiT 3
select 1 as id
-- OrDeR bY id
LiMiT 3
limit 5 Making each variant configurableWe could consider making a flag so that users can toggle between which trade-off they want to accept (limit that clobbers |
Enthusiastic about a flag where you can choose your tradeoff! I assume the above sample |
Good point @joellabes ! Yep, this is a perfect example of two competing edge cases. Variant 1 would be best for the As mentioned earlier, the code provided earlier comes with trade-offs that we can't fully get around without fully re-writing the query itself to either overwrite an existing To avoid bewilderment/frustration while wrestling with this, I have to remind myself of the true root cause:
So all these gymnastics are just trying to do our best to workaround that undesirable behavior. Here's a few follow-up ideas that are not mutually exclusive and could be used in combination. Idea 1One idea would be making the "show strategy" configurable somehow on a model-by-model basis. e.g., add a model config like the following:
The available strategies could be named like this (or alternatives):
The last one is to not apply any additional limit at all and just trust that the user is handling limits within the model itself. Idea 2Another idea is to allow users to make assertions about their model via a model-level config: e.g., add a model config like the following:
If we added a model-level config that allows users to be explicit, then we could raise appropriate warnings during inference, like:
Idea 3The real edge case is when both For users that have a limit clause at the end of their model code, we could encourage a pattern like the following that uses either the hard-coded limit in the model or the CLI limit (depending on which is least). Suppose the user had this before: limit 17 Then we'd encourage them to use the limit {{ [17, invocation_args_dict.get("limit", 999)] | min }} Gnarly! 😬 But if the model has this code, then we can bypass adding another limit altogether because the model will already have it. But like in the other scenarios discussed above, we'd probably need the user to explicitly say that they don't need/want anything added to the show query (for this model specifically). i.e., the |
Is this a new bug in dbt-core?
Current Behavior
At the moment, the
limit
flag ondbt show
is achieved by wrapping the main query in a subquery, so:instead of the more idiomatic
Because of this, the output isn't always correctly sorted (screenshot from the Cloud IDE, but behaviour is consistent across Cloud IDE, Cloud CLI and Core CLI)
![image](https://private-user-images.githubusercontent.com/7335046/306092396-50acccac-59a4-4bdf-b302-b0e9e792ab63.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQxMDE0MDIsIm5iZiI6MTcyNDEwMTEwMiwicGF0aCI6Ii83MzM1MDQ2LzMwNjA5MjM5Ni01MGFjY2NhYy01OWE0LTRiZGYtYjMwMi1iMGU5ZTc5MmFiNjMucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDgxOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA4MTlUMjA1ODIyWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9OThkM2ViMjk2Y2JkYTg3MTRjOTk3YTA2OTE4NTkxYjQ0MDMxMjQ4M2M4ODQ0NDZkNTZlMjZkYmQwNDFhNTA3MCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.XXddduYGFXFzQZEeJJG-VAtc7Ieq6AnWwm9g4I1DYqw)
Expected Behavior
Sort order to be respected. The easiest way to do this would be to not wrap in a subquery and instead append the limit directly, as shown above.
I imagine that we're doing it this way to abstract over differences in syntax across adapters (lookin at you, SQL Server)
(Potentially we're also doing it to avoid
dbt show --inline "select * from my_table limit 10" --limit 100
from rendering two limit clauses and causing an error. But IMO if you do that, you should get the error from the DWH and then remove one of your limits.)Steps To Reproduce
Unfortunately it reproduces sporadically - I'd hoped to provide you a basic query like
dbt_utils.generate_series
and sort by that, but I think there's something to do with how each DWH distributes the results that makes this tricky to force into existence.Relevant log output
No response
Environment
- dbt: 1.6.latest and 1.7.0
Which database adapter are you using with dbt?
snowflake
Additional Context
No response
The text was updated successfully, but these errors were encountered: