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

Star macro is not supporting SQLFluff when columns are not present #802

Closed
1 of 5 tasks
katieclaiborne opened this issue May 31, 2023 · 6 comments
Closed
1 of 5 tasks
Labels
bug Something isn't working

Comments

@katieclaiborne
Copy link

Describe the bug

Based on @vperron's investigation in #732, it appears that SQLFluff is not following the flags.WHICH == 'compile' path in the star macro, which is intended to keep it happy when get_filtered_columns_in_relation returns no columns.

Instead, SQLFluff's rendered query follows the star macro's else condition, which results in a parsing error.

Steps to reproduce

See @dbeatty10's comment here.

Expected results

We expected the macro to return the * in line 19.

Actual results

Instead, it returns the comment in line 26.

Screenshots and log output

See Doug's comment.

System information

The contents of your packages.yml file:

packages:
  - package: calogica/dbt_expectations
    version: [">=0.8.0", "<0.9.0"]
  - package: dbt-labs/dbt_project_evaluator
    version: 0.5.0

Which database are you using dbt with?

  • postgres
  • redshift
  • bigquery
  • snowflake
  • other (specify: ____________)

The output of dbt --version:

Core:
  - installed: 1.5.0
  - latest:    1.5.1 - Update available!

  Your version of dbt-core is out of date!
  You can find instructions for upgrading here:
  https://docs.getdbt.com/docs/installation

Plugins:
  - bigquery: 1.5.1 - Up to date!

Additional context

Doug suggested that we coordinate with SQLFluff maintainers on a path forward.

If I had to guess, I'd say that means understanding the actual value of flags.WHICH in this scenario, then updating the star macro and/or SQLFluff's behavior to produce the safe output.

Are you interested in contributing the fix?

Yes, although I'd likely need some hand-holding!

@katieclaiborne katieclaiborne added bug Something isn't working triage labels May 31, 2023
@katieclaiborne katieclaiborne changed the title Star macro is not supporting SQLFluff compilation when filtered columns are not present Star macro is not supporting SQLFluff when filtered columns are not present May 31, 2023
@katieclaiborne katieclaiborne changed the title Star macro is not supporting SQLFluff when filtered columns are not present Star macro is not supporting SQLFluff when columns are not present May 31, 2023
@katieclaiborne
Copy link
Author

Would it be possible to prioritize this issue?

It's causing problems in our slim CI setting, where the from relation may not be present in the target schema.

@moreaupascal56
Copy link

moreaupascal56 commented Jul 12, 2023

Hello, I've been pretty interested at this since I am diving a lot into flags.which for another use-case.
Here I think the issue is how the dbt templater works in sqlfluff. Indeed if you add a print in the star macro:

{% macro star(from, relation_alias=False, except=[], prefix='', suffix='', quote_identifiers=True) -%}
    {{ return(adapter.dispatch('star', 'dbt_utils')(from, relation_alias, except, prefix, suffix, quote_identifiers)) }}
{% endmacro %}

{% macro default__star(from, relation_alias=False, except=[], prefix='', suffix='', quote_identifiers=True) -%}
    {%- do dbt_utils._is_relation(from, 'star') -%}
    {%- do dbt_utils._is_ephemeral(from, 'star') -%}

    {#-- Prevent querying of db in parsing mode. This works because this macro does not create any new refs. #}
    {%- if not execute -%}
        {% do return('*') %}
    {%- endif -%}

    {% set cols = dbt_utils.get_filtered_columns_in_relation(from, except) %}

    {{ print("flags.which: " ~ flags.WHICH ) }}      ----------> HERE

    {%- if cols|length <= 0 -%}
        {% if flags.WHICH == 'compile' %}
            {% set response %}
*
/* No columns were returned. Maybe the relation doesn't exist yet 
or all columns were excluded. This star is only output during  
dbt compile, and exists to keep SQLFluff happy. */
            {% endset %}
            {% do return(response) %}
        {% else %}
            {% do return("/* no columns returned from star() macro */") %}
        {% endif %}
    {%- else -%}
        {%- for col in cols %}
            {%- if relation_alias %}{{ relation_alias }}.{% else %}{%- endif -%}
                {%- if quote_identifiers -%}
                    {{ adapter.quote(col)|trim }} {%- if prefix!='' or suffix!='' %} as {{ adapter.quote(prefix ~ col ~ suffix)|trim }} {%- endif -%}
                {%- else -%}
                    {{ col|trim }} {%- if prefix!='' or suffix!='' %} as {{ (prefix ~ col ~ suffix)|trim }} {%- endif -%}
                {% endif %}
            {%- if not loop.last %},{{ '\n  ' }}{%- endif -%}
        {%- endfor -%}
    {% endif %}
{%- endmacro %}


I have the following outputs (using the example dbt project from the other issue):

  • dbt compile
Capture d’écran 2023-07-12 à 15 22 17

So flags.WHICH is compile

  • sqlfluff render:
Capture d’écran 2023-07-12 à 15 17 55

So here the flags.WHICH is equal to run (and called 3 times for some reason) and therefore the macro go in the else clause.

So I think we have to understand why sqlfluff use dbt run instead of dbt compile in the templater.

Have a great day :)

