search

dbt-labs / dbt-athena

The athena adapter plugin for dbt (https://getdbt.com)
https://dbt-athena.github.io
Apache License 2.0
228 stars 100 forks source link

fix: Remove catalog from the DDL SQL generated by on_schema_change=sync_all_columns #684

Closed iconara closed 4 months ago

iconara commented 4 months ago

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_change is set to sync_all_columns generates alter 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_change is set to sync_all_columns. For example, I can see multiple ALTER 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 the sync_all_columns mode changes in this PR.

Checklist

nicor88 commented 4 months ago

@iconara thanks for your PR, there are some issues with some functional tests, could you have a look at them?

iconara commented 4 months ago

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 TestIcebergRetriesDisabled.test__retries_iceberg that fails, and it seems to fail because the run doesn't fail as expected.

iconara commented 4 months ago

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.

nicor88 commented 4 months ago

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.