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

t-sql hacks that shouldn't break mainline behavior #310

Merged
merged 10 commits into from
Jan 11, 2021

Conversation

dataders
Copy link
Contributor

@dataders dataders commented Dec 23, 2020

This is a:

  • bug fix PR with no breaking changes — please ensure the base branch is master
  • new functionality — please ensure the base branch is the latest dev/ branch
  • a breaking change — please ensure the base branch is the latest dev/ branch

Description & motivation

MSFT's T-SQL has some weird quirks to it such as:

  • subquery aggregate columns need aliases; and
  • there is no boolean type, so there's no such thing as true and false. So in a few schema tests, I've replaced true with 1=1; and,
  • limit 0 at the end of the query translates in TSQL to SELECT TOP (0). If the purpose is to return zero rows, WHERE 0=1 will accomplish the same more universally.

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
    • Redshift
    • Snowflake
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md

@dataders dataders marked this pull request as ready for review December 23, 2020 07:42
@dataders dataders requested a review from clrcrl as a code owner December 23, 2020 07:42
@clrcrl
Copy link
Contributor

clrcrl commented Dec 23, 2020

@jtcohen6 — what do you think of this PR? IIRC we incorporated some similar logic into dbt core since it's non-breaking, but I also know we say "create a shim package" for other adapter functionality

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Edit: I read my GitHub notifications in the wrong order, so now I've reread @swanderz's description and see the work over in #312.

I think the changes proposed in this PR are reasonable:

  • I have no problem with aliasing columns selected in schema tests
  • Switching from true to 1=1 doesn't bother me at all
  • Yes, our SQL style says to use numeric aliases for group by, but we changed the core schema tests in exactly this way, for exactly this reason

The only reason to not make these changes would be if it breaks these macros for another adapter that we don't know about. I think it's really unlikely that there's a database which doesn't support column aliasing or 1=1, though. And we should still move ahead with dispatch, too.

My original response:

For precedent's sake, what I'd rather do here is add dispatch functionality to each of these macros so that they can be rewritten/overridden in a shim package. I.e. replace

{% macro test_at_least_one(model) %}

with

{% macro test_at_least_one(model) %}
  {{ adapter.dispatch('test_at_least_one', packages = dbt_utils._get_utils_namespaces())(model, **kwargs) }}
{% endmacro %}

{% macro default__test_at_least_one(model) %}

For that matter, we should support dispatching for every macro defined in the package, even if all of Postgres/Redshift/Snowflake/BigQuery are happy with the default__ implementation.

@dataders dataders requested a review from jtcohen6 January 8, 2021 01:44
@@ -77,5 +77,5 @@ Instead, we'll manually check that the values of these dictionaries are equivale
{% endif %}

{{ log(ns.err_msg, info=True) }}
select 1 {% if ns.pass %} limit 0 {% endif %}
select 1 as col_name {% if ns.pass %} where 0=1 {% endif %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ouch. looks at if TSQL and BQ don't play nice together. From CircleCI

Query without FROM clause cannot have a WHERE clause

Copy link
Contributor

Choose a reason for hiding this comment

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

Gross. Perhaps we do need a dispatched macro {{ limit_zero() }} after all, just for the sake of integration tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where would limit_zero() live? as a macro just for integration tests, integration_tests/macros? could it be properly dispatched there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it could live in integration_tests/macros, and look like:

{% macro limit_zero() %}
    {{ return(adapter.dispatch('limit_zero', dbt_utils._get_utils_namespaces())()) }}
{% endmacro %}

{% macro default__limit_zero() %}
    {{ return('limit 10') }}
{% endmacro %}

i.e. It dispatches via the dbt_utils_dispatch_list var for namespace purposes, but it doesn't actually "live" in / ship with the dbt_utils package.

Then, you should be able to define:

{% macro sqlserver__limit_zero() %}
  {{ return('where 0=1') }} 
{% endmacro %}

This is the same thing as, or very similar to, what we did with prep_external over in the external tables package

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jan 8, 2021

@swanderz As for the current error, I think you just need to add the following line here:

vars:
  dbt_utils_dispatch_list: ['dbt_utils_integration_tests']

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

looks good! could you add a changelog entry to this one as well?

@dataders
Copy link
Contributor Author

looks good! could you add a changelog entry to this one as well?

done! stoked to have both these merged! game changer!

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

Successfully merging this pull request may close these issues.

3 participants