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-1603] [Feature] Evaluate Jinja expressions in var to their returned type rather than string. #6382

Closed
3 tasks done
elongl opened this issue Dec 5, 2022 · 4 comments
Closed
3 tasks done
Labels
duplicate This issue or pull request already exists enhancement New feature or request vars

Comments

@elongl
Copy link

elongl commented Dec 5, 2022

Is this your first time submitting a feature request?

  • I have read the expectations for open source contributors
  • I have searched the existing issues, and I could not find an existing issue for this feature
  • I am requesting a straightforward extension of existing dbt functionality, rather than a Big Idea better suited to a discussion

Describe the feature

If a Jinja expression is used within a var, reading it should return the evaluated type and value.
For example,

vars:
  is_prod: "{{ target.name == 'prod' }}"

Doing var("is_prod") in the Jinja context will evaluate to "True", not True.
However, I noticed that for the enabled configuration, this is not the case, meaning that I can do:

models:
  my_package:
    enabled: "{{ target.name == 'prod' }}"

As part of our package, we introduced feature flags that allow the users to disable certain parts of the functionality.
Users would often enable them only for prod like so:

vars:
  disable_something: "{{ target.name != 'prod' }}"

However, we later found out that this evaluates to "True" and "False" and not the respective boolean types and therefore doing {% if var("disable_something") %} would always be true because it's a non-empty string.

Describe alternatives you've considered

At the moment, we've added code that reads the var and if it's a string of "True" or "False", convert it to booleans.

Who will this benefit?

No response

Are you interested in contributing this feature?

Yes.

Anything else?

No response

@elongl elongl added enhancement New feature or request triage labels Dec 5, 2022
@github-actions github-actions bot changed the title [Feature] Evaluate Jinja expressions in var to their returned type rather than string. [CT-1603] [Feature] Evaluate Jinja expressions in var to their returned type rather than string. Dec 5, 2022
@dbeatty10 dbeatty10 self-assigned this Dec 5, 2022
@dbeatty10
Copy link
Contributor

Hey @elongl! Thanks for writing up what you are seeing, providing your work-around, and comparing it to the way enabled behaves.

tl;dr

Agreed that it is desirable for the vars project config can preserve the original data type even when it is a non-string (like boolean, integer, float, etc)!

I think the observed behavior is due to the implementation of vars within dbt_project.yml rather than the var function.

Ultimately, I don't know if implementing the desired behavior is trivial, non-trivial, or "impossible" -- I'm presuming it depends on how the Jinja is rendered for dbt_project.yml which I don't yet know the details about.

Reproduction case

I was able to reproduce the same thing as you even when using as_bool like this:

vars:
  is_prod: "{{ (target.name == 'prod') | as_bool }}"
Reproduction case

Created a file named analysis/vars_test with the content:

{% if var("is_prod") is boolean %}
✅ {{ var("is_prod") }} is a boolean, just like we want 💪
{% else %}
❌ {{ var("is_prod") }} is not a boolean 🙁
{% endif %}

Then:

dbt compile -s vars_test

And examining the output within target/compiled/{your project name here}/analysis/vars_test.sql

❌ False is not a boolean 🙁

Other info

var("is_prod") will be a boolean with this configuration:

vars:
  is_prod: false

But will be a string with this config:

vars:
  is_prod: "false"

@dbeatty10 dbeatty10 removed the triage label Dec 6, 2022
@elongl
Copy link
Author

elongl commented Dec 7, 2022

@dbeatty10
Thanks for addressing the issue!
I'd like to contribute this feature if possible.
Could you perhaps share some guidelines on the changes required?
Can also look it up if necessary.

@dbeatty10
Copy link
Contributor

Thanks for being willing to contribute this feature if possible @elongl !

Some further research would be required to know the exact areas that are relevant.

In the meantime, here's a couple git greps that might yield some promising leads:

$ git grep '"vars"' core                       
core/dbt/config/project.py:                "vars": self.vars.to_dict(),
core/dbt/config/renderer.py:            self[(k, "vars")] = _dict_if_none
core/dbt/config/renderer.py:        if first == "vars":
core/dbt/config/runtime.py:        cli_vars: Dict[str, Any] = parse_cli_vars(getattr(args, "vars", "{}"))
core/dbt/config/runtime.py:        cli_vars: Dict[str, Any] = parse_cli_vars(getattr(args, "vars", "{}"))
core/dbt/config/runtime.py:        cli_vars: Dict[str, Any] = parse_cli_vars(getattr(args, "vars", "{}"))
core/dbt/config/runtime.py:                "vars": self.vars.to_dict(),
core/dbt/config/runtime.py:        cli_vars: Dict[str, Any] = parse_cli_vars(getattr(args, "vars", "{}"))
core/dbt/parser/manifest.py:                    getattr(config.args, "vars", "{}") or "{}",
core/dbt/task/debug.py:        self.cli_vars = parse_cli_vars(getattr(self.args, "vars", "{}"))
core/dbt/utils.py:        if key == "vars" and var_args[key] == "{}":
$ git grep "to_project_config" core            
core/dbt/config/profile.py:        """Unlike to_project_config, this dict is not a mirror of any existing
core/dbt/config/project.py:        cfg = self.to_project_config(with_packages=True)
core/dbt/config/project.py:        return self.to_project_config(with_packages=True) == other.to_project_config(
core/dbt/config/project.py:    def to_project_config(self, with_packages=False):
core/dbt/config/project.py:            ProjectContract.validate(self.to_project_config())
core/dbt/config/runtime.py:        result = self.to_project_config(with_packages=True)
core/dbt/config/runtime.py:    def to_project_config(self, with_packages=False):
core/dbt/config/runtime.py:        Overrides dbt.config.Project.to_project_config to omit undefined profile
core/dbt/config/utils.py:    return project.to_project_config() if return_dict else project

@jtcohen6 jtcohen6 self-assigned this Dec 8, 2022
@jtcohen6
Copy link
Contributor

jtcohen6 commented Dec 8, 2022

I didn't catch this on the first read-through, but what's actually going on here: the entries in the vars block are not themselves Jinja-rendered!

When dbt_project.yml is loaded, rather than disable_something being stored as True or False, it's actually stored as the string "{{ target.name != 'prod' }}". (The wacky part is, if you call that variable in a model's Jinja-SQL downstream, it will sometimes be re-rendered.)

Basically, this is the same as the request made in #3105. I think it could be resolved by adding Jinja rendering to the vars block in dbt_project.yml. The question is: do we want to? That would be following one potential path for the purpose of vars, but there are a few divergent ones. There's way more discussion about this in #6170, and I'd invite you to jump in there!

In the meantime, I am going to close this issue as a duplicate.

@jtcohen6 jtcohen6 added the duplicate This issue or pull request already exists label Dec 8, 2022
@jtcohen6 jtcohen6 closed this as completed Dec 8, 2022
@jtcohen6 jtcohen6 closed this as not planned Won't fix, can't repro, duplicate, stale Dec 8, 2022
@jtcohen6 jtcohen6 added the vars label Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request vars
Projects
None yet
Development

No branches or pull requests

3 participants