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 name of the last with statement #419

Closed
wants to merge 1 commit into from
Closed

change the name of the last with statement #419

wants to merge 1 commit into from

Conversation

TimoKruth
Copy link
Contributor

@TimoKruth TimoKruth commented Sep 20, 2021

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

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 "dispatched" any new macro(s) so non-core adapters can also use them (e.g. the star() source)
  • 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

Final should be changed since there are DBs that use it in their syntax and so this schema test can't run on those DBs. One example is Exasol.
I named it final_counts because it contains the final counts of the two tables we are comparing.
@joellabes
Copy link
Contributor

Hey @TimoKruth, thanks for this PR as well!

In the same vein as my comment on #420 - can you clarify what the motivation is for this change? The dbt Labs style guide is strongly in favour of a final CTE, so I'm not super excited about breaking that convention.

If Exasol has final as a reserved word (I don't have access to an instance so can't query EXA_SQL_KEYWORDS, it might be better to look into a "shim" package as detailed here.

@TimoKruth
Copy link
Contributor Author

TimoKruth commented Oct 13, 2021

Hi @joellabes,

Thank you for your reply.

final is the 162nd reserved and the 267th over all key word on an Exasol if you order them alphabetically. If you want a list of all just ask.

As far as I understand the style guide the use of a final CTE is not referring to the name final in itself but rather to the fact that you should wrap your final output as a CTE to simplify the debugging of your code which I greatly appreciate and will adopt in our own style guide. So in my conclusion it would be in the result equivalent if one would use e.g. final_cte instead of final as the name of the final CTE. Atleast in respect to the aimed goal.

But if it does not work for fishtown to change their styleguide (which in conclusion of my theory would probably be necessary) I will be working with a company intern override of these methods. Or is there a way to bind that override to the DB in use? Like with the db adapters?

Thank you for your time.

@joellabes
Copy link
Contributor

Hi @TimoKruth, thanks for confirming it's a reserved word!

The style guide does call for final "or similar", but in practice it's overwhelmingly final.

It would theoretically be possible to have adapter-specific naming of the CTE, but would create a lot of extra overhead for a database that isn't in scope for dbt-utils. This is where a shim package would come in as discussed above!

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

Successfully merging this pull request may close these issues.

2 participants