Commit ec229f8
Fix: Retain multiple foreign-keys from one table to another after an incremental run (#1296)
<!-- Please review our pull request review process in CONTRIBUTING.md
before your proceed. -->
<!---
Include the number of the issue addressed by this PR above if
applicable.
Example:
resolves #1234
Please review our pull request review process in CONTRIBUTING.md before
your proceed.
-->
### Description
Encountered a bug when:
* `"use_materialization_v2": False`
* `config.contract.enforced: true`
* Multiple foreign keys are specified from one table to another (e.g. a
fact table with multiple date values, each of which has a foreign key
constrain to a common data dimension)
* An initial full run of the model with the FK constraints is carried
out, followed by an incremental run.
The bug is that on the initial full run the multiple FK constraints to
the common table are created correctly on the Databricks table, but then
when an incremental run is carried out these FKs are removed from the
Databricks table. Single FKs to other tables seem to be retained (this
isn't replicated in the test case in the interest of keeping it
minimal).
The bug is replicated in the integration test
[TestIncrementalMultipleFKsToSameTableNoV2](https://github.com/databricks/dbt-databricks/compare/main...Ewan-Keith:dbt-databricks:consistenty-persist-databricks-constraints?expand=1#diff-64ad276353f7ed237d5072874d1fe18c67310f90493ce7dc8a9fc69756925335R380),
before the fix is applied this passes the first assertion (after the
initial full run) but fails the second assertion (after the incremental
run) with an empty set of constraints returned. A matching test is also
present replicating the logic when using materialization_v2
[TestIncrementalMultipleFKsToSameTable](https://github.com/databricks/dbt-databricks/compare/main...Ewan-Keith:dbt-databricks:consistenty-persist-databricks-constraints?expand=1#diff-64ad276353f7ed237d5072874d1fe18c67310f90493ce7dc8a9fc69756925335R334)
which confirms the issue is not seen with v2 materialization (this
passes without any actual code changes).
The fix I've landed on in
`dbt/include/databricks/macros/materializations/incremental/incremental.sql`
is to treat constraints as one of the configuration changes that is
explicitly applied on an incremental run when changes are detected (the
same as for `tags`, `tblproperties`, and `liquid_clustering`). This gets
the failing test passing.
I don't think this is necessarily a root-cause fix, I think the root
issue is likely a change in constraints being falsely detected in this
instance, but I wasn't sure where to start on that, and this fix seems
sensible (even if the root issue is resolved it seems likely we'd want
changes to constraints to be applied without forcing a full-refresh
where this is possible?). There's a unit test checking if there are any
obvious bugs in the constraint diff checker but this doesn't turn
anything up (passes as expected).
### Checklist
- [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 updated the `CHANGELOG.md` and added information about my
change to the "dbt-databricks next" section.
---------
Signed-off-by: Ewan Keith <ewan.keith@kaluza.com>
Co-authored-by: Ben Cassell <98852248+benc-db@users.noreply.github.com>1 parent 3fa9099 commit ec229f8
File tree
5 files changed
+146
-5
lines changed- dbt
- adapters/databricks
- include/databricks/macros/materializations/incremental
- tests/functional/adapter/incremental
5 files changed
+146
-5
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
1 | 7 | | |
2 | 8 | | |
3 | 9 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
119 | 119 | | |
120 | 120 | | |
121 | 121 | | |
122 | | - | |
123 | 122 | | |
124 | | - | |
125 | | - | |
126 | | - | |
127 | | - | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
128 | 130 | | |
129 | 131 | | |
130 | 132 | | |
| |||
Lines changed: 5 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
177 | 177 | | |
178 | 178 | | |
179 | 179 | | |
| 180 | + | |
180 | 181 | | |
181 | 182 | | |
182 | 183 | | |
| |||
186 | 187 | | |
187 | 188 | | |
188 | 189 | | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
189 | 194 | | |
190 | 195 | | |
191 | 196 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1109 | 1109 | | |
1110 | 1110 | | |
1111 | 1111 | | |
| 1112 | + | |
| 1113 | + | |
| 1114 | + | |
| 1115 | + | |
| 1116 | + | |
| 1117 | + | |
| 1118 | + | |
| 1119 | + | |
| 1120 | + | |
| 1121 | + | |
| 1122 | + | |
| 1123 | + | |
| 1124 | + | |
| 1125 | + | |
| 1126 | + | |
| 1127 | + | |
| 1128 | + | |
| 1129 | + | |
| 1130 | + | |
| 1131 | + | |
| 1132 | + | |
| 1133 | + | |
| 1134 | + | |
| 1135 | + | |
| 1136 | + | |
| 1137 | + | |
| 1138 | + | |
| 1139 | + | |
| 1140 | + | |
| 1141 | + | |
| 1142 | + | |
| 1143 | + | |
| 1144 | + | |
| 1145 | + | |
| 1146 | + | |
| 1147 | + | |
| 1148 | + | |
| 1149 | + | |
| 1150 | + | |
| 1151 | + | |
| 1152 | + | |
| 1153 | + | |
| 1154 | + | |
| 1155 | + | |
| 1156 | + | |
| 1157 | + | |
| 1158 | + | |
| 1159 | + | |
| 1160 | + | |
| 1161 | + | |
| 1162 | + | |
| 1163 | + | |
Lines changed: 76 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
328 | 328 | | |
329 | 329 | | |
330 | 330 | | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
| 345 | + | |
| 346 | + | |
| 347 | + | |
| 348 | + | |
| 349 | + | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
| 377 | + | |
| 378 | + | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
| 385 | + | |
| 386 | + | |
| 387 | + | |
| 388 | + | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
| 396 | + | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
| 400 | + | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
0 commit comments