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

[CT-3174] [Bug] Unexpected macro overrides cause mysterious errors #8753

Closed
1 of 2 tasks
tomans-spirent opened this issue Sep 29, 2023 · 7 comments
Closed
1 of 2 tasks
Labels
bug Something isn't working

Comments

@tomans-spirent
Copy link

tomans-spirent commented Sep 29, 2023

Is this a new bug in dbt-core?

  • I believe this is a new bug in dbt-core
  • I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

Updated with more context: I defined a macro in my project that accidentally re-defined a built in macro - and resulted in mysterious behavior.


My scenario is that I have multiple dbt models in postgres - and there is a materialized view that is failing with mysterious compilation error output.

First run works fine.

Subsequent run errors.

Model is just a basic materialized view with a unique index to support concurrent refresh.
There is a second view that depends on the materialized view.

Expected Behavior

This error should not happen, or if it is unrecoverable it should print a helpful debug message.

If it is a user error it should be explained to the user so they can fix their model.

Steps To Reproduce

I do not have a minimal test case available.

I believe the following will be the test case:

Create a dbt-postgres view model that references a materialized view model.

Try to dbt run twice.

Relevant log output

unsupported operand type(s) for +: 'PostgresRelation' and 'str'
  
  > in macro materialized_view_get_build_sql (macros/materializations/models/materialized_view/materialized_view.sql)
  > called by macro materialization_materialized_view_default (macros/materializations/models/materialized_view/materialized_view.sql)
  > called by model ... (......sql)

Environment

- OS: macos ventura 13.6
- Python: 3.10.12
- dbt: 1.6.4

Which database adapter are you using with dbt?

dbt-postgres 1.6.4

Additional Context

No response

@tomans-spirent tomans-spirent added bug Something isn't working triage labels Sep 29, 2023
@github-actions github-actions bot changed the title [Bug] Error in postgres materialized views [CT-3174] [Bug] Error in postgres materialized views Sep 29, 2023
@tomans-spirent
Copy link
Author

tomans-spirent commented Oct 2, 2023

Update on this issue:

I happened to want to define a macro named refresh_materialized_view but so did macros/materializations/models/materialized_view/refresh_materialized_view.sql

This meant anytime the library wanted to invoke its macro it wound up invoking mine instead.

Here are the contents:

{% macro refresh_materialized_view(view_name) %}

{% set sql %}
    refresh materialized view concurrently {{ view_name }};
{% endset %}

{% do run_query(sql) %}
{% do log(view_name + " refreshed", info=True) %}

{% endmacro %}

Now the error makes sense - because the library version is not passing a string like I am expecting.

Maybe I should not have been allowed to define a macro with a reserved name? Happy however you would like to proceed with this issue - for now I have just renamed my macro to resolve my issue.

@dataders dataders self-assigned this Oct 3, 2023
@dataders
Copy link
Contributor

dataders commented Oct 4, 2023

@tomans-spirent thanks for the issue and followup! is my understanding that there isn't a bug in dbt-core, rather the issue had to do with a macro in your project that overrode dbt-core functionality?

the ability to override is a feature today not a bug, but can introduce situations like this

@tomans-spirent tomans-spirent changed the title [CT-3174] [Bug] Error in postgres materialized views [CT-3174] [Bug] Unexpected macro overrides cause mysterious errors Oct 4, 2023
@tomans-spirent
Copy link
Author

tomans-spirent commented Oct 4, 2023

Y'all can close this issue as an acceptable hazard if you prefer.

Probably not a bug but an enhancement.

My belief is that this shouldn't have happened to me - because I was not intending to override existing behavior.

If it was an acceptable risk - I think I should have been guided back to safety by the tool through helpful messages.

I just wanted to make a macro and it mysteriously broke my project because it collided with some internal routine which I had no idea existed - and without my debugging effort would never have found.

For example python duck typing includes __foobar__ names to avoid accidental collision with names like foobar. I understand if a lot of people are overriding foobar today a rename to __foobar__ would be a breaking change.

One option for improvement might be when you run dbt it could say 'your project is overriding the following macros' and provide a list or something.

As an end user I just saw a mysterious runtime error.

@tomans-spirent
Copy link
Author

@dataders

Could you show me the docs for the macro overrides feature? Is it a secret feature? I couldn't find it. I was thinking "the docs could be updated to include a hazard label"

@dataders
Copy link
Contributor

dataders commented Oct 5, 2023

Could you show me the docs for the macro overrides feature? Is it a secret feature? I couldn't find it. I was thinking "the docs could be updated to include a hazard label"

Good callout. I went looking and couldn't find much on the topic beyond the specific example of overriding the generate_schema_name() macro.

We likely should do a better job at this.

That said, my feeling is that perhaps we call this ticket resolved and instead open a new issue that is more targeted. tell me if i'm offbase at understanding your request(s) as the following?

there should be

  1. some warning alert to users when they have macros in their project that are actually "reserved" macros. this alert could be silenced if the users wishes
  2. there should be a naming convention for these macros to prevent unintentional collision
  3. there should be a docs page spelling out the current behavior clearly

if this is the case, i believe that 1 and 2 should be issues on this repo, and the 3rd would be a great candidate issue for the dbt-labs/docs.getdbt.com repo

@tomans-spirent
Copy link
Author

@dataders thanks for your response and attention to this issue

I think your summary is good.

I am happy with documentation somewhere if it could prevent some UX suffering.

The ticket you opened seems nice. I think it could be an addendum in the main macro documentation if it is just part and parcel of how macros work in dbt.

Relevant tooling output is even nicer. I am not clear on the effort required (I had trouble even finding where the original refresh_materialized_view macro I was re-defining was originally defined) - I am happy to say it would be a "nice to have".

I will to close this ticket because there is a workaround for me and anyone else who runs into this issue.

Any future work as a result to improve the user experience is icing on the cake.

Thank you again @dataders

@dataders
Copy link
Contributor

dataders commented Oct 7, 2023

ok great. by all means open the other issues we discussed, if I haven't yet gotten the chance.

also wanted to mention that I remembered another post you might enjoy because it talks about macro-overriding, but also because it has to do with the global_project in general: Did you know? dbt ships with its own project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants