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

Change the default behavior of get_show_sql to avoid wrapping in subquery #207

Closed
jtcohen6 opened this issue May 14, 2024 · 2 comments · Fixed by #249
Closed

Change the default behavior of get_show_sql to avoid wrapping in subquery #207

jtcohen6 opened this issue May 14, 2024 · 2 comments · Fixed by #249
Assignees
Labels
enhancement New feature or request

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented May 14, 2024

Context:

Implementation:

  • By default, get_show_sql should not wrap in a subquery. Instead, it should simply append limit {{ limit }} to the end of the query.
  • This does not work for queries that already contain a limit clause, but it preserves ordering in all other cases.
  • Because this only changes the behavior for dbt show, which is a development (not production) workflow, I do not believe this needs to be gated behind a behavior change flag. Users can always opt back into the previous behavior by reimplementing get_show_sql within their projects.
{% macro get_show_sql(compiled_code, sql_header, limit) -%}
  {%- if sql_header -%}
  {{ sql_header }}
  {%- endif -%}
  {{ compiled_code }}
  {% if limit is not none %}
  limit {{ limit }}
  {%- endif -%}
{% endmacro %}

Acceptance tests:

  • I expect all existing tests to pass, since BaseShowLimit.test_limit is validating only that the limit is templated into the query (important!)
  • Ideally, we'd write a new test that reliably reproduces incorrect ordering when the query is wrapped in a subquery, and then prove that it is always correctly sorted. Unfortunately I haven't managed to come up with a simple reproduction case of this.
@jtcohen6 jtcohen6 added the enhancement New feature or request label May 14, 2024
@jtcohen6
Copy link
Contributor Author

From estimation:

  • existing macro, same signature
  • not reimplemented in most adapters
  • could add a test for the known failure mode (limit repeated twice: once within actual query, once by supplying --limit flag)

@VersusFacit
Copy link
Contributor

See merge comments in #team-dev-adapters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants