[Bug] config.get(config.field, default_value) not working properly?

[hui-zheng]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

for the code below in adapters,

{%- set enable_refresh = config.get('enable_refresh', false) -%}

When the field enable_refresh is not set by model config, a default value false is not used by config.get().
Instead, enable_refresh is set to none.

Expected Behavior

for the code below in adapters,

{%- set enable_refresh = config.get('enable_refresh', false) -%}

When the field enable_refresh is not set by model config, a default value false shall be used. enable_refresh shall be set to `false.

Interestingly, if I use the code below, it works as expected.

{%- set enable_refresh = config.get('enable_refresh', default=false) -%}

Steps To Reproduce

Background

I came across this unexpected behavior when I tried to upgrade BQ materialized_view in dbt-labs-experimental-features to work with dbt 0.21.0

If I have a model materialized_view_A.

Not setting the enable_refresh field in the model config.

{{
  config(
    materialized = "materialized_view",
    partition_by = {
      "field" : "TIMESTAMP_TRUNC(stamp_hourly, DAY)",
      "data_type" : "date"
    },
  )
}}

in bigquery__create_materialized_view_as macro,

    {%- set enable_refresh = config.get('enable_refresh', false) -%}

if you log the value of enable_refresh, you will see it's set to none, which is unexpected.

Now, if you change the code to

    {%- set enable_refresh = config.get('enable_refresh', default=false) -%}

if you log the value of enable_refresh, you will see it's set to false, which is expected.

Relevant log output

No response

Environment

- OS: MacOS
- Python: 3.7
- dbt: 0.21.0

What database are you using dbt with?

bigquery

Additional Context

No response

[jtcohen6]

@hui-zheng You're right! This is a duplicate of a much older issue, one we closed a while ago: #2441.

@gshank It looks like you resolved that issue after v0.20, and I agree we got 90% of the way there, by making config behave much more like a dict, and config.get() a lot more like dict.get(). The subtle difference, raised again in this issue, is that you can provide the default value as the second positional argument for dict.get(). By contrast, default is still not the second positional argument for our config.get()—it's the third, with validator coming in second:

dbt-core/core/dbt/context/providers.py

Lines 328 to 329 in 0722922

	def get(self, name, validator=None, default=None):
	return ''

dbt-core/core/dbt/context/providers.py

Lines 372 to 373 in 0722922

	def get(self, name, validator=None, default=None):
	to_return = self._lookup(name, default)

I think the resolution to this issue would be as simple as switching the argument order:

    def get(self, name, default=None, validator=None):
        return ''

    def get(self, name, default=None, validator=None):
        to_return = self._lookup(name, default)

That way, config.get('arg1', 'arg2') will be the same as config.get('arg1', default = 'arg2').

This would be a big breaking change. v1 is absolutely the right time to do it; v1 after we've already put out the first release candidate, not as much. Reasons I'm still open to this:

• We're planning a longer release candidacy, and we're planning to put out a few more RCs before the final
• Setting default is much more common than setting validator; 100x more common, if I had to guess. (The only place I can think where we use the validator arg is here in a dbt-spark macro, and even there we use it as a keyword argument, since it's a pretty uncommon thing.)

So, what do we think of making this change?

---

@hui-zheng As an unrelated point: Where did you see enable_refresh? I don't see that code anywhere in our adapter plugins; I think it's in the "experimental feature" for materialized views.

[gshank]

I think the 'get' in the providers is not where config.get() ought to be... that should be the get def in BaseConfig. Let me play with this a bit.

[gshank]

The python dictionary get method only returns the default if the key doesn't exist. So if 'enable_refresh' is set to None in the config, that's what it will return, without regard to the default. I'm thinking that this should work how it does in Python, which it does right now.

[jtcohen6]

@gshank If that's the case, I'd expect:

-- models/any_model.sql
select {{ config.get('made_up_nonexistent_key', 'default_value') }} as col_value

To compile to:

select 'default_value' as col_value

Instead of (current):

select None as col_value

[gshank]

It works in Python. I think Jinja does something to the arguments.

[gshank]

You're right, it's the 'get' method in the RuntimeConfigObject. I didn't realize that we were wrapping that there. What is the 'validator' used for?

[gshank]

My proposal is that we remove the 'validator' keyword argument from 'get' and 'require'. All it does is pass along and execute the passed in function with the value, which can be done in the template if that's wanted. No tests failed. Do you know of anybody who uses it?

[jtcohen6]

Offhand, I know of exactly two places (really one place) where validator is used, in the dbt-spark plugin.

My vote might be to reverse the order of the keyword arguments, but I'd be okay with removing validator entirely if that's cleaner. We'd just need to update dbt-spark, which is fine.

[gshank]

We can leave it in place.

[gshank]

This was fixed with PR #4297. It didn't automatically close because I incorrectly put the ticket number as 4272