@moreaupascal56
Copy link

moreaupascal56 commented Jul 12, 2023

FYI I have been able to force sqlfluff to have a "compile" flag by adding the following line in this file from sqlfluff:

    which: Optional[str] = "compile"

The class would be:

# From sqlfluff-templater-dbt/sqlfluff_templater_dbt/templater.py

@dataclass
class DbtConfigArgs:
    """Arguments to load dbt runtime config."""

    project_dir: Optional[str] = None
    profiles_dir: Optional[str] = None
    profile: Optional[str] = None
    target: Optional[str] = None
    threads: int = 1
    single_threaded: bool = False
    # dict in 1.5.x onwards, json string before.
    vars: Optional[Union[Dict, str]] = None if DBT_VERSION_TUPLE >= (1, 5) else ""
    which: Optional[str] = "compile"

With this I am able to get this output:
Capture d’écran 2023-07-12 à 15 55 03

I may open an issue on SQLFluff to understand if this is normal that we do not set the which attribute of flag in this file or if this was just not needed before.

[EDIT] We have the flag.WHICH = run because in this sqlfluff file, from the dbt_templater that defines the flags used in the templater, it calls the function flags.set_from_args from dbt below:

# From dbt-core/core/dbt/flags.py

def set_from_args(args: Namespace, user_config):
    global GLOBAL_FLAGS
    from dbt.cli.main import cli
    from dbt.cli.flags import Flags, convert_config

    # we set attributes of args after initialize the flags, but user_config
    # is being read in the Flags constructor, so we need to read it here and pass in
    # to make sure we use the correct user_config
    if (hasattr(args, "PROFILES_DIR") or hasattr(args, "profiles_dir")) and not user_config:
        from dbt.config.profile import read_user_config

        profiles_dir = getattr(args, "PROFILES_DIR", None) or getattr(args, "profiles_dir")
        user_config = read_user_config(profiles_dir)

    # make a dummy context to get the flags, totally arbitrary
    ctx = cli.make_context("run", ["run"])  ----------> HERE
    flags = Flags(ctx, user_config)
    for arg_name, args_param_value in vars(args).items():
        args_param_value = convert_config(arg_name, args_param_value)
        object.__setattr__(flags, arg_name.upper(), args_param_value)
        object.__setattr__(flags, arg_name.lower(), args_param_value)
    GLOBAL_FLAGS = flags  # type: ignore

The line

    # make a dummy context to get the flags, totally arbitrary
    ctx = cli.make_context("run", ["run"]) 

set a dummy context with a run CLI command which causes the flags.which = run. And because I think the dbt templater from sqlfluff does not run dbt commands directly but functions from the dbt-core templater directly it never overwrite this dummy run config.

@katieclaiborne
Copy link
Author

Thanks for investigating, @moreaupascal56! I've subscribed to sqlfluff/sqlfluff#4965 as well.

@moreaupascal56
Copy link

moreaupascal56 commented Jul 20, 2023

hey @katieclaiborne, it should be good with the new release of sqlfluff (version 2.1.3). If you can confirm on an your side it would be perfect :)

@katieclaiborne
Copy link
Author

Done! I rendered a previously failing model with the new version of SQLFluff, and confirmed that the star macro was indeed following the happy path. The model now also passes sqlfluff lint.

  select

*
/* No columns were returned. Maybe the relation doesn't exist yet
or all columns were excluded. This star is only output during
dbt compile, and exists to keep SQLFluff happy. */

Thanks for your work on this, @moreaupascal56! I will close the issue.

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

No branches or pull requests

3 participants