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: conditional check to see if cols contains columns, if not replace with * #561

Merged
merged 1 commit into from
Jun 13, 2022

Conversation

swanjson
Copy link
Contributor

@swanjson swanjson commented Apr 27, 2022

Fixes #605

This is a:

  • bug fix with no breaking changes — please ensure the base branch is next/patch
  • new functionality — please ensure the base branch is next/minor
  • a breaking change — please ensure the base branch is next/major

Description & motivation

I have been using sqlfluff to lint my dbt sql code. Sqlfluff relies on the dbt compiled code to lint. When dbt_util.star is called with a reference that does not yet exist (parsing mode) without running dbt run it does not know how to handle get_filtered_columns_in_relation being empty. This PR should add the case where if get_filtered_columns_in_relation is empty it will replace the entire dbt_utils query with a star. This allows the compiled version to be properly linted without a parsing error.
Previously:

SELECT
FROM "Table.Name"

Now:

SELECT *
FROM "Table.Name"

I'm not sure if the submitted "fix" for this in Release 0.8.3 works as intended, but should be reviewed by someone more familiar with the codebase.

The method I used for testing was running dbt compile --select path/to/file on a file like the following:

WITH table1asCTE AS(
    SELECT {{ dbt_utils.star(from=ref('table1'),
        except=["Column1"
        ,"Column2"
    ]) }}
    FROM {{ref ('table1')}}
),

SELECT *
FROM table1asCTE

A prerequisite for the above is the referenced table cannot exist in the database.

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
    • Redshift
    • Snowflake
  • I followed guidelines to ensure that my changes will work on "non-core" adapters by:
    • dispatching any new macro(s) so non-core adapters can also use them (e.g. the star() source)
    • using the limit_zero() macro in place of the literal string: limit 0
    • using dbt_utils.type_* macros instead of explicit datatypes (e.g. dbt_utils.type_timestamp() instead of TIMESTAMP
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md

@dataders dataders requested a review from dbeatty10 April 27, 2022 02:29
@dbeatty10 dbeatty10 changed the base branch from next/patch to main June 13, 2022 21:37
Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an elegant and targeted solution -- thank you @swanjson 🥇

I was hoping to add some integration tests for this case, but in the meantime, I've manually verified the problem and this solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linting dbt_utils.star with Sqlfluff has an empty select
2 participants