[CT-3430] [Feature] dbt should tell me if a custom macro in my project overrides one in the global project or my adapter

[dataders]

Is this your first time submitting a feature request?

• I have read the expectations for open source contributors
• I have searched the existing issues, and I could not find an existing issue for this feature
• I am requesting a straightforward extension of existing dbt functionality, rather than a Big Idea better suited to a discussion

Describe the feature

today, users can name custom macros whatever they like, even using the same names as those defined within dbt-core's global project! We call that overriding, and laud it as a feature rather than a bug.

However, there be dragons here. One way to keep the dragons at bay would for dbt to at least warn in stdout that there exists a name collision and that a core macro is being overridden.

Describe alternatives you've considered

disallow use of reserved macro names entirely?

Who will this benefit?

dbt-core#8753 is one example.

@danielmcauley shared another in a #db-snowflake thread where when upgrading from 1.5 to 1.7 he discovers that the root cause is that their project contains a drop_table() macro that now in 1.7 is now overriding a newly introduced global project macro.

Are you interested in contributing this feature?

sure

Anything else?

No response

[dbeatty10]

Dragons, indeed 🐉 -- thanks for raising this @dataders.

If we emit a warning to the logs, a user could use the warn_error_options config to exclude that warning. But it would be an all-or-nothing thing. i.e., if a user has silenced this warning and then creates a new macro that overrides a built-in, then it will just override it silently (which is just the same as the current behavior). Have you thought if there's a way we could opt-in/out on a macro-by-macro basis?

[Updated above to reflect that --warn-error-options is only for turning warnings into errors, not silencing warnings entirely]

Some other questions:

• what will happen if we start emitting these warnings?
• what will happen if we don't emit any warnings?

[jtcohen6]

It is of course a feature that users can override/reimplement global macros, as a way to plug into & change the behavior of dbt-core / adapters with just user-space code.

I agree there ought to be a better & clearer way for a user to say: "I know what I'm doing, and I mean to be doing it."

If we start raising a warning, we also need:

• A way for users to silence it, for specific macros they intend to override global ones. (Is this a macro property? A decorator on the macro definition itself?)
• A way for existing projects to opt out of seeing the warning at all, thereby giving them time to work through silencing ^those warnings for intended overrides

[joellabes]

Here's a thread of someone who got burned by this, because a developer predating them changed the macro's behaviour.

It makes sense to be able to suppress the warnings in day to day use, but maybe dbt debug or similar should be able to list out any overridden Core macros so that when someone else comes along one day they have a chance at working it out.

[MichelleArk]

From refinement:

• How do we know which macros are coming from the adapter? Package name + name of current running adapter
• This would apply for any macros, as well as materialization ones
• Would be best as parse-time warning, but would need to have all macros parsed in order to do that. Feels like a global check
• We should not raise a warning if an adapter overrides a global macro, just for installed packages
• What should be the interplay between this warning and the dispatch config? If the override is based on opt-in dispatch, it would probably be annoying to see this warning.

[jtcohen6]

From discussion with @graciegoheen @MichelleArk @dbeatty10 the other day: Having only one warning that's either "on" or "silenced" doesn't feel granular enough. If I've silenced the warning, I don't find out about any new collisions!

I think there are two stories here:

• As a user upgrading dbt-core, I want to find out proactively about collisions when a new global macro has the same name as a macro I've defined in my project. (This is similar to "reserved" words in DWHs.)
• As a superuser intentionally reimplementing a global macro, I want a clear way to say, "I know what I'm doing"

Ideas:

1. Create a separate warning event per macro. Every time the user reimplements a global macro, they'll also need to silence the specific warning for that macro.
2. Introduce new syntax for macros defined in the root project that knowingly override a macro from the global namespace. By providing this syntax ("I know what I'm doing"), they should not raise a warning. Ideas:

{% macro global current_timestamp() %}
...
{% macro dbt.current_timestamp() %}
...

What about "global" macros that are redefined in packages, if I've given that package dispatch rights over the dbt namespace? In my opinion, this is me opting in to have that package override these macros (generate_schema_name, create_table_as, etc), and I wouldn't expect to see a warning for each override.

[MichelleArk]

Partially resolved by: #9981, which addresses the materialization macros component of this feature request and the will go out in 1.8 and will also be backported to 1.6 and 1.7.

[ChenyuLInx]

Hi all, sorry that I kept missing this issue. Just did some digging in on the issue and came to the following conclusion:

• We can do a parsing warning for all macros in root project that will overwrite dbt macros and package pretty easily
  • in order to do that, we can just do the check when we construct the namespace here
• We can provide a flag to disable all warnings pretty easliy
  • this will just stop the check we add in the previous step
• We can let the user define a specific set macros that they don't want to raise error on. There are some plumbing to do but not to bad
  • we add some a new field in dbt_project.yml, value being a list of macros, we skip checking these in step 1
• We can add special syntax so the overwrite is defined at where the macros are defined(idea 2 here), this seems like a big change.
  • this requires use to implement jinja extension for parsing macros(something like this)

Any thoughts on which step we want to get to in this issue?

[dbeatty10]

Thanks for the deep dive @ChenyuLInx! 🤩

If we can only get these two things done, that would be better than nothing:

• We can do a parsing warning for all macros in root project that will overwrite dbt macros and package pretty easily
  • in order to do that, we can just do the check when we construct the namespace here
• We can provide a flag to disable all warnings pretty easliy
  • this will just stop the check we add in the previous step

Then we could create a separate issue to refine the user experience and technical implementation for these pieces:

• We can let the user define a specific set macros that they don't want to raise error on. There are some plumbing to do but not to bad
  • we add some a new field in dbt_project.yml, value being a list of macros, we skip checking these in step 1
• We can add special syntax so the overwrite is defined at where the macros are defined(idea 2 here), this seems like a big change.
  • this requires use to implement jinja extension for parsing macros(something like this)

Although not the full desired end state, the reason that splitting this seems okay to me:

• Any user that silences this warning would just get the same behavior as they have today
• All other users would gain visibility into which macros are overriding one in the global project

@jtcohen6 and @graciegoheen How would you feel about splitting this along those lines? Would that be acceptable, or do you feel strongly that it should be all-or-nothing in relation to the granularity of silencing on a macro-by-macro basis?

If we do split this into separate issues, we could recommend not silencing this warning unless they'd accept not being warned about any new collisions.

[ChenyuLInx]

talked during estimation: we want to create an opt-in flag to warn about what project root/adapter macros are overwritten by the user.
@graciegoheen what should the name of the flag should be? --strict?