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-729 Include schema model config in unrendered config #5344

Merged
merged 5 commits into from
Aug 9, 2022

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Jun 7, 2022

resolves #5338

Description

Unrendered_config originally only included config from models. When we added config for models in schema files, we didn't include that in unrendered_config. Unrendered_config still doesn't include model config from dbt_project.yml, since the code explicitly avoided that.

Checklist

@gshank gshank requested review from a team as code owners June 7, 2022 16:33
@gshank gshank requested review from nathaniel-may and iknox-fa June 7, 2022 16:33
@cla-bot cla-bot bot added the cla:yes label Jun 7, 2022
@gshank
Copy link
Contributor Author

gshank commented Jun 7, 2022

@jtcohen6 I didn't find any actual documentation about what unrendered_config is supposed to contain. The code goes to some trouble to not include the config from dbt_project.yml. I'm not exactly clear on why that is. Could you cast some light on it?

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jun 7, 2022

@gshank My impression is that unrendered_config stores any raw Jinja expressions in dbt_project.yml. So for instance:

# dbt_project.yml
models:
  +enabled: "{{ target.name == 'dev' }}"

The entry for a model node in the manifest should show up like:

"config": {
        "enabled": true,
        "alias": null,
        "schema": null,
        "database": null,
        "tags": [],
        "meta": {},
        "materialized": "view",
        "persist_docs": {},
        "quoting": {},
        "column_types": {},
        "full_refresh": null,
        "unique_key": null,
        "on_schema_change": "ignore",
        "grants": {},
        "packages": [],
        "post-hook": [],
        "pre-hook": []
      },
...
"unrendered_config": {
        "enabled": "{{ target.name == 'dev' }}"
      },

dbt should use the "unrendered" form of enabled for comparison, instead of the rendered form, which inevitably differs across target environments (even though it hasn't changed).

Docs on this behavior: https://docs.getdbt.com/reference/node-selection/state-comparison-caveats#false-positives

I believe we are updating unrendered_config with values from dbt_project.yml, it's just happening during config resolution here:

# unrendered_config is used to compare the original database/schema/alias
# values and to handle 'same_config' and 'same_contents' calls
parsed_node.unrendered_config = config.build_config_dict(rendered=False)

@gshank
Copy link
Contributor Author

gshank commented Jun 7, 2022

If you look at "build_config_dict" in core/dbt/context/context_config.py it uses different renderers for rendered and unrendered. The "unrendered" flavor uses UnrenderedConfigGenerator. If you compare that to ContextConfigGenerator, the unrendered config generator explicitly skips loading the config from dbt_project.yml.

@stu-k stu-k requested a review from a team June 7, 2022 17:05
@gshank
Copy link
Contributor Author

gshank commented Jun 7, 2022

It looks like the issue is not that the dbt_project.yml config isn't processed, but that the unrendered_config dictionaries aren't merged, so that we lose dbt_project.yml config items in unrendered_config that are overwritten by later config. The actual config in the node is correct, because that part is merged properly.

@iknox-fa iknox-fa removed their request for review July 5, 2022 19:44
@iknox-fa
Copy link
Contributor

iknox-fa commented Jul 5, 2022

Removing myself as a reviewer on this one since I don't know enough about parsing to be useful here.

@gshank gshank force-pushed the ct-729-unrendered_config branch from f764b26 to a238f2e Compare August 9, 2022 13:28
@jtcohen6
Copy link
Contributor

jtcohen6 commented Aug 9, 2022

@gshank Did we decide if the behavior change here was the right one? Or better than the status quo, at least? I remember discussing that we don't have any way to access the unrendered form of patch (.yml) configs for the model, but that is the most correct set of information to include in the model's unrendered_config.

@gshank
Copy link
Contributor Author

gshank commented Aug 9, 2022

My recollection is that not including the dbt_project configs when re-rendering was actually a bug.

@gshank gshank merged commit 3aab9be into main Aug 9, 2022
@gshank gshank deleted the ct-729-unrendered_config branch August 9, 2022 16:49
VersusFacit pushed a commit that referenced this pull request Sep 5, 2022
* Pass patch_config_dict to build_config_dict when creating
unrendered_config

* Add test case for unrendered_config

* Changie

* formatting, fix test

* Fix test so unrendered config includes docs config
agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request Sep 16, 2022
* Pass patch_config_dict to build_config_dict when creating
unrendered_config

* Add test case for unrendered_config

* Changie

* formatting, fix test

* Fix test so unrendered config includes docs config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-729] [Bug] unrendered_config in manifest.json doesn't update correctly from model properties yaml
4 participants