fix: Remove catalog from the DDL SQL generated by on_schema_change=sync_all_columns #684
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iconara
requested review from
jessedobbelaere,
Jrmyy,
mattiamatrix,
nicor88 and
svdimchenko
as code owners
nicor88
reviewed
Jul 4, 2024
| from dbt.contracts.results import RunStatus | ||
| from dbt.tests.util import run_dbt, run_dbt_and_capture | ||
|
|
||
| models__table_base_model = """ |
There was a problem hiding this comment.
great test, how about replicating it for iceberg tables as well, just to be sure that we don't break anything in there?
There was a problem hiding this comment.
I parameterized the tests so that they now run once with table_type=hive and once with table_type=iceberg. The file name was "test_hive_on_schema_change" and I renamed it to "test_on_schema_change" since it covers both table types.
nicor88
reviewed
Jul 4, 2024
|
@iconara thanks for your PR, there are some issues with some functional tests, could you have a look at them? |
Using on_schema_change=sync_all_columns would result in an ALTER TABLE … REPLACE COLUMNS that had a relation that included the catalog, e.g. `awsdatacatalog`.`some_schema`.`some_table`. Hive DDL does not support this three-component relation specification, only `some_schema`.`some_table` is supported. This change removes the database (i.e. `awsdatacatalog`) component from all Hive DDL.
The previous commit included tests for the sync_all_columns mode, since that had a bug. There are no functional tests that cover the on_schema_change modes, so this adds coverage for the other three modes. I also added an additional check to ensure the column names have actually been changed (or not changed).
iconara
force-pushed
the
fix-alter-table-replace-columns
branch
from
2b458fc to
d60b2fb
Compare
iconara
commented
Jul 5, 2024
|
I fixed the functional test that failed because of my change, but now a different functional test is failing. It's not failing locally, and I can't figure out why it fails from the output. It's |
Parameterize the on_schema_change functional tests to run once with table_type=hive and once with table_type=iceberg so that both modes are covered. Since the file is called "test_hive_…" rename it without "hive_".
iconara
force-pushed
the
fix-alter-table-replace-columns
branch
from
d60b2fb to
644f69a
Compare
|
I forced the tests to re-run by touching the last commit and they succeeded this time. Looks like Iceberg retry tests are not stable. |
ah, I see, I tried to address this issue adding more retries, but as I was in a rush yesterday I didn't spot what went wrong. |
nicor88
reviewed
Jul 5, 2024
Co-authored-by: nicor88 <[email protected]>
nicor88
approved these changes
Jul 5, 2024
kodiakhq bot
referenced
this pull request
in cloudquery/policies
Sep 1, 2024
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [dbt-athena-community](https://togithub.com/dbt-athena/dbt-athena) | patch | `==1.8.3` -> `==1.8.4` | --- ### Release Notes <details> <summary>dbt-athena/dbt-athena (dbt-athena-community)</summary> ### [`v1.8.4`](https://togithub.com/dbt-athena/dbt-athena/releases/tag/v1.8.4) [Compare Source](https://togithub.com/dbt-athena/dbt-athena/compare/v1.8.3...v1.8.4) #### What's Changed ##### Fixes - fix: Remove catalog from the DDL SQL generated by on_schema_change=sync_all_columns by [@​iconara](https://togithub.com/iconara) in [https://github.com/dbt-athena/dbt-athena/pull/684](https://togithub.com/dbt-athena/dbt-athena/pull/684) - fix: Query comment for create table statement by [@​sanromeo](https://togithub.com/sanromeo) in [https://github.com/dbt-athena/dbt-athena/pull/702](https://togithub.com/dbt-athena/dbt-athena/pull/702) - fix: remove leading whitespaces on post-hook operations by [@​sanromeo](https://togithub.com/sanromeo) in [https://github.com/dbt-athena/dbt-athena/pull/705](https://togithub.com/dbt-athena/dbt-athena/pull/705) - fix: vacuum more runs needed error by [@​Jrmyy](https://togithub.com/Jrmyy) in [https://github.com/dbt-athena/dbt-athena/pull/703](https://togithub.com/dbt-athena/dbt-athena/pull/703) ##### Dependencies - chore: Update dbt-tests-adapter requirement from ~=1.9.1 to ~=1.9.2 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/dbt-athena/dbt-athena/pull/687](https://togithub.com/dbt-athena/dbt-athena/pull/687) - chore: Update pytest requirement from ~=8.2 to ~=8.3 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/dbt-athena/dbt-athena/pull/690](https://togithub.com/dbt-athena/dbt-athena/pull/690) - chore: Update pyupgrade requirement from ~=3.16 to ~=3.17 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/dbt-athena/dbt-athena/pull/692](https://togithub.com/dbt-athena/dbt-athena/pull/692) - chore: Update tenacity requirement from ~=8.2 to >=8.2,<10.0 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/dbt-athena/dbt-athena/pull/693](https://togithub.com/dbt-athena/dbt-athena/pull/693) - chore: Update black requirement from ~=24.4 to ~=24.8 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/dbt-athena/dbt-athena/pull/694](https://togithub.com/dbt-athena/dbt-athena/pull/694) - chore: Update boto3-stubs\[s3] requirement from ~=1.34 to ~=1.35 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/dbt-athena/dbt-athena/pull/707](https://togithub.com/dbt-athena/dbt-athena/pull/707) - chore: Update moto requirement from ~=5.0.12 to ~=5.0.13 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/dbt-athena/dbt-athena/pull/708](https://togithub.com/dbt-athena/dbt-athena/pull/708) - chore: Update pyparsing requirement from ~=3.1.2 to ~=3.1.4 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/dbt-athena/dbt-athena/pull/709](https://togithub.com/dbt-athena/dbt-athena/pull/709) #### New Contributors - [@​iconara](https://togithub.com/iconara) made their first contribution in [https://github.com/dbt-athena/dbt-athena/pull/684](https://togithub.com/dbt-athena/dbt-athena/pull/684) **Full Changelog**: dbt-labs/dbt-athena@v1.8.3...v1.8.4 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 4am on the first day of the month" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJhdXRvbWVyZ2UiXX0=-->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This resolves #677
When generating Hive DDL, the relation spec must not include the catalog name (i.e. "awsdatacatalog"), only the database (schema) and table name. #677 discovered that the DDL generated when
on_schema_changeis set tosync_all_columnsgeneratesalter table awsdatacatalog.schema_name.table_name replace columns (…)(with backticks around the relation spec components, but I don't know how to make Markdown include literal backticks). This DDL fails with a syntax error.I don't know why all DDL doesn't fail, why this was only a problem when
on_schema_changeis set tosync_all_columns. For example, I can see multipleALTER TABLE … SET TBLPROPERTIES …that are run when I run the functional tests, and none of them have the catalog in the relation spec.My fix makes sure that the catalog is never included when rendering relation names in Hive DDL.
There were no functional tests that covered
on_schema_change, and since I had to add one to figure out why #677 happened, I added tests for all four modes, even though only thesync_all_columnsmode changes in this PR.Checklist