From 5436674e4e138b9c3f1a1406a3867436f3fc6901 Mon Sep 17 00:00:00 2001 From: Mila Page <67295367+VersusFacit@users.noreply.github.com> Date: Thu, 12 Jan 2023 12:31:01 -0800 Subject: [PATCH 1/5] Mp/reduce ci daily run intervals (#392) * Only run at 7:00 am est (before folks get into the office) * Split jobs and adjust timestamps. * Trim comments. * Need both workflows on each job. Co-authored-by: Mila Page --- .github/workflows/main-branch-tests.yml | 59 ++++++++++++++++++++++ .github/workflows/release-branch-tests.yml | 2 +- 2 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/main-branch-tests.yml diff --git a/.github/workflows/main-branch-tests.yml b/.github/workflows/main-branch-tests.yml new file mode 100644 index 000000000..fcf69d027 --- /dev/null +++ b/.github/workflows/main-branch-tests.yml @@ -0,0 +1,59 @@ +# **what?** +# The purpose of this workflow is to trigger CI to run for main on a regular +# cadence. If the CI workflow fails for a branch, it will post to +# dev-core-alerts to raise awareness. The 'aurelien-baudet/workflow-dispatch' + +# **why?** +# Ensures main is always shippable and not broken. Also, can catch any +# dependencies shifting beneath us that might introduce breaking changes +# (could also impact Cloud). + +# **when?** +# Mainly on a schedule of 9:00, 13:00, 18:00 UTC everyday. +# Manual trigger can also test on demand + +name: Main branch scheduled testing + +on: + schedule: + - cron: '0 9,13,18 * * *' # 9:00, 13:00, 18:00 UTC + + workflow_dispatch: # for manual triggering + +# no special access is needed +permissions: read-all + +jobs: + kick-off-ci: + name: Kick-off CI + runs-on: ubuntu-latest + + strategy: + # must run CI 1 branch at a time b/c the workflow-dispatch Action polls for + # latest run for results and it gets confused when we kick off multiple runs + # at once. There is a race condition so we will just run in sequential order. + max-parallel: 1 + fail-fast: false + matrix: + branch: [main] + workflow_name: [main.yml, integration.yml] + + steps: + - name: Call CI workflow for ${{ matrix.branch }} branch + id: trigger-step + uses: aurelien-baudet/workflow-dispatch@v2.1.1 + with: + workflow: ${{ matrix.workflow_name }} + ref: ${{ matrix.branch }} + token: ${{ secrets.FISHTOWN_BOT_PAT }} + + - name: Post failure to Slack + uses: ravsamhq/notify-slack-action@v1 + if: ${{ always() && !contains(steps.trigger-step.outputs.workflow-conclusion,'success') }} + with: + status: ${{ job.status }} + notification_title: 'dbt-snowflake scheduled run of ${{ matrix.workflow_name }} on "${{ matrix.branch }}" branch not successful' + message_format: ':x: ${{ matrix.workflow_name }} CI on branch "${{ matrix.branch }}" ${{ steps.trigger-step.outputs.workflow-conclusion }}' + footer: 'Linked failed CI run ${{ steps.trigger-step.outputs.workflow-url }}' + env: + SLACK_WEBHOOK_URL: ${{ secrets.SLACK_DEV_CORE_ALERTS }} diff --git a/.github/workflows/release-branch-tests.yml b/.github/workflows/release-branch-tests.yml index 802f03b54..4111925ab 100644 --- a/.github/workflows/release-branch-tests.yml +++ b/.github/workflows/release-branch-tests.yml @@ -39,7 +39,7 @@ jobs: max-parallel: 1 fail-fast: false matrix: - branch: [1.0.latest, 1.1.latest, 1.2.latest, 1.3.latest, main] + branch: [1.0.latest, 1.1.latest, 1.2.latest, 1.3.latest, 1.4.latest] workflow_name: [main.yml, integration.yml] steps: From 0fdb0fc11a6cf2272beb3475e9b6f33bd16bf2fe Mon Sep 17 00:00:00 2001 From: Matthew McKnight <91097623+McKnight-42@users.noreply.github.com> Date: Tue, 17 Jan 2023 15:33:51 -0600 Subject: [PATCH 2/5] [CT-1507] create config for incremental models to use temp tables or views to fix regression in perf (#389) * init push for pr to work on regression in temp tables vs views for dbt-snowflake * adding additional table selection logic, functionality works locally based off snowflake query history * extend condition logic to smaller more readable elif's take into account some lost table options * add changie * breaking out language selection for more readability, reorganizing conditional flow * typo fix * spacing fix * change messaging for incremental model strategy selection * add direct reference to delete+insert * changing comment formatting and structure --- .../unreleased/Features-20230112-162620.yaml | 8 ++++ dbt/adapters/snowflake/impl.py | 1 + .../macros/materializations/incremental.sql | 38 ++++++++++++++----- 3 files changed, 37 insertions(+), 10 deletions(-) create mode 100644 .changes/unreleased/Features-20230112-162620.yaml diff --git a/.changes/unreleased/Features-20230112-162620.yaml b/.changes/unreleased/Features-20230112-162620.yaml new file mode 100644 index 000000000..85707d99d --- /dev/null +++ b/.changes/unreleased/Features-20230112-162620.yaml @@ -0,0 +1,8 @@ +kind: Features +body: add back optional old uses of temp tables for incremental models in snowflake + for perf increase in some cases +time: 2023-01-12T16:26:20.813848-06:00 +custom: + Author: McKnight-42, dbeatty10 + Issue: "323" + PR: "389" diff --git a/dbt/adapters/snowflake/impl.py b/dbt/adapters/snowflake/impl.py index 5089395f9..c1551138c 100644 --- a/dbt/adapters/snowflake/impl.py +++ b/dbt/adapters/snowflake/impl.py @@ -27,6 +27,7 @@ class SnowflakeConfig(AdapterConfig): copy_grants: Optional[bool] = None snowflake_warehouse: Optional[str] = None query_tag: Optional[str] = None + merge_tmp_relation_type: Optional[str] = None merge_update_columns: Optional[str] = None diff --git a/dbt/include/snowflake/macros/materializations/incremental.sql b/dbt/include/snowflake/macros/materializations/incremental.sql index b9d4c46dd..32e1407ac 100644 --- a/dbt/include/snowflake/macros/materializations/incremental.sql +++ b/dbt/include/snowflake/macros/materializations/incremental.sql @@ -1,19 +1,37 @@ {% macro dbt_snowflake_get_tmp_relation_type(strategy, unique_key, language) %} - + {%- set merge_tmp_relation_type = config.get('merge_tmp_relation_type', default="view") -%} /* {# + High-level principles: If we are running multiple statements (DELETE + INSERT), - we must first save the model query results as a temporary table - in order to guarantee consistent inputs to both statements. - + and we want to guarantee identical inputs to both statements, + then we must first save the model query results as a temporary table + (which presumably comes with a performance cost). If we are running a single statement (MERGE or INSERT alone), - we can save the model query definition as a view instead, - for faster overall incremental processing. + we _may_ save the model query definition as a view instead, + for (presumably) faster overall incremental processing. + + Low-level specifics: + Languages other than SQL (like Python) will use a temporary table. + With the default strategy of merge, the user may choose between a temporary + table and view (defaulting to view). + The append strategy can use a view because it will run a single INSERT statement. + When the unique_key is none, + then we can use a view because it will run a single INSERT statement. + Otherwise, play it safe by using a temporary table. #} */ - {% if language == 'sql' and (strategy in ('default', 'append', 'merge') or (unique_key is none)) %} - {{ return('view') }} - {% else %} {#-- play it safe -- #} - {{ return('table') }} + {% if language != "sql" %} + {{ return("table") }} + {% elif strategy in ("default", "merge") and merge_tmp_relation_type == "table" %} + {{ return("table") }} + {% elif strategy in ("default", "merge") and merge_tmp_relation_type == "view" %} + {{ return("view") }} + {% elif strategy in ("default", "merge", "append") %} + {{ return("view") }} + {% elif strategy == "delete+insert" and unique_key is none %} + {{ return("view") }} + {% else %} + {{ return("table") }} {% endif %} {% endmacro %} From 0fb747f42755f607413724f0f4cbef0b87321e6d Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Thu, 19 Jan 2023 13:47:58 +0100 Subject: [PATCH 3/5] Convert incremental on_schema_change tests (#398) * Convert incremental on_schema_change tests * Switch to dbt-core main --- .../test_incremental_on_schema_change.py | 4 + .../test_incremental_predicates.py | 0 .../test_incremental_run_result.py | 0 .../test_incremental_unique_id.py | 0 .../models/incremental_append_new_columns.sql | 29 ---- ...remental_append_new_columns_remove_one.sql | 28 ---- ...l_append_new_columns_remove_one_target.sql | 19 --- .../incremental_append_new_columns_target.sql | 19 --- .../models/incremental_fail.sql | 19 --- .../models/incremental_ignore.sql | 19 --- .../models/incremental_ignore_target.sql | 15 -- .../models/incremental_sync_all_columns.sql | 31 ---- .../incremental_sync_all_columns_target.sql | 20 --- .../models/model_a.sql | 22 --- .../models/schema.yml | 68 --------- .../test_incremental_schema.py | 142 ------------------ .../tests/select_from_a.sql | 1 - ...ct_from_incremental_append_new_columns.sql | 1 - ...remental_append_new_columns_remove_one.sql | 1 - ...l_append_new_columns_remove_one_target.sql | 1 - ..._incremental_append_new_columns_target.sql | 1 - .../tests/select_from_incremental_ignore.sql | 1 - .../select_from_incremental_ignore_target.sql | 1 - ...lect_from_incremental_sync_all_columns.sql | 1 - ...om_incremental_sync_all_columns_target.sql | 1 - 25 files changed, 4 insertions(+), 440 deletions(-) create mode 100644 tests/functional/adapter/incremental/test_incremental_on_schema_change.py rename tests/functional/adapter/{ => incremental}/test_incremental_predicates.py (100%) rename tests/functional/adapter/{ => incremental}/test_incremental_run_result.py (100%) rename tests/functional/adapter/{ => incremental}/test_incremental_unique_id.py (100%) delete mode 100644 tests/integration/incremental_schema_tests/models/incremental_append_new_columns.sql delete mode 100644 tests/integration/incremental_schema_tests/models/incremental_append_new_columns_remove_one.sql delete mode 100644 tests/integration/incremental_schema_tests/models/incremental_append_new_columns_remove_one_target.sql delete mode 100644 tests/integration/incremental_schema_tests/models/incremental_append_new_columns_target.sql delete mode 100644 tests/integration/incremental_schema_tests/models/incremental_fail.sql delete mode 100644 tests/integration/incremental_schema_tests/models/incremental_ignore.sql delete mode 100644 tests/integration/incremental_schema_tests/models/incremental_ignore_target.sql delete mode 100644 tests/integration/incremental_schema_tests/models/incremental_sync_all_columns.sql delete mode 100644 tests/integration/incremental_schema_tests/models/incremental_sync_all_columns_target.sql delete mode 100644 tests/integration/incremental_schema_tests/models/model_a.sql delete mode 100644 tests/integration/incremental_schema_tests/models/schema.yml delete mode 100644 tests/integration/incremental_schema_tests/test_incremental_schema.py delete mode 100644 tests/integration/incremental_schema_tests/tests/select_from_a.sql delete mode 100644 tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns.sql delete mode 100644 tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns_remove_one.sql delete mode 100644 tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns_remove_one_target.sql delete mode 100644 tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns_target.sql delete mode 100644 tests/integration/incremental_schema_tests/tests/select_from_incremental_ignore.sql delete mode 100644 tests/integration/incremental_schema_tests/tests/select_from_incremental_ignore_target.sql delete mode 100644 tests/integration/incremental_schema_tests/tests/select_from_incremental_sync_all_columns.sql delete mode 100644 tests/integration/incremental_schema_tests/tests/select_from_incremental_sync_all_columns_target.sql diff --git a/tests/functional/adapter/incremental/test_incremental_on_schema_change.py b/tests/functional/adapter/incremental/test_incremental_on_schema_change.py new file mode 100644 index 000000000..192097bc5 --- /dev/null +++ b/tests/functional/adapter/incremental/test_incremental_on_schema_change.py @@ -0,0 +1,4 @@ +from dbt.tests.adapter.incremental.test_incremental_on_schema_change import BaseIncrementalOnSchemaChange + +class TestIncrementalOnSchemaChange(BaseIncrementalOnSchemaChange): + pass diff --git a/tests/functional/adapter/test_incremental_predicates.py b/tests/functional/adapter/incremental/test_incremental_predicates.py similarity index 100% rename from tests/functional/adapter/test_incremental_predicates.py rename to tests/functional/adapter/incremental/test_incremental_predicates.py diff --git a/tests/functional/adapter/test_incremental_run_result.py b/tests/functional/adapter/incremental/test_incremental_run_result.py similarity index 100% rename from tests/functional/adapter/test_incremental_run_result.py rename to tests/functional/adapter/incremental/test_incremental_run_result.py diff --git a/tests/functional/adapter/test_incremental_unique_id.py b/tests/functional/adapter/incremental/test_incremental_unique_id.py similarity index 100% rename from tests/functional/adapter/test_incremental_unique_id.py rename to tests/functional/adapter/incremental/test_incremental_unique_id.py diff --git a/tests/integration/incremental_schema_tests/models/incremental_append_new_columns.sql b/tests/integration/incremental_schema_tests/models/incremental_append_new_columns.sql deleted file mode 100644 index 18d0d5d88..000000000 --- a/tests/integration/incremental_schema_tests/models/incremental_append_new_columns.sql +++ /dev/null @@ -1,29 +0,0 @@ -{{ - config( - materialized='incremental', - unique_key='id', - on_schema_change='append_new_columns' - ) -}} - -{% set string_type = 'string' if target.type == 'bigquery' else 'varchar(10)' %} - -WITH source_data AS (SELECT * FROM {{ ref('model_a') }} ) - -{% if is_incremental() %} - -SELECT id, - cast(field1 as {{string_type}}) as field1, - cast(field2 as {{string_type}}) as field2, - cast(field3 as {{string_type}}) as field3, - cast(field4 as {{string_type}}) as field4 -FROM source_data WHERE id NOT IN (SELECT id from {{ this }} ) - -{% else %} - -SELECT id, - cast(field1 as {{string_type}}) as field1, - cast(field2 as {{string_type}}) as field2 -FROM source_data where id <= 3 - -{% endif %} \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/models/incremental_append_new_columns_remove_one.sql b/tests/integration/incremental_schema_tests/models/incremental_append_new_columns_remove_one.sql deleted file mode 100644 index 19c8ea616..000000000 --- a/tests/integration/incremental_schema_tests/models/incremental_append_new_columns_remove_one.sql +++ /dev/null @@ -1,28 +0,0 @@ -{{ - config( - materialized='incremental', - unique_key='id', - on_schema_change='append_new_columns' - ) -}} - -{% set string_type = 'string' if target.type == 'bigquery' else 'varchar(10)' %} - -WITH source_data AS (SELECT * FROM {{ ref('model_a') }} ) - -{% if is_incremental() %} - -SELECT id, - cast(field1 as {{string_type}}) as field1, - cast(field3 as {{string_type}}) as field3, - cast(field4 as {{string_type}}) as field4 -FROM source_data WHERE id NOT IN (SELECT id from {{ this }} ) - -{% else %} - -SELECT id, - cast(field1 as {{string_type}}) as field1, - cast(field2 as {{string_type}}) as field2 -FROM source_data where id <= 3 - -{% endif %} \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/models/incremental_append_new_columns_remove_one_target.sql b/tests/integration/incremental_schema_tests/models/incremental_append_new_columns_remove_one_target.sql deleted file mode 100644 index 419fdf96b..000000000 --- a/tests/integration/incremental_schema_tests/models/incremental_append_new_columns_remove_one_target.sql +++ /dev/null @@ -1,19 +0,0 @@ -{{ - config(materialized='table') -}} - -{% set string_type = 'string' if target.type == 'bigquery' else 'varchar(10)' %} - -with source_data as ( - - select * from {{ ref('model_a') }} - -) - -select id, - cast(field1 as {{string_type}}) as field1, - cast(CASE WHEN id > 3 THEN NULL ELSE field2 END as {{string_type}}) AS field2, - cast(CASE WHEN id <= 3 THEN NULL ELSE field3 END as {{string_type}}) AS field3, - cast(CASE WHEN id <= 3 THEN NULL ELSE field4 END as {{string_type}}) AS field4 - -from source_data \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/models/incremental_append_new_columns_target.sql b/tests/integration/incremental_schema_tests/models/incremental_append_new_columns_target.sql deleted file mode 100644 index 55ed7b2c5..000000000 --- a/tests/integration/incremental_schema_tests/models/incremental_append_new_columns_target.sql +++ /dev/null @@ -1,19 +0,0 @@ -{{ - config(materialized='table') -}} - -{% set string_type = 'string' if target.type == 'bigquery' else 'varchar(10)' %} - -with source_data as ( - - select * from {{ ref('model_a') }} - -) - -select id - ,cast(field1 as {{string_type}}) as field1 - ,cast(field2 as {{string_type}}) as field2 - ,cast(CASE WHEN id <= 3 THEN NULL ELSE field3 END as {{string_type}}) AS field3 - ,cast(CASE WHEN id <= 3 THEN NULL ELSE field4 END as {{string_type}}) AS field4 - -from source_data \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/models/incremental_fail.sql b/tests/integration/incremental_schema_tests/models/incremental_fail.sql deleted file mode 100644 index 590f5b56d..000000000 --- a/tests/integration/incremental_schema_tests/models/incremental_fail.sql +++ /dev/null @@ -1,19 +0,0 @@ -{{ - config( - materialized='incremental', - unique_key='id', - on_schema_change='fail' - ) -}} - -WITH source_data AS (SELECT * FROM {{ ref('model_a') }} ) - -{% if is_incremental() %} - -SELECT id, field1, field2 FROM source_data - -{% else %} - -SELECT id, field1, field3 FROm source_data - -{% endif %} \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/models/incremental_ignore.sql b/tests/integration/incremental_schema_tests/models/incremental_ignore.sql deleted file mode 100644 index 51dee6022..000000000 --- a/tests/integration/incremental_schema_tests/models/incremental_ignore.sql +++ /dev/null @@ -1,19 +0,0 @@ -{{ - config( - materialized='incremental', - unique_key='id', - on_schema_change='ignore' - ) -}} - -WITH source_data AS (SELECT * FROM {{ ref('model_a') }} ) - -{% if is_incremental() %} - -SELECT id, field1, field2, field3, field4 FROM source_data WHERE id NOT IN (SELECT id from {{ this }} ) - -{% else %} - -SELECT id, field1, field2 FROM source_data LIMIT 3 - -{% endif %} \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/models/incremental_ignore_target.sql b/tests/integration/incremental_schema_tests/models/incremental_ignore_target.sql deleted file mode 100644 index 92d4564e0..000000000 --- a/tests/integration/incremental_schema_tests/models/incremental_ignore_target.sql +++ /dev/null @@ -1,15 +0,0 @@ -{{ - config(materialized='table') -}} - -with source_data as ( - - select * from {{ ref('model_a') }} - -) - -select id - ,field1 - ,field2 - -from source_data \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/models/incremental_sync_all_columns.sql b/tests/integration/incremental_schema_tests/models/incremental_sync_all_columns.sql deleted file mode 100644 index 56a3e3c0f..000000000 --- a/tests/integration/incremental_schema_tests/models/incremental_sync_all_columns.sql +++ /dev/null @@ -1,31 +0,0 @@ -{{ - config( - materialized='incremental', - unique_key='id', - on_schema_change='sync_all_columns' - - ) -}} - -WITH source_data AS (SELECT * FROM {{ ref('model_a') }} ) - -{% set string_type = 'string' if target.type == 'bigquery' else 'varchar(10)' %} - -{% if is_incremental() %} - -SELECT id, - cast(field1 as {{string_type}}) as field1, - cast(field3 as {{string_type}}) as field3, -- to validate new fields - cast(field4 as {{string_type}}) AS field4 -- to validate new fields - -FROM source_data WHERE id NOT IN (SELECT id from {{ this }} ) - -{% else %} - -select id, - cast(field1 as {{string_type}}) as field1, - cast(field2 as {{string_type}}) as field2 - -from source_data where id <= 3 - -{% endif %} \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/models/incremental_sync_all_columns_target.sql b/tests/integration/incremental_schema_tests/models/incremental_sync_all_columns_target.sql deleted file mode 100644 index abffbf746..000000000 --- a/tests/integration/incremental_schema_tests/models/incremental_sync_all_columns_target.sql +++ /dev/null @@ -1,20 +0,0 @@ -{{ - config(materialized='table') -}} - -with source_data as ( - - select * from {{ ref('model_a') }} - -) - -{% set string_type = 'string' if target.type == 'bigquery' else 'varchar(10)' %} - -select id - ,cast(field1 as {{string_type}}) as field1 - --,field2 - ,cast(case when id <= 3 then null else field3 end as {{string_type}}) as field3 - ,cast(case when id <= 3 then null else field4 end as {{string_type}}) as field4 - -from source_data -order by id \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/models/model_a.sql b/tests/integration/incremental_schema_tests/models/model_a.sql deleted file mode 100644 index 2a0b2ddaf..000000000 --- a/tests/integration/incremental_schema_tests/models/model_a.sql +++ /dev/null @@ -1,22 +0,0 @@ -{{ - config(materialized='table') -}} - -with source_data as ( - - select 1 as id, 'aaa' as field1, 'bbb' as field2, 111 as field3, 'TTT' as field4 - union all select 2 as id, 'ccc' as field1, 'ddd' as field2, 222 as field3, 'UUU' as field4 - union all select 3 as id, 'eee' as field1, 'fff' as field2, 333 as field3, 'VVV' as field4 - union all select 4 as id, 'ggg' as field1, 'hhh' as field2, 444 as field3, 'WWW' as field4 - union all select 5 as id, 'iii' as field1, 'jjj' as field2, 555 as field3, 'XXX' as field4 - union all select 6 as id, 'kkk' as field1, 'lll' as field2, 666 as field3, 'YYY' as field4 - -) - -select id - ,field1 - ,field2 - ,field3 - ,field4 - -from source_data \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/models/schema.yml b/tests/integration/incremental_schema_tests/models/schema.yml deleted file mode 100644 index 21aa6095f..000000000 --- a/tests/integration/incremental_schema_tests/models/schema.yml +++ /dev/null @@ -1,68 +0,0 @@ -version: 2 - -models: - - name: model_a - columns: - - name: id - tags: [column_level_tag] - tests: - - unique - - - name: incremental_ignore - columns: - - name: id - tags: [column_level_tag] - tests: - - unique - - - name: incremental_ignore_target - columns: - - name: id - tags: [column_level_tag] - tests: - - unique - - - name: incremental_append_new_columns - columns: - - name: id - tags: [column_level_tag] - tests: - - unique - - - name: incremental_append_new_columns_target - columns: - - name: id - tags: [column_level_tag] - tests: - - unique - - - name: incremental_append_new_columns_remove_one - columns: - - name: id - tags: [column_level_tag] - tests: - - unique - - - name: incremental_append_new_columns_remove_one_target - columns: - - name: id - tags: [column_level_tag] - tests: - - unique - - - name: incremental_sync_all_columns - columns: - - name: id - tags: [column_level_tag] - tests: - - unique - - - name: incremental_sync_all_columns_target - columns: - - name: id - tags: [column_leveL_tag] - tests: - - unique - - - \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/test_incremental_schema.py b/tests/integration/incremental_schema_tests/test_incremental_schema.py deleted file mode 100644 index 2a752ac2f..000000000 --- a/tests/integration/incremental_schema_tests/test_incremental_schema.py +++ /dev/null @@ -1,142 +0,0 @@ -from tests.integration.base import DBTIntegrationTest, use_profile - - -class TestSelectionExpansion(DBTIntegrationTest): - @property - def schema(self): - return "test_incremental_schema" - - @property - def models(self): - return "models" - - @property - def project_config(self): - return { - "config-version": 2, - "test-paths": ["tests"] - } - - def list_tests_and_assert(self, include, exclude, expected_tests): - list_args = ['ls', '--resource-type', 'test'] - if include: - list_args.extend(('--select', include)) - if exclude: - list_args.extend(('--exclude', exclude)) - listed = self.run_dbt(list_args) - print(listed) - assert len(listed) == len(expected_tests) - test_names = [name.split('.')[-1] for name in listed] - assert sorted(test_names) == sorted(expected_tests) - - def run_tests_and_assert( - self, include, exclude, expected_tests, compare_source, compare_target - ): - - run_args = ['run'] - if include: - run_args.extend(('--models', include)) - results_one = self.run_dbt(run_args) - results_two = self.run_dbt(run_args) - - self.assertEqual(len(results_one), 3) - self.assertEqual(len(results_two), 3) - - test_args = ['test'] - if include: - test_args.extend(('--models', include)) - if exclude: - test_args.extend(('--exclude', exclude)) - - results = self.run_dbt(test_args) - tests_run = [r.node.name for r in results] - assert len(tests_run) == len(expected_tests) - assert sorted(tests_run) == sorted(expected_tests) - self.assertTablesEqual(compare_source, compare_target) - - def run_incremental_ignore(self): - select = 'model_a incremental_ignore incremental_ignore_target' - compare_source = 'incremental_ignore' - compare_target = 'incremental_ignore_target' - exclude = None - expected = [ - 'select_from_a', - 'select_from_incremental_ignore', - 'select_from_incremental_ignore_target', - 'unique_model_a_id', - 'unique_incremental_ignore_id', - 'unique_incremental_ignore_target_id' - ] - - self.list_tests_and_assert(select, exclude, expected) - self.run_tests_and_assert(select, exclude, expected, compare_source, compare_target) - - def run_incremental_append_new_columns(self): - select = 'model_a incremental_append_new_columns incremental_append_new_columns_target' - compare_source = 'incremental_append_new_columns' - compare_target = 'incremental_append_new_columns_target' - exclude = None - expected = [ - 'select_from_a', - 'select_from_incremental_append_new_columns', - 'select_from_incremental_append_new_columns_target', - 'unique_model_a_id', - 'unique_incremental_append_new_columns_id', - 'unique_incremental_append_new_columns_target_id' - ] - self.list_tests_and_assert(select, exclude, expected) - self.run_tests_and_assert(select, exclude, expected, compare_source, compare_target) - - def run_incremental_append_new_columns_remove_one(self): - select = 'model_a incremental_append_new_columns_remove_one incremental_append_new_columns_remove_one_target' - compare_source = 'incremental_append_new_columns_remove_one' - compare_target = 'incremental_append_new_columns_remove_one_target' - exclude = None - expected = [ - 'select_from_a', - 'select_from_incremental_append_new_columns_remove_one', - 'select_from_incremental_append_new_columns_remove_one_target', - 'unique_model_a_id', - 'unique_incremental_append_new_columns_remove_one_id', - 'unique_incremental_append_new_columns_remove_one_target_id' - ] - self.run_tests_and_assert(select, exclude, expected, compare_source, compare_target) - - def run_incremental_sync_all_columns(self): - select = 'model_a incremental_sync_all_columns incremental_sync_all_columns_target' - compare_source = 'incremental_sync_all_columns' - compare_target = 'incremental_sync_all_columns_target' - exclude = None - expected = [ - 'select_from_a', - 'select_from_incremental_sync_all_columns', - 'select_from_incremental_sync_all_columns_target', - 'unique_model_a_id', - 'unique_incremental_sync_all_columns_id', - 'unique_incremental_sync_all_columns_target_id' - ] - self.list_tests_and_assert(select, exclude, expected) - self.run_tests_and_assert(select, exclude, expected, compare_source, compare_target) - - def run_incremental_fail_on_schema_change(self): - select = 'model_a incremental_fail' - results_one = self.run_dbt(['run', '--models', select, '--full-refresh']) - results_two = self.run_dbt(['run', '--models', select], expect_pass = False) - self.assertIn('Compilation Error', results_two[1].message) - - @use_profile('snowflake') - def test__snowflake__run_incremental_ignore(self): - self.run_incremental_ignore() - - @use_profile('snowflake') - def test__snowflake__run_incremental_append_new_columns(self): - self.run_incremental_append_new_columns() - self.run_incremental_append_new_columns_remove_one() - - @use_profile('snowflake') - def test__snowflake__run_incremental_sync_all_columns(self): - self.run_incremental_sync_all_columns() - - @use_profile('snowflake') - def test__snowflake__run_incremental_fail_on_schema_change(self): - self.run_incremental_fail_on_schema_change() diff --git a/tests/integration/incremental_schema_tests/tests/select_from_a.sql b/tests/integration/incremental_schema_tests/tests/select_from_a.sql deleted file mode 100644 index 3dc8f2857..000000000 --- a/tests/integration/incremental_schema_tests/tests/select_from_a.sql +++ /dev/null @@ -1 +0,0 @@ -select * from {{ ref('model_a') }} where false diff --git a/tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns.sql b/tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns.sql deleted file mode 100644 index 947e84588..000000000 --- a/tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns.sql +++ /dev/null @@ -1 +0,0 @@ -select * from {{ ref('incremental_append_new_columns') }} where false \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns_remove_one.sql b/tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns_remove_one.sql deleted file mode 100644 index 06d52c6d6..000000000 --- a/tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns_remove_one.sql +++ /dev/null @@ -1 +0,0 @@ -select * from {{ ref('incremental_append_new_columns_remove_one') }} where false \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns_remove_one_target.sql b/tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns_remove_one_target.sql deleted file mode 100644 index 07d2412b0..000000000 --- a/tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns_remove_one_target.sql +++ /dev/null @@ -1 +0,0 @@ -select * from {{ ref('incremental_append_new_columns_remove_one_target') }} where false \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns_target.sql b/tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns_target.sql deleted file mode 100644 index 8b86eddd7..000000000 --- a/tests/integration/incremental_schema_tests/tests/select_from_incremental_append_new_columns_target.sql +++ /dev/null @@ -1 +0,0 @@ -select * from {{ ref('incremental_append_new_columns_target') }} where false \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/tests/select_from_incremental_ignore.sql b/tests/integration/incremental_schema_tests/tests/select_from_incremental_ignore.sql deleted file mode 100644 index d565c8464..000000000 --- a/tests/integration/incremental_schema_tests/tests/select_from_incremental_ignore.sql +++ /dev/null @@ -1 +0,0 @@ -select * from {{ ref('incremental_ignore') }} where false diff --git a/tests/integration/incremental_schema_tests/tests/select_from_incremental_ignore_target.sql b/tests/integration/incremental_schema_tests/tests/select_from_incremental_ignore_target.sql deleted file mode 100644 index 35d535c5c..000000000 --- a/tests/integration/incremental_schema_tests/tests/select_from_incremental_ignore_target.sql +++ /dev/null @@ -1 +0,0 @@ -select * from {{ ref('incremental_ignore_target') }} where false \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/tests/select_from_incremental_sync_all_columns.sql b/tests/integration/incremental_schema_tests/tests/select_from_incremental_sync_all_columns.sql deleted file mode 100644 index aedc9f803..000000000 --- a/tests/integration/incremental_schema_tests/tests/select_from_incremental_sync_all_columns.sql +++ /dev/null @@ -1 +0,0 @@ -select * from {{ ref('incremental_sync_all_columns') }} where false \ No newline at end of file diff --git a/tests/integration/incremental_schema_tests/tests/select_from_incremental_sync_all_columns_target.sql b/tests/integration/incremental_schema_tests/tests/select_from_incremental_sync_all_columns_target.sql deleted file mode 100644 index 4b703c988..000000000 --- a/tests/integration/incremental_schema_tests/tests/select_from_incremental_sync_all_columns_target.sql +++ /dev/null @@ -1 +0,0 @@ -select * from {{ ref('incremental_sync_all_columns_target') }} where false \ No newline at end of file From aa8d20fdfe14ae6ed8d51df16f42781483354b83 Mon Sep 17 00:00:00 2001 From: Matthew McKnight <91097623+McKnight-42@users.noreply.github.com> Date: Thu, 19 Jan 2023 12:18:22 -0600 Subject: [PATCH 4/5] [CT-1507] part 2: Expand configuration to all strategies (#403) * init push for pr to work on regression in temp tables vs views for dbt-snowflake * adding additional table selection logic, functionality works locally based off snowflake query history * extend condition logic to smaller more readable elif's take into account some lost table options * add changie * breaking out language selection for more readability, reorganizing conditional flow * typo fix * spacing fix * change messaging for incremental model strategy selection * add direct reference to delete+insert * changing comment formatting and structure * expand tmp_relation_type to be configurable for all incremental options * re organize conditionl logic, add leading condtional for exceptions * typo * seperate if statements * fix exception blocking * seperate execptions condtional into two small condtionals to prevent a always case * change conditional logic based on language * change delete+insert exception logic, and messages for both exceptions * change condition for more jinja language * remove default value assigned to tmp_relation_type, change delete+insert excpetion to only kick off if view is assigned and not unique_key not none * reword python exception * minor comment change, revert some condtional logic (need to deal with none) value * re add assignment of config * comment changes * put back low level part about append strategy * minor change to workign for excpetion * add changelog entry * fix previous changelog entry * change change log entry, remove some config from integration test --- .../unreleased/Features-20230112-162620.yaml | 2 +- .../Under the Hood-20230119-103909.yaml | 7 +++++ dbt/adapters/snowflake/impl.py | 2 +- .../macros/materializations/incremental.sql | 31 ++++++++++++++++--- .../simple_copy_test/test_simple_copy.py | 2 +- 5 files changed, 36 insertions(+), 8 deletions(-) create mode 100644 .changes/unreleased/Under the Hood-20230119-103909.yaml diff --git a/.changes/unreleased/Features-20230112-162620.yaml b/.changes/unreleased/Features-20230112-162620.yaml index 85707d99d..7c47dcac9 100644 --- a/.changes/unreleased/Features-20230112-162620.yaml +++ b/.changes/unreleased/Features-20230112-162620.yaml @@ -3,6 +3,6 @@ body: add back optional old uses of temp tables for incremental models in snowfl for perf increase in some cases time: 2023-01-12T16:26:20.813848-06:00 custom: - Author: McKnight-42, dbeatty10 + Author: McKnight-42 dbeatty10 Issue: "323" PR: "389" diff --git a/.changes/unreleased/Under the Hood-20230119-103909.yaml b/.changes/unreleased/Under the Hood-20230119-103909.yaml new file mode 100644 index 000000000..73483bbb2 --- /dev/null +++ b/.changes/unreleased/Under the Hood-20230119-103909.yaml @@ -0,0 +1,7 @@ +kind: Under the Hood +body: 'Enable a `tmp_relation_type` config for applicable incremental models' +time: 2023-01-19T10:39:09.774872-06:00 +custom: + Author: McKnight-42 dbeatty10 + Issue: "402" + PR: "403" diff --git a/dbt/adapters/snowflake/impl.py b/dbt/adapters/snowflake/impl.py index c1551138c..44b8228fd 100644 --- a/dbt/adapters/snowflake/impl.py +++ b/dbt/adapters/snowflake/impl.py @@ -27,7 +27,7 @@ class SnowflakeConfig(AdapterConfig): copy_grants: Optional[bool] = None snowflake_warehouse: Optional[str] = None query_tag: Optional[str] = None - merge_tmp_relation_type: Optional[str] = None + tmp_relation_type: Optional[str] = None merge_update_columns: Optional[str] = None diff --git a/dbt/include/snowflake/macros/materializations/incremental.sql b/dbt/include/snowflake/macros/materializations/incremental.sql index 32e1407ac..9172c061e 100644 --- a/dbt/include/snowflake/macros/materializations/incremental.sql +++ b/dbt/include/snowflake/macros/materializations/incremental.sql @@ -1,5 +1,5 @@ {% macro dbt_snowflake_get_tmp_relation_type(strategy, unique_key, language) %} - {%- set merge_tmp_relation_type = config.get('merge_tmp_relation_type', default="view") -%} +{%- set tmp_relation_type = config.get('tmp_relation_type') -%} /* {# High-level principles: If we are running multiple statements (DELETE + INSERT), @@ -11,20 +11,41 @@ for (presumably) faster overall incremental processing. Low-level specifics: + If an invalid option is specified, then we will raise an + excpetion with corresponding message. + Languages other than SQL (like Python) will use a temporary table. With the default strategy of merge, the user may choose between a temporary table and view (defaulting to view). + The append strategy can use a view because it will run a single INSERT statement. - When the unique_key is none, - then we can use a view because it will run a single INSERT statement. + + When unique_key is none, the delete+insert strategy can use a view beacuse a + single INSERT statement is run with no DELETES as part of the statement. Otherwise, play it safe by using a temporary table. #} */ + {% if language == "python" and tmp_relation_type is not none %} + {% do exceptions.raise_compiler_error( + "Python models currently only support 'table' for tmp_relation_type but " + ~ tmp_relation_type ~ " was specified." + ) %} + {% endif %} + + {% if strategy == "delete+insert" and tmp_relation_type is not none and tmp_relation_type != "table" and unique_key is not none %} + {% do exceptions.raise_compiler_error( + "In order to maintain consistent results when `unique_key` is not none, + the `delete+insert` strategy only supports `table` for `tmp_relation_type` but " + ~ tmp_relation_type ~ " was specified." + ) + %} + {% endif %} + {% if language != "sql" %} {{ return("table") }} - {% elif strategy in ("default", "merge") and merge_tmp_relation_type == "table" %} + {% elif tmp_relation_type == "table" %} {{ return("table") }} - {% elif strategy in ("default", "merge") and merge_tmp_relation_type == "view" %} + {% elif tmp_relation_type == "view" %} {{ return("view") }} {% elif strategy in ("default", "merge", "append") %} {{ return("view") }} diff --git a/tests/integration/simple_copy_test/test_simple_copy.py b/tests/integration/simple_copy_test/test_simple_copy.py index 6e0f7ce43..ba7c3d4d3 100644 --- a/tests/integration/simple_copy_test/test_simple_copy.py +++ b/tests/integration/simple_copy_test/test_simple_copy.py @@ -266,7 +266,7 @@ def test__snowflake__incremental_overwrite(self): # Setting the incremental_strategy should make this succeed self.use_default_project({ "models": { - "incremental_strategy": "delete+insert" + "incremental_strategy": "delete+insert", }, "seed-paths": [self.dir("snowflake-seed-update")], }) From e6d206107c41e209a58800254e2605e53299ab7a Mon Sep 17 00:00:00 2001 From: Matthew McKnight <91097623+McKnight-42@users.noreply.github.com> Date: Thu, 19 Jan 2023 12:34:39 -0600 Subject: [PATCH 5/5] remove unnded comma (#405) --- tests/integration/simple_copy_test/test_simple_copy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/simple_copy_test/test_simple_copy.py b/tests/integration/simple_copy_test/test_simple_copy.py index ba7c3d4d3..6e0f7ce43 100644 --- a/tests/integration/simple_copy_test/test_simple_copy.py +++ b/tests/integration/simple_copy_test/test_simple_copy.py @@ -266,7 +266,7 @@ def test__snowflake__incremental_overwrite(self): # Setting the incremental_strategy should make this succeed self.use_default_project({ "models": { - "incremental_strategy": "delete+insert", + "incremental_strategy": "delete+insert" }, "seed-paths": [self.dir("snowflake-seed-update")], })