-
Notifications
You must be signed in to change notification settings - Fork 207
🩹 Remove table_owner from text only cols #1404
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
base: main
Are you sure you want to change the base?
🩹 Remove table_owner from text only cols #1404
Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Morgan Kerle.
|
PR dbt-labs#1057 added several new column names to the `text_only_columns` arg to the `table_from_rows` call in the `_catalog_filter_table` class method. This method is called as part of the `dbt docs generate` task. However, some of the new column names included are not included in the list of columns returned by certain adapter implementations for the `__get_catalog` and `__get_catalog_relations`. For example, the relevant BigQuery and Athena macros do not return a column named `table_owner` (see dbt-labs#1135). This results in the agate type checker emitting a warning when attempting typing checking runs. Any columns added to the `text_only_columns` argument for the call to `table_from_rows` in the base implementation should presumably be columns that are returned by all adapters implementations.
a6de9e7 to
e715fe0
Compare
|
Thank you for your pull request! We could not find a changelog entry for this change in the dbt-adapters package. For details on how to document a change, see the Contributing Guide. |
|
Thank you for your pull request! We could not find a changelog entry for this change in the dbt-snowflake package. For details on how to document a change, see the Contributing Guide. |
| # On snowflake, users can set QUOTED_IDENTIFIERS_IGNORE_CASE, so force | ||
| # the column names to their lowercased forms. | ||
| from dbt_common.clients.agate_helper import table_from_rows | ||
| from dbt.adapters.base.impl import _catalog_filter_schemas | ||
|
|
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.
nit: move imports above comment
| # On snowflake, users can set QUOTED_IDENTIFIERS_IGNORE_CASE, so force | |
| # the column names to their lowercased forms. | |
| from dbt_common.clients.agate_helper import table_from_rows | |
| from dbt.adapters.base.impl import _catalog_filter_schemas | |
| from dbt_common.clients.agate_helper import table_from_rows | |
| from dbt.adapters.base.impl import _catalog_filter_schemas | |
| # On snowflake, users can set QUOTED_IDENTIFIERS_IGNORE_CASE, so force | |
| # the column names to their lowercased forms. |
| table = table_from_rows( | ||
| table.rows, | ||
| table.column_names, | ||
| text_only_columns=[ |
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.
one other option here would be to add a method that adapters implement to override this instead of needing to override the whole method
i.e.
def _get_text_only_cols() -> List[str]
return [...]
Then we snowflake / whomever can implement this method to override the default
Problem
PR #1057 added several new column names to the
text_only_columnsarg to thetable_from_rowscall in the_catalog_filter_tableclass method. This method is called as part of thedbt docs generatetask.However, some of the new column names included are not included in the list of columns returned by certain adapter implementations for the
__get_catalogand__get_catalog_relations.For example, the relevant BigQuery and Athena macros do not return a column named
table_owner(see #1135).This results in the agate type checker emitting a warning when attempting typing checking runs.
Solution
Any columns added to the
text_only_columnsargument for the call totable_from_rowsin the base implementation should presumably be columns that are returned by all adapters implementations.This PR removes the
table_ownername from the list of text_only_columns included in the base implementation of_catalog_filter_tableand moves this to a Snowflake-specific adapter implementation.I investigated whether agate provides a means of suppressing this warning but there does not appear to be a mechanism to do so. Additionally, I am unsure whether this would be desirable.
resolves #1135
Checklist