Bigquery uniqueness test fails on column name matching table name about dbt-bigquery

[jtcohen6]

@abuckenheimer You're totally right.

This is tougher than it should be today because a test doesn't actually have a good link between itself and the model it's defined on. It's going to be even trickier after the introduction of the where config in v0.20.0 (#3392). If this config is set, instead of templating out {{ model }} as <your dataset>.foo, dbt will template it out as (select * from <your dataset>.foo where col = 1) foo.

At the time, I was thinking that it was important for the subquery to be aliased the same as the model identifier, in case the generic test definition depended on using the model identifier as a column alias. But it wouldn't really be a generic test if it did, now would it?

Here are the options I can think of:

1. Not do this. In general, I don't think it's a good practice for a BigQuery column to share the name of its view/table, for exactly this reason—it causes unpleasant surprises when querying.
2. Change the {{ model }} templating to always alias the selected-from object/subquery as model, whether or not the where config is supplied. In the unique, not_null, and accepted_values tests, qualify columns with the model alias.
3. Add another subquery + alias, to wrap around the subquery + alias associated with the optional where config:

{% macro default__test_unique(model, column_name) %}

select
    model.{{ column_name }},
    count(*) as n_records

from (select * from {{ model }}) model
where model.{{ column_name }} is not null
group by model.{{ column_name }}
having count(*) > 1

{% endmacro %}

Option 3 is something you can do right now, in your own project, to get this working in the meantime. But I'm leaning toward option 2, which would require adjusting this (gnarly) code:

https://github.com/fishtown-analytics/dbt/blob/c794600242e734d870d4bf1c8fb4c0349ab961eb/core/dbt/parser/schema_test_builders.py#L381-L391

In order to always alias, and always use the same alias (just the word model):

    def build_model_str(self): 
        targ = self.target 
        cfg_where = "config.get('where')" 
        alias = "model"
        if isinstance(self.target, UnparsedNodeUpdate): 
            identifier = self.target.name 
            target_str = f"{{{{ ref('{targ.name}') }}}}" 
        elif isinstance(self.target, UnpatchedSourceDefinition):
            target_str = f"{{{{ source('{targ.source.name}', '{targ.table.name}') }}}}" 
        unfiltered = f"{target_str} {alias}"
        filtered = f"(select * from {target_str} where {{{{{cfg_where}}}}}) {alias}" 
        return f"{{% if {cfg_where} %}}{filtered}{{% else %}}{unfiltered}{{% endif %}}"

From there, it's as simple as adding model. to qualify the columns referenced in generic test definitions.

I don't love using model as an alias—it's an overloaded term, and the thing being tested could just as easily be a source, snapshot, or seed—but for better or for worse, generic test definitions have been written with {{ model }} in their bodies. I struggle to come up with a better alias name that isn't overly clunky (thing_being_tested).

What do you think? If you're on board with the proposal, we may be able to sneak it in for v0.20.0rc2, since it's a relevant tweak to code that's changing in v0.20.

[jtcohen6]

Yes, I think you're right, we should pick an alias that's unlikely to clash. It would be a shame to fix this issue (name + column clash) and create another!

There's another option, which now feels so obvious that I'm embarrassed I didn't think of it earlier. We could rewrite generic tests to use an "import" CTE, similar to our style guide:

with dbt_test__target as (

    select * from {{ model }} 

)

select
    {{ column_name }},
    count(*) as n_records

from dbt_test__target
where {{ column_name }} is not null
group by {{ column_name }}
having count(*) > 1

I don't think we'd need to qualify the column name at all, assuming there's no change the column is also named dbt_test__target.

[jtcohen6]

Resolved by #10

[abuckenheimer]

I think your onto something with the model alias but I actually think instead of picking a natural name you should pick an unnatural one, model is just as likely to run into the disambiguation problem I ran into (presumably multiple people have columns with model as the name), but since its injected by dbt the reasoning behind it may be a bit more obscure. If you use something like __dbt_model as the name your less likely to have a collision and consequently have everything work naturally without having to rely on the user specifying <alias>.<column>. Also think __dbt_model may better hint to the user that this is an implementation detail of dbt and not something they'd normally have to worry about.

[abuckenheimer]

ah actually I may be conflating your approaches 2 and 3. I guess what your saying for 2 is that users should always qualify model columns with the model.<column> syntax which may let dbt better understand the test expression in the process. While 3 is that dbt tests macros should always alias to something (which is more relevant for the __dbt_model comment above)

[abuckenheimer]

love it

[jtcohen6]

@abuckenheimer Any interest in a contribution? :)