Remove double underscore in epheremal model generated CTE (__dbt__CTE__...)

[tglunde]

Describe the bug

In Exasol identifiers starting with double underscore are illegal - hence emepheral models fail when using dbt-exasol adapter.

Steps To Reproduce

Configure you project to use dbt-exasol adapter (available on https://pypi.org/project/dbt-exasol/) and then create an ephemeral model. Once you use that model in a ref dbt generates a CTE named "dbt__CTE" which is not a valid identifier in exasol.

Expected behavior

Ephemeral models should work in Exasol. Therefore we suggest to generate those CTE without the leading "__" characters.

System information

Which database are you using dbt with?

• postgres
• redshift
• bigquery
• snowflake
• other (specify: exasol)

The output of dbt --version:

All versions above 0.14.x

The operating system you're using:
ubuntu 18.04.1
The output of python --version:
Python 3.6.9

Additional context

N/A

[jtcohen6]

• The quick, Exasol-friendly answer here is to remove the leading double underscore. I can think of at least one thing that would break, though it's an easy fix.
• I think the better answer, with an eye toward all the other idiosyncratic plugins, is to move add_ephemeral_model_prefix out of core/dbt/utils and onto the adapter. @beckjake do you have a feeling about this option?

That change would feel in line with making schema test definitions (#2415) and data test counting (#2609) logic into adapter macros. We'd written generic SQL that flew fine on every database we could think of, and now we've met some where it doesn't.

[beckjake]

I think the ideal solution to this is actually that dbt support a method or methods on the adapter for inserting CTEs that includes the question of "what prefix to use" as a sort of sub-problem. I think that involves some compiler changes so it accesses adapters, which is fine.

My reasoning here is that some databases don't support CTEs and only support subqueries, and while I don't imagine that behavior ever being a first-class citizen in dbt, it would be nice to give them that mechanism.

edit: Phenomenal timing @jtcohen6

[tglunde]

Created PR and also for dbt-utils to remove double underscore. This is indeed the quick fix. Could we have this merged before thinking of pushing the prefix decision into the adapter?

[beckjake]

I'm sorry, but I'm pretty reluctant to break dbt_utils (again) for a feature that would be a non-breaking change if we implemented it the way we want to implement it in the first place. I understand this is frustrating as a plugin author, and I agree that this is a change that should have been made a long time ago, but here we are. If I'd known about this dbt_utils behavior I would definitely have warned you about it in slack!

This isn't the kind of thing I'd normally suggest, but you might be best off monkey-patching dbt.compilation.add_ephemeral_model_prefix and dbt.providers.add_ephemeral_model_prefix in the adapter's __init__ for now if you're eager to have this working immediately.

If you're committed to the PR path, I think I would be in favor of a PR that does the bare minimum here and makes add_ephemeral_model_prefix an adapter method instead of coming from utils.py. The base adapter could implement it as return '__dbt__CTE_{}'.format(name), and exasol could do the same without the leading __. That's not where we want to be in the end, but it is in the right direction and doesn't break dbt_utils. It's quite likely that the eventual form we do want to implement would be a non-breaking change for adapters after that (even exasol!), which would be wonderful.

[tglunde]

Hi - I am not with you on this definition of breaking change versus non-breaking change. Changing the adapter interface by adding a new, required method is also a breaking change, is it not? dbt-utils will have to be changed anyhow.

…

On Wed, Jul 29, 2020 at 11:32 PM Jacob Beck ***@***.***> wrote: I'm sorry, but I'm pretty reluctant to break dbt_utils (again) for a feature that would be a non-breaking change if we implemented it the way we want to implement it in the first place. I understand this is frustrating as a plugin author, and I agree that this is a change that should have been made a long time ago, but here we are. If I'd known about this dbt_utils behavior I would definitely have warned you about it in slack! This isn't the kind of thing I'd normally suggest, but you might be best off monkey-patching dbt.compilation.add_ephemeral_model_prefix and dbt.providers.add_ephemeral_model_prefix in the adapter's __init__ for now if you're eager to have this working immediately. If you're committed to the PR path, I think I would be in favor of a PR that does the bare minimum here and makes add_ephemeral_model_prefix an adapter method instead of coming from utils.py. The base adapter could implement it as return '__dbt__CTE_{}'.format(name), and exasol could do the same without the leading __. That's not where we want to be in the end, but it is in the right direction and doesn't break dbt_utils. It's quite likely that the eventual form we do want to implement would be a non-breaking change for adapters after that (even exasol!), which would be wonderful. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2660 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMXWIBKIQOG4UJ3Y3CYCOTR6CINNANCNFSM4PL2TFCA> .

-- Torsten Glunde Chief Executive Officer (CEO) Mobil: +49 (0)1702128934 torsten.glunde@alligator-company.de http://www.alligator-company.de Alligator Company Software GmbH . Kirchwender Str. 22 . 30175 Hannover Amtsgericht Hannover HRB 217339 . USt.-IdNr. DE226230162 Geschäftsführer: Torsten Glunde

[beckjake]

If we provide a default implementation in the base adapter then the only adapter that has to override it is exasol (and since CTEs don't work on exasol as it is, really nobody). All the other adapters would inherit that method and behave like they do today.

[tglunde]

So should I close the PR and make a new one with your suggested methods? But dbt-utils still needs to be changed to also use the adapter method, right?

[beckjake]

I think opening a new PR would be reasonable, yeah. We might want to have a method that does the reverse: take a CTE name and return the name it came from as well, or None if it didn't. Then systems like dbt-utils could use that to give useful error messages like this.

I don't think we need to update dbt-utils immediately, but yes fixing it would be a good idea once this is in dbt-core. If a single plugin temporarily reports the wrong model name in a dbt-utils error message when a user incorrectly uses an ephemeral model, I think that is enough of an edge case to be ok!