-
Notifications
You must be signed in to change notification settings - Fork 506
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
Added case for handling postgres foreign tables... #476
Conversation
…ernal to current database and are imported into current database from remote data stores by using Foreign Data Wrappers functionallity).
@@ -11,6 +11,7 @@ | |||
case table_type | |||
when 'BASE TABLE' then 'table' | |||
when 'EXTERNAL TABLE' then 'external' | |||
when 'FOREIGN' then 'external' |
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.
Of course I could add separate foreign type, which would require adding new relation type here:
class RelationType(StrEnum):
Table = 'table'
View = 'view'
CTE = 'cte'
MaterializedView = 'materializedview'
External = 'external'
and/or add this case specifically to postgres__get_tables_by_pattern_sql
instead of default__get_tables_by_pattern_sql
, but I think that would be an overkill.
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.
I agree that making a separate foreign type would be overkill, but I don't hate the idea of getting a focused postgres__get_tables_by_pattern_sql
version of the macro, or at least a subset.
What about a new macro that generates the case statement for different table types? Something like
{% macro map_table_types(table_type) %}
{{ return adapter.dispatch(yada yada) }}
{% endmacro %}
{% macro default__map_table_types() %}
case table_type
when 'BASE TABLE' then 'table'
...
{% endmacro %}
{% macro postgres__map_table_types() %}
case table_type
when 'BASE TABLE' then 'table'
...
when 'FOREIGN' then 'external'
{% endmacro %}
and then get_tables_by_pattern_sql swaps out the case statement for a call to map_table_types()
(I don't like the macro name; feel free to pick something better).
Handling an adapter-specific piece of functionality that other adapters either shouldn't know about or might have an adverse reaction to in the future (living in fear of Snowflake one day adding a FOREIGN type which is some other wild thing 😱) feels like exactly the sort of thing that multiple dispatch lends itself to with minimal overhead.
How does that sound to you?
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.
Sounds reasonable to me, will update this PR today.
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.
Love it! I've added some thoughts based on your comments about how far to take it - I'm not wed to this, but I think it's a good way to balance the competing needs of minimal copy-paste vs not exposing adapter-specific logic to everything else.
@@ -11,6 +11,7 @@ | |||
case table_type | |||
when 'BASE TABLE' then 'table' | |||
when 'EXTERNAL TABLE' then 'external' | |||
when 'FOREIGN' then 'external' |
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.
I agree that making a separate foreign type would be overkill, but I don't hate the idea of getting a focused postgres__get_tables_by_pattern_sql
version of the macro, or at least a subset.
What about a new macro that generates the case statement for different table types? Something like
{% macro map_table_types(table_type) %}
{{ return adapter.dispatch(yada yada) }}
{% endmacro %}
{% macro default__map_table_types() %}
case table_type
when 'BASE TABLE' then 'table'
...
{% endmacro %}
{% macro postgres__map_table_types() %}
case table_type
when 'BASE TABLE' then 'table'
...
when 'FOREIGN' then 'external'
{% endmacro %}
and then get_tables_by_pattern_sql swaps out the case statement for a call to map_table_types()
(I don't like the macro name; feel free to pick something better).
Handling an adapter-specific piece of functionality that other adapters either shouldn't know about or might have an adverse reaction to in the future (living in fear of Snowflake one day adding a FOREIGN type which is some other wild thing 😱) feels like exactly the sort of thing that multiple dispatch lends itself to with minimal overhead.
How does that sound to you?
Wonderful @Aesthet! Please add yourself to the 0.8.1 CHANGELOG, then we'll get this merged in 🎉 |
* Fix/timestamp withought timezone (#458) * timestamp and changelog updates * changelog fix * Add context for why change to no timezone Co-authored-by: Joel Labes <joel.labes@dbtlabs.com> * also ignore dbt_packages (#463) * also ignore dbt_packages * Update CHANGELOG.md * Update CHANGELOG.md Co-authored-by: Joel Labes <joel.labes@dbtlabs.com> * date_spine: transform comment to jinja (#462) * Have union_relations raise exception when include parameter results in no columns (#473) * Raise exception if no columns in column_superset * Add relation names to compiler error message * Add `union_relations` fix to changelog * Added case for handling postgres foreign tables... (#476) * Add link for fewer_rows_than schema test in docs (#465) * Added case for handling postgres foreign tables (tables which are external to current database and are imported into current database from remote data stores by using Foreign Data Wrappers functionallity). * Reworked getting of postges table_type. * Added needed changes to CHANGELOG. Co-authored-by: José Coto <jlcoto@users.noreply.github.com> Co-authored-by: Taras Stetsiak <tstetsiak@health-union.com> * Enhance usability of star macro by only generating column aliases when prefix and/or suffix is specified (#468) * The star macro should only produce column aliases when there is either a prefix or suffix specified. * Enhanced the readme for the star macro. * Add new integration test Co-authored-by: Nick Perrott <nperrott@roiti.com> Co-authored-by: Josh Elston-Green Co-authored-by: Joel Labes <joel.labes@dbtlabs.com> * fix: extra brace typo in insert_by_period_materialization (#480) * Support quoted column names in sequential_values test (#479) * Add any value (#501) * Add link for fewer_rows_than schema test in docs (#465) * Update get_query_results_as_dict example to demonstrate accessing columnar results as dictionary values (#474) * Update get_qu ery_results_as_dict example to demonstrate accessing columnar results as dictionary values * Use slugify in example * Fix slugify example with dbt_utils. package prefix Co-authored-by: Elize Papineau <elize.papineau@dbtlabs.com> * Add note about not_null_where deprecation to Readme (#477) * Add note about not_null_where deprecation to Readme * Add docs to unique_where test * Update pull_request_template.md to reference `main` vs `master` (#496) * Correct coalesce -> concatenation typo (#495) * add any_value cross-db macro * Missing colon in test * Update CHANGELOG.md Co-authored-by: José Coto <jlcoto@users.noreply.github.com> Co-authored-by: Elize Papineau <elizepapineau@gmail.com> Co-authored-by: Elize Papineau <elize.papineau@dbtlabs.com> Co-authored-by: Joe Ste.Marie <stemarie.joe@gmail.com> Co-authored-by: Niall Woodward <niall@niallrees.com> * Fix changelog * Second take at fixing pivot to allow single quotes (#503) * fix pivot : in pivoted column value, single quote must be escaped (on postgresql) else ex. syntax error near : when color = 'blue's' * patched expected * single quote escape : added dispatched version of the macro to support bigquery & snowflake * second backslash to escape in Jinja, change case of test file columns Let's see if other databases allow this * explicitly list columns to compare * different tests for snowflake and others * specific comparison seed * Don't quote identifiers for apostrophe, to avoid BQ and SF problems * Whitespace management for macros * Update CHANGELOG.md Co-authored-by: Marc Dutoo <marc.dutoo@gmail.com> * Add bool or cross db (#504) * Create bool_or cross-db func * Forgot a comma * Update CHANGELOG.md * Code review tweaks Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com> Co-authored-by: Anders <swanson.anders@gmail.com> Co-authored-by: Mikaël Simarik <mikael.simarik@gmail.com> Co-authored-by: Graham Wetzler <graham@wetzler.dev> Co-authored-by: Taras <32882370+Aesthet@users.noreply.github.com> Co-authored-by: José Coto <jlcoto@users.noreply.github.com> Co-authored-by: Taras Stetsiak <tstetsiak@health-union.com> Co-authored-by: nickperrott <46330920+nickperrott@users.noreply.github.com> Co-authored-by: Nick Perrott <nperrott@roiti.com> Co-authored-by: Ted Conbeer <tconbeer@users.noreply.github.com> Co-authored-by: Armand Duijn <armandduijn@users.noreply.github.com> Co-authored-by: Elize Papineau <elizepapineau@gmail.com> Co-authored-by: Elize Papineau <elize.papineau@dbtlabs.com> Co-authored-by: Joe Ste.Marie <stemarie.joe@gmail.com> Co-authored-by: Niall Woodward <niall@niallrees.com> Co-authored-by: Marc Dutoo <marc.dutoo@gmail.com>
* Fix/timestamp withought timezone (#458) * timestamp and changelog updates * changelog fix * Add context for why change to no timezone Co-authored-by: Joel Labes <joel.labes@dbtlabs.com> * also ignore dbt_packages (#463) * also ignore dbt_packages * Update CHANGELOG.md * Update CHANGELOG.md Co-authored-by: Joel Labes <joel.labes@dbtlabs.com> * date_spine: transform comment to jinja (#462) * Have union_relations raise exception when include parameter results in no columns (#473) * Raise exception if no columns in column_superset * Add relation names to compiler error message * Add `union_relations` fix to changelog * Added case for handling postgres foreign tables... (#476) * Add link for fewer_rows_than schema test in docs (#465) * Added case for handling postgres foreign tables (tables which are external to current database and are imported into current database from remote data stores by using Foreign Data Wrappers functionallity). * Reworked getting of postges table_type. * Added needed changes to CHANGELOG. Co-authored-by: José Coto <jlcoto@users.noreply.github.com> Co-authored-by: Taras Stetsiak <tstetsiak@health-union.com> * Enhance usability of star macro by only generating column aliases when prefix and/or suffix is specified (#468) * The star macro should only produce column aliases when there is either a prefix or suffix specified. * Enhanced the readme for the star macro. * Add new integration test Co-authored-by: Nick Perrott <nperrott@roiti.com> Co-authored-by: Josh Elston-Green Co-authored-by: Joel Labes <joel.labes@dbtlabs.com> * fix: extra brace typo in insert_by_period_materialization (#480) * Support quoted column names in sequential_values test (#479) * Add any value (#501) * Add link for fewer_rows_than schema test in docs (#465) * Update get_query_results_as_dict example to demonstrate accessing columnar results as dictionary values (#474) * Update get_qu ery_results_as_dict example to demonstrate accessing columnar results as dictionary values * Use slugify in example * Fix slugify example with dbt_utils. package prefix Co-authored-by: Elize Papineau <elize.papineau@dbtlabs.com> * Add note about not_null_where deprecation to Readme (#477) * Add note about not_null_where deprecation to Readme * Add docs to unique_where test * Update pull_request_template.md to reference `main` vs `master` (#496) * Correct coalesce -> concatenation typo (#495) * add any_value cross-db macro * Missing colon in test * Update CHANGELOG.md Co-authored-by: José Coto <jlcoto@users.noreply.github.com> Co-authored-by: Elize Papineau <elizepapineau@gmail.com> Co-authored-by: Elize Papineau <elize.papineau@dbtlabs.com> Co-authored-by: Joe Ste.Marie <stemarie.joe@gmail.com> Co-authored-by: Niall Woodward <niall@niallrees.com> * Fix changelog * Second take at fixing pivot to allow single quotes (#503) * fix pivot : in pivoted column value, single quote must be escaped (on postgresql) else ex. syntax error near : when color = 'blue's' * patched expected * single quote escape : added dispatched version of the macro to support bigquery & snowflake * second backslash to escape in Jinja, change case of test file columns Let's see if other databases allow this * explicitly list columns to compare * different tests for snowflake and others * specific comparison seed * Don't quote identifiers for apostrophe, to avoid BQ and SF problems * Whitespace management for macros * Update CHANGELOG.md Co-authored-by: Marc Dutoo <marc.dutoo@gmail.com> * Add bool or cross db (#504) * Create bool_or cross-db func * Forgot a comma * Update CHANGELOG.md * Code review tweaks * Fix union_relations error when no include/exclude provided (#509) * Update CHANGELOG.md * Add _is_ephemeral test to get_column_values (#518) * Add _is_ephemeral test Co-authored-by: Elize Papineau <elize.papineau@dbtlabs.com> * Add deduplication macro (#512) * Update README.md * Mutually excl range examples in disclosure triangle * Fix union_relations error when no include/exclude provided * Fix union_relations error when no include/exclude provided (#509) * Update CHANGELOG.md * Add dedupe macro * Add test for dedupe macro * Add documentation to README * Add entry to CHANGELOG * Implement review * Typed materialized views as views (#525) * Typed materialized views as views * Update get_relations_by_pattern.sql * Moving fix from get_tables_by_pattern_sql reverting changes to this file to add a fix to the macro get_tables_by_pattern_sql * removing quoting from table_type removing quoting from table_type as this was causing an error when calling this macro within get_tables_by_pattern_sql * calling get_table_types_sql for materialized views calling get_table_types_sql macro to handle materialized views in sources. * Add `alias` argument to `deduplicate` macro (#526) * Add `alias` argument to `deduplicate * Test `alias` argument * Rename `alias` to `relation_alias` * Fix/use generic test naming style instead of schema test (#521) * Updated Rreferences to 'schema test' in README along with small improvements to test descriptions. Updates were also carried out in folder structure and integration README * Updated references to 'schema test' in Changelog * updated changelog with changes to documentation and fproject file structure * Apply suggestions from code review Update macro descriptions to be "asserts that" * Update CHANGELOG.md * Update README.md Co-authored-by: Joel Labes <joel.labes@dbtlabs.com> * Remove extraneous whitespace (#529) * rm whitespace from date_trunc * datediff * rm uncessary whitespace control * change log * fix CHANGELOG * address comments * Feature/add listagg macro (#530) * Update README.md * Mutually excl range examples in disclosure triangle * Fix union_relations error when no include/exclude provided * Fix union_relations error when no include/exclude provided (#509) * Update CHANGELOG.md * Add to_condition to relationships where * very minor nit - update "an new" to "a new" (#519) * add quoting to split_part (#528) * add quoting to split_part * update docs for split_part * typo * corrected readme syntax * revert and update to just documentation * add new line * Update README.md * Update README.md * Update README.md Co-authored-by: Joel Labes <joel.labes@dbtlabs.com> * add macro to get columns (#516) * add macro to get columns * star macro should use get_columns * add adapter. * swap adapter for dbt_utils Co-authored-by: Joel Labes <joel.labes@dbtlabs.com> * update documentation * add output_lower arg * update name to get_filtered_columns_in_relation from get_columns * add tests * forgot args * too much whitespace removal ----------- Actual: ----------- --->"field_3"as "test_field_3"<--- ----------- Expected: ----------- --->"field_3" as "test_field_3"<--- * didnt mean to move a file that i did not create. moving things back. * remove lowercase logic * limit_zero Co-authored-by: Joel Labes <joel.labes@dbtlabs.com> * Add listagg macro and integration test * remove type in listagg macro * updated integration test * Add redshift to listagg macro * remove redshift listagg * explicitly named group by column * updated default values * Updated example to use correct double vs. single quotes * whitespace control * Added redshift specific macro * Remove documentation * Update integration test so less likely to accidentally work Co-authored-by: Joel Labes <joel.labes@dbtlabs.com> * default everything but measure to none * added limit functionality for other dbs * syntax bug for postgres * update redshift macro * fixed block def control * Fixed bug in redshift * Bug fix redshift * remove unused group_by arg * Added additional test without order by col * updated to regex replace * typo * added more integration_tests * attempt to make redshift less complicated * typo * update redshift * replace to substr * More explicit versions with added complexity * handle special characters Co-authored-by: Joel Labes <joel.labes@dbtlabs.com> Co-authored-by: Jamie Rosenberg <james.rosenberg@canva.com> Co-authored-by: Pat Kearns <pat.kearns@fishtownanalytics.com> * patch default behaviour in get_column_values (#533) * Update changelog, add missing quotes around get_table_types_sql * rm whitespace Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com> Co-authored-by: Anders <swanson.anders@gmail.com> Co-authored-by: Mikaël Simarik <mikael.simarik@gmail.com> Co-authored-by: Graham Wetzler <graham@wetzler.dev> Co-authored-by: Taras <32882370+Aesthet@users.noreply.github.com> Co-authored-by: José Coto <jlcoto@users.noreply.github.com> Co-authored-by: Taras Stetsiak <tstetsiak@health-union.com> Co-authored-by: nickperrott <46330920+nickperrott@users.noreply.github.com> Co-authored-by: Nick Perrott <nperrott@roiti.com> Co-authored-by: Ted Conbeer <tconbeer@users.noreply.github.com> Co-authored-by: Armand Duijn <armandduijn@users.noreply.github.com> Co-authored-by: Elize Papineau <elizepapineau@gmail.com> Co-authored-by: Elize Papineau <elize.papineau@dbtlabs.com> Co-authored-by: Joe Ste.Marie <stemarie.joe@gmail.com> Co-authored-by: Niall Woodward <niall@niallrees.com> Co-authored-by: Marc Dutoo <marc.dutoo@gmail.com> Co-authored-by: Judah Rand <17158624+judahrand@users.noreply.github.com> Co-authored-by: Luis Leon <98919783+luisleon90@users.noreply.github.com> Co-authored-by: Brid Moynihan <moynihanbridgit@gmail.com> Co-authored-by: SunriseLong <44146580+SunriseLong@users.noreply.github.com> Co-authored-by: Grace Goheen <53586774+graciegoheen@users.noreply.github.com> Co-authored-by: Jamie Rosenberg <james.rosenberg@canva.com> Co-authored-by: Pat Kearns <pat.kearns@fishtownanalytics.com> Co-authored-by: James McNeill <55981540+jpmmcneill@users.noreply.github.com>
(tables which are external to current database and are imported into current database from remote data stores by using Foreign Data Wrappers functionality).
This is a:
master
dev/
branchdev/
branchDescription & motivation
As described in this issue we are facing errors when we are using Postgres Foreign Data Wrappers functionality - in particular tables which are imported as part of IMPORT FOREIGN SCHEMA command. Those tables are external to database in which they are imported and they have type FOREIGN, which is causing errors during execution of get_relations_by_pattern macros which is getting values from get_tables_by_pattern_sql macros.
Checklist
star()
source)limit_zero()
macro in place of the literal string:limit 0
dbt_utils.type_*
macros instead of explicit datatypes (e.g.dbt_utils.type_timestamp()
instead ofTIMESTAMP