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

[SPIKE] Investigate treating dynamic tables as RelationType.Table #1040

Closed
3 tasks done
dataders opened this issue May 14, 2024 · 3 comments
Closed
3 tasks done

[SPIKE] Investigate treating dynamic tables as RelationType.Table #1040

dataders opened this issue May 14, 2024 · 3 comments
Labels
enhancement New feature or request Stale

Comments

@dataders
Copy link
Contributor

Is this your first time submitting a feature request?

  • I have read the expectations for open source contributors
  • I have searched the existing issues, and I could not find an existing issue for this feature
  • I am requesting a straightforward extension of existing dbt-snowflake functionality, rather than a Big Idea better suited to a discussion

Describe the feature

spin off of @jtcohen6's #1038

Investigate treating dynamic tables as RelationType.Table instead of SnowflakeRelationType.DynamicTable with the understanding that we need to run an additional describe query at the start of the dynamic_table materialization to figure out if it's actually a dynamic or a static table (among other configs).

Similar to the workaround described here: #1016 (comment)

This should only be done in the event that #1038 does not pan out, performance-wise

Pros

  • Runs an additional query, but only while materializing dynamic tables. For DTs that already exist, we already need to run this query to check and see if any configs have changed.
  • Longer-term, this additional query would no longer be necessary if Snowflake added support for create or replace when switching between tables and dynamics tables. (The team is looking into this, it may be feasible but it may not be.)

Cons

  • Shorter-term, this would require one additional describe statement per DT, until we can re-plumb the materialization logic (or start caching more relation attributes) to avoid rerunning.
  • Until the Snowflake team can support create or replace table for DT → table, this will have a known edge case where switching materialized: 'dynamic_table' to materialized: table does not work.

Describe alternatives you've considered

if #1038 works, we don't need to do this

Who will this benefit?

No response

Are you interested in contributing this feature?

No response

Anything else?

No response

@dataders dataders added enhancement New feature or request triage labels May 14, 2024
@dataders dataders changed the title [SPIKE] Investigate treating dynamic tables as RelationType.Table [SPIKE] Investigate treating dynamic tables as RelationType.Table May 14, 2024
@dataders
Copy link
Contributor Author

the framing here makes me think of a hypothetical third option based on work done to abstract away relation-specific details from CRUD DDL statements. Of course with SQL, instead of CRUD, we have CRADD: CREATE, REPLACE, ALTER, DROP,DESCRIBE. dbt-snowflake's macros/relations/ directory shows this clearly.

This work was done to support Dynamic Tables, but rolling this out globally for the other materializations might serve addressing option 2.

By that I mean we might have options available immediately to remove some of the cases, without removing the SnowflakeRelationType.DynamicTable entirely.

for example, the lowest-hanging fruit here is that purportedly, a Dynamic Table can be dropped with a DROP TABLE and DYNAMIC is not required.

We can either:

  1. retain the snowflake__get_drop_dynamic_table_sql() macro, but drop DYNAMIC from it, or
  2. introduce a snowflake__ override of default__get_create_sql() that executes snowflake__get_drop_table_sql() if relation.is_dynamic

the describe hitch

While we can certainly alter dynamic table materialization directly, to call the additionally required DESCRIBE, it would be cleaner if it could be incorporated into the existing relation-agnostic "CRADD" macros -- especially given that we'll likely need to take this path for future relation types on the roadmap (EXTERNAL ICEBERG HYBRID)

perhaps the existing snowflake__describe_dynamic_table() could be incorporated somehow into either:

  • a relation-agnostic get_describe_sql(), or
  • a version of load_cached_relation()/list_relations_without_caching() that can dispatch to relation-specific shims?

Copy link
Contributor

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please comment on the issue or else it will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 11, 2024
Copy link
Contributor

Although we are closing this issue as stale, it's not gone forever. Issues can be reopened if there is renewed community interest. Just add a comment to notify the maintainers.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Stale
Projects
None yet
Development

No branches or pull requests

1 participant