-
Notifications
You must be signed in to change notification settings - Fork 496
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
Update star.sql to allow for non-quote wrapped column names #706
Conversation
column encased in quotes functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice persistence on this @CR-Lough 💪
This one is kinda tricky to test! Let me try to explain why, and a possible path forward.
This PR is enabling new behavior to choose between quoted identifiers (default) and non-quoted identifiers. To me, it seems the best way to test this is by actually examine the string produced by star()
and seeing if the value matches the expectations (e.g., starting and ending with the adapter.quote
value or not depending on the arguments).
Creating such a test could be a bit different than existing tests. It might look something like this (which I didn't actually try out!):
integration_tests/data/sql/single_column_table.csv
my_column
x
integration_tests/models/sql/test_star_quote_identifiers.sql
select
{{ dbt.string_literal(adapter.quote("my_column")) | lower }} as expected,
{{ dbt_utils.star(from=ref('single_column_table', quote_identifiers=True)) | trim | lower }} as actual
union all
select
{{ dbt.string_literal("my_column") | lower }} as expected,
{{ dbt_utils.star(from=ref('single_column_table', quote_identifiers=False)) | trim | lower }} as actual
integration_tests/models/sql/schema.yml
- name: test_star_quote_identifiers
tests:
- assert_equal:
actual: actual
expected: expected
I put in all the extra lower
filters because some databases will return lowercase and others uppercase, and we want the same logic to work across adapters. It could just as easily be upper
filters instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome @CR-Lough -- nice work figuring out those tests 💪
I took another pass through this PR and saw that there's no updates reflecting the new argument within the README.
Could you add your new argument here?
Lines 936 to 942 in 6a5dd1e
**Args:** | |
- `from` (required): a [Relation](https://docs.getdbt.com/reference/dbt-classes#relation) (a `ref` or `source`) that contains the list of columns you wish to select from | |
- `except` (optional, default=`[]`): The name of the columns you wish to exclude. (case-insensitive) | |
- `relation_alias` (optional, default=`''`): will prefix all generated fields with an alias (`relation_alias`.`field_name`). | |
- `prefix` (optional, default=`''`): will prefix the output `field_name` (`field_name as prefix_field_name`). | |
- `suffix` (optional, default=`''`): will suffix the output `field_name` (`field_name as field_name_suffix`). |
And an example usage here?
Lines 944 to 965 in 6a5dd1e
**Usage:** | |
```sql | |
select | |
{{ dbt_utils.star(ref('my_model')) }} | |
from {{ ref('my_model') }} | |
``` | |
```sql | |
select | |
{{ dbt_utils.star(from=ref('my_model'), except=["exclude_field_1", "exclude_field_2"]) }} | |
from {{ ref('my_model') }} | |
``` | |
```sql | |
select | |
{{ dbt_utils.star(from=ref('my_model'), except=["exclude_field_1", "exclude_field_2"], prefix="max_") }} | |
from {{ ref('my_model') }} | |
``` |
adds example usage of star macro's quote_identifiers argument
@dbeatty10 that last addition to the README.md should be the last checkbox in the list of items at the top of the page 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CR-Lough Thanks for this contribution to add the optional configuration to star()
for quoting identifiers! 🚀
@@ -933,6 +933,7 @@ the star macro. | |||
This macro also has an optional `relation_alias` argument that will prefix all generated fields with an alias (`relation_alias`.`field_name`). | |||
The macro also has optional `prefix` and `suffix` arguments. When one or both are provided, they will be concatenated onto each field's alias | |||
in the output (`prefix` ~ `field_name` ~ `suffix`). NB: This prevents the output from being used in any context other than a select statement. | |||
This macro also has an optional `quote_identifiers` argument that will encase the selected columns and their aliases in double quotes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first time I read this, I thought it meant quoting the optional relation_alias
. But re-reading it, I realize it means the alias it is assigned (if any).
* add safe_divide documentation * add safe_divide macro * add integration test for safe_divide macro * Merge changes from main into utils v1 (#699) * Correct link from README to the CONTRIBUTING guide. (#687) * fix typo (#688) Co-authored-by: Alex Malins <[email protected]> * Change `escape_single_quotes` Reference in Pivot Macro (#692) * Update pivot.sql * Changelog Updates Co-authored-by: Liam O'Boyle <[email protected]> Co-authored-by: Alex Malins <[email protected]> Co-authored-by: Alex Malins <[email protected]> Co-authored-by: zachoj10 <[email protected]> * Use backwards comaptible versions of timestamp macro * moved macro and documentation to new SQL generator section * add tests with expressions * fix syntax errors (#705) * fix syntax errors * remove whitespace in seed file * Restore dbt. prefix for all migrated cross-db macros (#701) * added prefix dbt. on cross db macros * Also prefix for new macro * Adding changelog change * Squashed commit of the following: commit 5eba82b Author: Deanna Minnick <[email protected]> Date: Wed Oct 12 10:30:42 2022 -0400 remove whitespace in seed file commit 7a2a5e3 Author: Deanna Minnick <[email protected]> Date: Wed Oct 12 10:22:07 2022 -0400 fix syntax errors Co-authored-by: Joel Labes <[email protected]> * Remove obsolete condition argument from expression_is_true (#700) * Remove obsolete condition argument from expression_is_true * Improve docs * Improve docs * Update star.sql to allow for non-quote wrapped column names (#706) * Update star.sql * Update star.sql * feat: add testing to star macro column encased in quotes functionality * chore: update schema.yml * Update star.sql * chore: update star.sql and schema.yml * chore: update star.sql to trim blank space * Update README.md * Update README.md adds example usage of star macro's quote_identifiers argument Co-authored-by: crlough <[email protected]> * Switch to dbt.escape_single_quotes * Change deprecation resolution advice * Wrap xdb warnings in if execute block * Slugify for snowflake (#707) * Merge main into utils-v1 (#726) * Feature/safe divide (#697) * add safe_divide documentation * add safe_divide macro * add integration test for safe_divide macro * moved macro and documentation to new SQL generator section Co-authored-by: Grace Goheen <[email protected]> * Revert "Feature/safe divide (#697)" (#702) This reverts commit f368cec. * Quick nitpicks (#718) I was doing some studying on these and spotted some stuff. One verb conjugation and a consistency in macro description Co-authored-by: deanna-minnick <[email protected]> Co-authored-by: Grace Goheen <[email protected]> Co-authored-by: ian-fahey-dbt <[email protected]> * Feat: add macro get_query_results_as_single_value (#696) * feat: add query_results_as_single_value.sql macro * chore: update the macro definition Current error to work through: "failed to find conversion function from unknown to text" * chore: update test * chore: final edits * chore: remove extra model reference * chore: update return() to handle BigQuery * chore: README.md, macro updates * feat: factoring in first review changes * chore: updates to testing * chore: updates tests * chore: update test for bigquery * chore: update cast for bigquery * Use example with a single record in readme * Add default value when no record found * test when no results are found * Rename test file * Add test definitions * Fix incorrect ref * And another one * Update test_get_query_results_as_single_value.sql * cast strings as strings * Put arg in right place * Update test_get_query_results_as_single_value.sql * switch to limit zero for BQ * Update test_get_query_results_as_single_value.sql * quote column name in arg * snowflake wont let you safe cast something to itself * warning to future readers [skip ci] * Add singular test to check for multi row/multi column setup * forgot to save comment [skip ci] * Rename to get_single_value Co-authored-by: crlough-gitkraken <[email protected]> Co-authored-by: Joel Labes <[email protected]> * Remove rc1 requirement for utils v1 * Recency truncate date option (#731) * WIP changing recency test * Add tests * cast to timestamp for bq * forgot the curlies * avoid lateral column aliasing * ts not dt * cast source as timestamp * don't cast inside test * cast as date instead of truncate * Update recency.sql * log bq events * store pg artifacts * int tests dir * Correctly store artifacts * try casting to date or datetime * order of operations more like order of ooperations * dt -> ts * Do I really have to cast this? * Revert "Do I really have to cast this?" This reverts commit 21e2c0d. * Output a warning when star finds no columns, not '*' (#732) * Change star() behaviour when no columns returned * Code review: return a * in compile mode * README changes * Delete xdb_deprecation_warning.sql * Update README.md * Remove from ToC * Update toc * Fix surrogate key variable example Co-authored-by: Deanna Minnick <[email protected]> Co-authored-by: Liam O'Boyle <[email protected]> Co-authored-by: Alex Malins <[email protected]> Co-authored-by: Alex Malins <[email protected]> Co-authored-by: zachoj10 <[email protected]> Co-authored-by: Grace Goheen <[email protected]> Co-authored-by: deanna-minnick <[email protected]> Co-authored-by: Simon Quvang <[email protected]> Co-authored-by: miles <[email protected]> Co-authored-by: Connor <[email protected]> Co-authored-by: crlough <[email protected]> Co-authored-by: fivetran-catfritz <[email protected]> Co-authored-by: ian-fahey-dbt <[email protected]> Co-authored-by: crlough-gitkraken <[email protected]>
resolves #644
This is a:
All pull requests from community contributors should target the
main
branch (default).Description & motivation
The star.sql macro inside of dbt-utils/sql forces users to encase column names in quotes. This change adds the optional 'quote_identifier' parameter (default True) to the macro.
Checklist
star()
source)limit_zero()
macro in place of the literal string:limit 0
dbt.type_*
macros instead of explicit datatypes (e.g.dbt.type_timestamp()
instead ofTIMESTAMP