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

"if not target_relation" is not true even if relation does not exist in get_column_values #444

Closed
1 task done
mothiki opened this issue Nov 18, 2021 · 5 comments · Fixed by #448
Closed
1 task done
Labels
bug Something isn't working pending

Comments

@mothiki
Copy link

mothiki commented Nov 18, 2021

Describe the bug

In dbt_utils<0.7.4, while using get_column_values macro, dbt compile on a dummy schema that doesn't exist in the database yet, returns the default values specified in the macro.
But, In dbt_utils==0.7.4, dbt compile is trying to query the column values even though the table does not exist yet and throws schema <dummy_schema_1111> does not exist error.

Steps to reproduce

Step 1:
This is a sample model that we have.

{{ config(materialized='view', enabled=true) }}

{%- set activity_relation= ref('stg_sf__activities') -%}

with pivot_activity_type as (

  select
    sf_account_id,
    reporting_date,
    {{
      dbt_utils.pivot(
        'activity_type',
        dbt_utils.get_column_values(
          activity_relation,
          'activity_type',
          default=['email', 'call']
        ),
      prefix= 'activity_type_'
      )
    }}

    from {{activity_relation}}

    group by sf_account_id, reporting_date
)

select * from pivot_activity_type

Step 2:
Set schema in profiles.yml to a dummy value.

Step 3:
run dbt compile

Expected results

Previously the macro has:

{%- set target_relation = adapter.get_relation(database=table.database,
                                          schema=table.schema,
                                         identifier=table.identifier) -%} 

which sets target_relation to null when it doesn't exist and the code return default values ['email', 'call'] and the compilation succeeds without querying the database.

Actual results

Now since the code is changed to:

{%- set target_relation = table -%}

it returns schema does not exist error.

System information

The contents of your packages.yml file:

packages:
  - package: dbt-labs/dbt_utils
     version: 0.7.4

Which database are you using dbt with?

  • redshift

The output of dbt --version:

installed version: 0.21.0
  latest version: 0.21.0

Up to date!

Plugins:
  - redshift: 0.21.0
  - postgres: 0.21.0

Additional context

The code doesn't go to the below block even if the relation doesn't exist in the database:

        {%- elif not target_relation and default is not none -%}

          {{ log("Relation " ~ table ~ " does not exist. Returning the default value: " ~ default) }}

          {{ return(default) }}

Are you interested in contributing the fix?

Yes, I would love to contribute.

@mothiki mothiki added bug Something isn't working triage labels Nov 18, 2021
@joellabes
Copy link
Contributor

Thanks for the great writeup and repro case @mothiki!

@jtcohen6 I made this change in #440 to solve #424. Can you think of any way of squaring this circle and supporting both use cases?

Potentially we need to still use the adapter.get_relation call but wrap every piece of database, schema and table in a something that quotes each component correctly? I could have sworn there was a function like adapter.quote but I don't see it on the docs page.

If we have to choose only one winner, I think we have to leave this as-is and put the onus on the macro user to create their own Relation if they think it may not exist. I don't like it but I think it's more important to allow people to access Relations that do exist, no matter what weird quoting they have, than support not accessing Relations that don't.

@mothiki is the relation not existing yet a common use case for you, or something that bit you as a one-off? (Perhaps unintuitively, if it's a one-off I feel worse about leaving it in a broken state.)

@joellabes joellabes added pending and removed triage labels Nov 18, 2021
@jtcohen6
Copy link
Contributor

jtcohen6 commented Nov 18, 2021

Oof, lots going on for this one! Thanks for wading through the muck to get here.

It sounds like we need to find a way to do all of the following:

  • use the existing relation, with all its quoting particularities, rather than creating a totally new one
  • see if that existing relation exists in the database
  • if it does, keep using it
  • if it doesn't, return the default value instead

So perhaps something like:

{%- set existing_relation = load_relation(table) -%} {# this is shorthand for get_relation() #}

{%- if existing_relation -%}
    {# it exists, use it! #}
    {%- set target_relation = table -%}
{%- else -%}
    {# it doesn't exist, so we'll opt for the default further down #}
    {%- set target_relation = None -%}
{%- endif -%}

@mothiki
Copy link
Author

mothiki commented Nov 18, 2021

@joellabes @jtcohen6, Thanks for the detailed explanation.
Our CI pipelines have a compile stage on every push to test code integrity. And, run -> test -> drop schema stages on a merge request. All this happens on a dynamic schema generated using username_pipelineid which will not exist in DB yet.
get_column_values is used in only the example model I specified above. I am able to replace get_column_values with a custom macro on our end and everything works well. I do not mind leaving it how it is right now. We can ignore this if no other user has an issue.

@davidwitk
Copy link

davidwitk commented Nov 22, 2021

Only for visibility: We are facing the same issue, as we use the default value introduced by @emilieschario for this purpose. For now, I will go ahead and use a custom macro, but will follow what is going on here

@joellabes
Copy link
Contributor

Thanks @chodera! Intending to get a bugfix release out for this in the next week or so 🤞

Worth noting that you can also go back to 0.7.3 in the meantime!

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

Successfully merging a pull request may close this issue.

4 participants