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

Fix incremental delete+insert SQL #9459

Closed
wants to merge 2 commits into from

Conversation

ataft
Copy link

@ataft ataft commented Jan 26, 2024

resolves dbt-labs/dbt-adapters#150

Problem

The delete query for the 'delete+insert' incremental_strategy with 2+ unique_key columns is VERY inefficient. In many cases, it will hang and never return for deleting small amounts of data (<100K rows).

Solution

Improve the query by switching to a much more efficient delete strategy:

delete from table1
where (col1, col2) in (
    select distinct col1, col2 from table1_tmp
)

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
  • This PR includes type annotations for new and modified functions

@ataft ataft requested a review from a team as a code owner January 26, 2024 00:27
Copy link

cla-bot bot commented Jan 26, 2024

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @ataft

Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

changelog for Fix incremental delete+insert SQL
@ataft ataft requested a review from a team as a code owner January 26, 2024 00:38
@cla-bot cla-bot bot added the cla:yes label Jan 26, 2024
@dbeatty10 dbeatty10 added the Team:Adapters Issues designated for the adapter area of the code label Jan 26, 2024
@dbeatty10
Copy link
Contributor

Thanks for raising this PR @ataft !

Could you resolve the merge conflict?

image

@ataft
Copy link
Author

ataft commented Jan 26, 2024

Here's the merge conflict message I'm getting:
image

Oddly, I don't see the global_project directory in the main branch. Not sure what's going on, see here:
https://github.com/dbt-labs/dbt-core/tree/main/core/dbt/include

@colin-rogers-dbt
Copy link
Contributor

Hi @ataft sorry for the confusion but we're in the middle of a large scale refactor of dbt.adapters. Once that has been wrapped up (in the next couple weeks) we will move your PR to the new dbt-adapters repo and review it there.

More info: #9171

@dbeatty10 dbeatty10 added the dbt-adapters Needs migration to the dbt-adapters repo label Feb 5, 2024
@dbeatty10 dbeatty10 added the community This PR is from a community member label Mar 22, 2024
@dbeatty10 dbeatty10 added the bug Something isn't working label Apr 10, 2024
@dataders
Copy link
Contributor

hey @ataft thanks for taking the time to open this PR. Since opening, this code now lives in a separate repo: dbt-adapters. A consequence of the decoupling is that PR can't be merged anymore as is, so we're closing it. For more context see #9171.

The linked issue has already been transferred. Please don't hesitate to re-open on the dbt-adapters repo.

@dataders dataders closed this Apr 10, 2024
@ataft
Copy link
Author

ataft commented Apr 10, 2024

All good, just moved the PR over to dbt-labs/dbt-adapters#151

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla:yes community This PR is from a community member dbt-adapters Needs migration to the dbt-adapters repo Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-3493] [Bug] unique_key list incremental model has performance issues on the delete phase
4 participants