Repository: dbt-labs/dbt-core
Pull request: Fix/incremental column precision changes
resolves #5351

<!---
  Include the number of the issue addressed by this PR above if applicable.
  PRs for code changes without an associated issue *will not be merged*.
  See CONTRIBUTING.md for more information.
-->

### Description

<!---
  Describe the Pull Request here. Add any references and info to help reviewers
  understand your changes. Include any tradeoffs you considered.
-->
Consider whether column may be successfully expanded when qualifying schema changes
### Checklist

- [x] I have read [the contributing guide](https://github.com/dbt-labs/dbt-core/blob/main/CONTRIBUTING.md) and understand what's expected of me
- [x] I have signed the [CLA](https://docs.getdbt.com/docs/contributor-license-agreements)
- [x] I have run this code in development and it appears to resolve the stated issue
- [x] This PR includes tests, or tests are not required/relevant for this PR
- [x] I have [opened an issue to add/update docs](https://github.com/dbt-labs/docs.getdbt.com/issues/new/choose), or docs changes are not required/relevant for this PR
- [x] I have run `changie new` to [create a changelog entry](https://github.com/dbt-labs/dbt-core/blob/main/CONTRIBUTING.md#Adding-CHANGELOG-Entry)


Problem statement:
[CT-736] [CT-735] varchar column reduces in size on an incremental run which leads to "Value too long for character type" error
### Describe the bug

I have an incremental model with the following properties:

```
materialized="incremental",
sort="date_col",
dist="date_col",
unique_key="user_id_date",
incremental_strategy="merge",
on_schema_change="sync_all_columns"
```

The first fresh run worked fine. One of the column named `platform` (which is built from a case-when statement) was detected automatically by dbt to be of type `varchar(20)`.

On the next incremental run, `platform` column of the new portion of data only had strings of maximum 17 characters. This led to dbt trying to perform a column type change on redshift from `varchar(20)` to `varchar(17)`, and got this error:

> Value too long for character type

### Steps To Reproduce

- There's a `platform` column build from case-when in the incremental model
- First fresh run works fine
- First incremental run hits the error

### Expected behavior
The new column type should be big enough to hold both new and old values

### Screenshots and log output

From dbt log file:

```sql
In "tbl":
  Schema changed: True
  Source columns not in target: []
  Target columns not in source: []
  New column types: [{'column_name': 'platform', 'new_type': 'character varying(17)'}]

alter table "tbl" add column "platform__dbt_alter" character varying(17);
update "tbl" set "platform__dbt_alter" = "platform";
alter table "tbl" drop column "platform" cascade;
alter table "tbl" rename column "platform__dbt_alter" to "platform"

15:43:07.968169 [debug] [Thread-1  ]: Postgres adapter: Postgres error: Value too long for character type
DETAIL:  
  -----------------------------------------------
  error:  Value too long for character type
  code:      8001
  context:   Value too long for type character varying(17)
  query:     13418344
  location:  string.cpp:247
  process:   query2_263_13418344 [pid=11812]
  -----------------------------------------------
```

### System information
**The output of `dbt --version`:**
```
Core:
  - installed: 1.1.0
  - latest:    1.1.0 - Up to date!

Plugins:
  - bigquery:  1.1.0 - Up to date!
  - snowflake: 1.1.0 - Up to date!
  - redshift:  1.1.0 - Up to date!
  - postgres:  1.1.0 - Up to date!
```

**The operating system you're using:** MacOS 12.3.1

**The output of `python --version`:** Python 3.8.9

Hints from issue discussion:
@stnguyen Thanks for opening!

It sounds like `on_schema_change` logic [here](https://github.com/dbt-labs/dbt-core/blob/72b6a80b07b3acc9aa64ae65c5d82191dbb1aa11/core/dbt/include/global_project/macros/materializations/models/incremental/on_schema_change.sql#L79-L85), or more likely the logic in [`diff_column_data_types` here](https://github.com/dbt-labs/dbt-core/blob/72b6a80b07b3acc9aa64ae65c5d82191dbb1aa11/core/dbt/include/global_project/macros/materializations/models/incremental/column_helpers.sql#L44-L46), is missing logic to the effect of: If the _type_ is the same, and only the precision differs, don't treat that as a data type change. There are [some `Column` methods](https://github.com/dbt-labs/dbt-core/blob/72b6a80b07b3acc9aa64ae65c5d82191dbb1aa11/core/dbt/adapters/base/column.py#L85-L101) we might be able to use to this effect.

If the precision has _increased_ from the older data to the newer data, dbt should already be handling that during [`expand_target_column_types`](https://github.com/dbt-labs/dbt-core/blob/72b6a80b07b3acc9aa64ae65c5d82191dbb1aa11/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql#L40-L42), which occurs right before schema changes are detected and processed.

I'm going to transfer this issue to the `dbt-core` repo, since that's actually where the code change should happen, in the default/global project macros.
I'd like to contribute to this issue if it's not already being worked on :) 
@epapineau I'm good with you contributing if you are up for this! @jtcohen6 is this good with you as well?
very much so! excited to have you back for round 2, @epapineau :)
@epapineau I'm going to assign this to you just so we know you are working on it. If for any reason you have to drop it, just remove yourself so we can keep track. Thanks again for the help here!

Commit message: Move test from integration to adapter tests

Patch to review:
diff --git a/.changes/unreleased/Fixes-20220617-193731.yaml b/.changes/unreleased/Fixes-20220617-193731.yaml
new file mode 100644
index 00000000000..dc52163d3f3
--- /dev/null
+++ b/.changes/unreleased/Fixes-20220617-193731.yaml
@@ -0,0 +1,7 @@
+kind: Fixes
+body: 'Resolves #5351 - Do not consider shorter varchar cols as schema changes'
+time: 2022-06-17T19:37:31.885484-07:00
+custom:
+  Author: epapineau
+  Issue: "5351"
+  PR: "5395"
diff --git a/core/dbt/include/global_project/macros/materializations/models/incremental/column_helpers.sql b/core/dbt/include/global_project/macros/materializations/models/incremental/column_helpers.sql
index f8f34051be9..95e35f31f70 100644
--- a/core/dbt/include/global_project/macros/materializations/models/incremental/column_helpers.sql
+++ b/core/dbt/include/global_project/macros/materializations/models/incremental/column_helpers.sql
@@ -41,7 +41,7 @@
   {% for sc in source_columns %}
     {% set tc = target_columns | selectattr("name", "equalto", sc.name) | list | first %}
     {% if tc %}
-      {% if sc.data_type != tc.data_type %}
+      {% if sc.data_type != tc.data_type and not sc.can_expand_to(other_column=tc) %}
         {{ result.append( { 'column_name': tc.name, 'new_type': sc.data_type } ) }}
       {% endif %}
     {% endif %}
diff --git a/tests/adapter/dbt/tests/adapter/basic/files.py b/tests/adapter/dbt/tests/adapter/basic/files.py
index 5772233e7d0..3b825b5e4a9 100644
--- a/tests/adapter/dbt/tests/adapter/basic/files.py
+++ b/tests/adapter/dbt/tests/adapter/basic/files.py
@@ -198,6 +198,17 @@
   select * from {{ ref('ephemeral') }}
 """
 
+incremental_not_schema_change_sql = """
+{{ config(materialized="incremental", unique_key="user_id_current_time") }}
+select
+    1 || '-' || current_timestamp as user_id_current_time,
+    {% if is_incremental() %}
+        'thisis18characters' as platform
+    {% else %}
+        'okthisis20characters' as platform
+    {% endif %}
+"""
+
 base_materialized_var_sql = config_materialized_var + model_base
 base_table_sql = config_materialized_table + model_base
 base_view_sql = config_materialized_view + model_base
diff --git a/tests/adapter/dbt/tests/adapter/basic/test_incremental.py b/tests/adapter/dbt/tests/adapter/basic/test_incremental.py
index 7f6e8b3aa45..85f9c54a6ac 100644
--- a/tests/adapter/dbt/tests/adapter/basic/test_incremental.py
+++ b/tests/adapter/dbt/tests/adapter/basic/test_incremental.py
@@ -1,10 +1,12 @@
 import pytest
 from dbt.tests.util import run_dbt, check_relations_equal, relation_from_name
+from dbt.contracts.results import RunStatus
 from dbt.tests.adapter.basic.files import (
     seeds_base_csv,
     seeds_added_csv,
     schema_base_yml,
     incremental_sql,
+    incremental_not_schema_change_sql,
 )
 
 
@@ -58,5 +60,30 @@ def test_incremental(self, project):
         assert len(catalog.sources) == 1
 
 
+class BaseIncrementalNotSchemaChange:
+    @pytest.fixture(scope="class")
+    def project_config_update(self):
+        return {"name": "incremental"}
+
+    @pytest.fixture(scope="class")
+    def models(self):
+        return {"incremental_not_schema_change.sql": incremental_not_schema_change_sql}
+
+    def test_incremental_not_schema_change(self, project):
+        # Schema change is not evaluated on first run, so two are needed
+        run_dbt(["run", "--select", "incremental_not_schema_change"])
+        run_result = (
+            run_dbt(["run", "--select", "incremental_not_schema_change"]).results[0].status
+        )
+
+        print(run_result)
+
+        assert run_result == RunStatus.Success
+
+
 class Testincremental(BaseIncremental):
     pass
+
+
+class TestBaseIncrementalNotSchemaChange(BaseIncrementalNotSchemaChange):
+    pass


Code review comments:
diff_hunk: @@ -58,5 +60,30 @@ def test_incremental(self, project):
         assert len(catalog.sources) == 1
 
 
+class BaseIncrementalNotSchemaChange:
+    @pytest.fixture(scope="class")
+    def project_config_update(self):
+        return {"name": "incremental"}
+
+    @pytest.fixture(scope="class")
+    def models(self):
+        return {"incremental_not_schema_change.sql": incremental_not_schema_change_sql}
+
+    def test_incremental_not_schema_change(self, project):
+        # Schema change is not evaluated on first run, so two are needed
+        run_dbt(["run", "--select", "incremental_not_schema_change"])
+        run_result = (
+            run_dbt(["run", "--select", "incremental_not_schema_change"]).results[0].status
+        )
+
+        print(run_result)
path: tests/adapter/dbt/tests/adapter/basic/test_incremental.py
text: @user1:
This can be removed before merge

@author:
Oops! Removed.