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

[ADAP-1044] [Bug] Incremental model with merge strategy fails to remove __dbt_tmp table when using on_schema_change=sync_all_columns #1032

Closed
2 tasks done
MikkoPaajanen opened this issue Nov 23, 2023 · 4 comments
Labels
feature:incremental type:bug Something isn't working

Comments

@MikkoPaajanen
Copy link

Is this a new bug in dbt-bigquery?

  • I believe this is a new bug in dbt-bigquery
  • I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

Current behaviour:
adapter does not remove __dbt_tmp table when using +on_schema_change: "sync_all_columns" and incremental strategy "merge".

I know these are created as "temp" tables but these are not actually bigquery temp tables as stated in: #163 (comment)

Expected Behavior

After running merge script it should check if following are true (tmp_relation_exists and strategy = "merge") and then drop tmp relation.

I believe this:

{%- if strategy == 'merge' and tmp_relation -%}
      {{ adapter.drop_relation(tmp_relation) }}
{%- endif -%}

Would resolve the issue if added in :

I don't see any reason why temp table shouldn't be dropped after the merge. Is there a reason?

Steps To Reproduce

  1. dbt-core== 1.7.1, dbt-bigquery==1.7.1
  2. Configurations
    In the model:
    {{ config(materialized='incremental') }}
    In dbt_project.yml:
    +on_schema_change: "sync_all_columns"
  3. Run dbt run -s
  4. See __dbt_tmp table in bigquery after run

Relevant log output

No response

Environment

- OS: macOS Sonoma 14.0
- Python: 3.10
- dbt-core: 1.7.1
- dbt-bigquery: 1.7.1

Additional Context

I did find some discussion on the topic in other closed issues:
#163
#154

@MikkoPaajanen MikkoPaajanen added type:bug Something isn't working triage:product labels Nov 23, 2023
@github-actions github-actions bot changed the title [Bug] Incremental model with merge strategy fails to remove __dbt_tmp table when using on_schema_change=sync_all_columns [ADAP-1044] [Bug] Incremental model with merge strategy fails to remove __dbt_tmp table when using on_schema_change=sync_all_columns Nov 23, 2023
@dbeatty10
Copy link
Contributor

I don't see any reason why temp table shouldn't be dropped after the merge. Is there a reason?

Thanks for reporting this @MikkoPaajanen !

I don't see any reason why that table shouldn't be dropped after the merge. In fact, it seems to me that any intermediate tables should be dropped as a cleanup step by the end of the materialization.

Acceptance criteria

  • No matter if a SQL model or a Python model, any incremental materialization that creates an intermediate table (whether it is temporary or permanent) table should explicitly clean up that table by the end of the materialization.

Bonus criteria

Each of these is optional and ordered from the most impactful to least.

  1. Create a functional test that verifies the intermediate table does not exist
    • e.g., use something like make_temp_relation to determine what the intermediate object will be named, and make sure it doesn't exist after building the model
  2. Refactor to use make_intermediate_relation instead of make_temp_relation since they are functionally equivalent, and calling it an "intermediate" relation avoids confusion with non-permanent temporary tables in BigQuery
  3. Consider renaming tmp_relation to something like intermediate_relation for the same reasons as above

@dbeatty10
Copy link
Contributor

Implementation idea

To do this most robustly, I'd think we'd want to refactor these lines:

{%- if language == 'python' and tmp_relation -%}
{{ adapter.drop_relation(tmp_relation) }}
{%- endif -%}

i.e, remove the lines above and add the code below somewhere in here:

  {%- if tmp_relation_exists -%} 
    {{ adapter.drop_relation(tmp_relation) }} 
  {%- endif -%} 

@nathaniel-may
Copy link
Contributor

Closing in favor of #1036.

Please re-open this issue if you can observe the temp tables persisting for more than 12 hours.

@dbeatty10
Copy link
Contributor

Re-labeling as closed in favor of #1036 (gray "closed") rather than "closed as completed / resolved" (purple "closed").

@dbeatty10 dbeatty10 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature:incremental type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants