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

[CT-3293] [Feature] Use RelationType enum values in get_catalog_relations / Catalog artifacts #8947

Open
2 of 6 tasks
dbeatty10 opened this issue Oct 31, 2023 · 1 comment
Labels
enhancement New feature or request Refinement Maintainer input needed

Comments

@dbeatty10
Copy link
Contributor

dbeatty10 commented Oct 31, 2023

Housekeeping

  • I am a maintainer of dbt-core

Short description

Include values from the RelationType enum within the following:

  • get_catalog_relations [1, 2]
  • get_catalog [1, 2]
  • Catalog artifacts

There should be a mapping from each adapter-specific table_type to a canonical relation type. Here's a swag at what it might look like:

Postgres table_type Canonical relation type
BASE TABLE table
LOCAL TEMPORARY table
VIEW view
MATERIALIZED VIEW materialized_view
EXTERNAL external
FOREIGN foreign

Acceptance criteria

  • dbt has an enumeration of canonical types of relations -- not all need to be representable within a database platform (i.e., ephemeral and/or cte)
  • Each dbt adapter translates from adapter-specific types into canonical types
  • Optional: both canonical and adapter-specific types are surfaced in macro SQL, metadata API queries, and generated artifacts (like get_catalog_relations and catalog.json)
  • Add functional tests to dbt-core that validate that each valid materialized option results in the relevant enum witin get_catalog_relations and catalog.json
  • Inherit functional tests in downstream adapters

Impact to Other Teams

  • Cloud Artifacts (CA)
  • Potentially others

Will backports be required?

TBD 🤷

Context

Initial background context from #8864:

I did some things. Please take a look at the four PRs attached to this issue.

It's worth noting that all four adapters which support materialized views (dbt-posgres, dbt-snowflake, dbt-bigquery, and dbt-redshift) were affected. All failed the new integration test that I created and all now pass that test. It's also worth noting that the new integration test tests all supported relation types for that adapter.

👇 And here's the key portion that prompted this issue being opened:

I'd like to call out one thing in particular; it looks like we're not consistent across adapters with what table_type returns. Most of them return one of "BASE TABLE", "VIEW", (and now) "MATERIALIZED VIEW"/"DYNAMIC TABLE". However, dbt-bigquery returns "table", "view", (and now) "materialized view". I did not want to change that because I didn't want to break something downstream. My knee jerk reaction is that these should return a value in the RelationType enum. I would consider all of this as beyond the scope of this fix though.

Originally posted by @mikealfare in #8864 (comment)

Other considerations

There are multiple places that contain similar logic. This is one of them:

@github-actions github-actions bot changed the title Use RelationType enum values in get_catalog_relations / Catalog artifacts [CT-3293] Use RelationType enum values in get_catalog_relations / Catalog artifacts Oct 31, 2023
@dbeatty10 dbeatty10 added enhancement New feature or request triage labels Oct 31, 2023
@dbeatty10 dbeatty10 changed the title [CT-3293] Use RelationType enum values in get_catalog_relations / Catalog artifacts [CT-3293] [Feature] Use RelationType enum values in get_catalog_relations / Catalog artifacts Oct 31, 2023
@dbeatty10
Copy link
Contributor Author

Needs refinement

I am not certain of the consequences if we don't do this -- it might be that this is ultimately not necessary or valuable!

Also not sure if we've covered all the key considerations (like all the places that have something similar).

Labeling this as "Refinement" to continue discovery work here.

@dbeatty10 dbeatty10 added Refinement Maintainer input needed and removed triage labels Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Refinement Maintainer input needed
Projects
None yet
Development

No branches or pull requests

1 participant