dbt-labs / dbt-labs/dbt-adapters

[CT-3409] rename_relation macro should call get_rename_sql instead of defaulting to alter table

Open
#152 3 comments 0 reactions 0 assignees View on GitHub
type:documentation type:enhancement
Dominant language
Python
Stars
233
Forks
362
Avg merge
3d 22h
Merged PRs (30d)
9

Description

### Housekeeping

- [ ] I am a maintainer of dbt-core

### Short description

_Sorry, could not really find a good match in the issue templates for this._

The issue is about [this line](https://github.com/dbt-labs/dbt-core/blob/1c9cec17878ca0fec537a4cc13812f91ee97b29b/core/dbt/include/global_project/macros/relations/rename.sql#L33C1-L34C1) in the `default__rename_relation` macro.

The name of the macro implies that it would be able to rename a relation, but in fact, it's only able to rename tables as it's calling `alter table`.

We do have a better version of this in the codebase already, namely the `get_rename_sql` macro.
[That version](https://github.com/dbt-labs/dbt-core/blob/1c9cec17878ca0fec537a4cc13812f91ee97b29b/core/dbt/include/global_project/macros/relations/rename.sql#L7) is much more extensive and is capable of renaming all supported relations.

My proposal would be to forward the `alter table` call in `rename_relation` to `get_rename_sql`, so that we end up with something like this:

```sql+jinja
{% macro default__rename_relation(from_relation, to_relation) -%}
{% set target_name = adapter.quote_as_configured(to_relation.identifier, 'identifier') %}
{{ log('from_relation: ' ~ from_relation, info=True) }}
{% call statement('rename_relation') -%}
{{ get_rename_sql(from_relation, target_name) }}
{%- endcall %}
{% endmacro %}
```

Right now, adapter maintainers who'd like to simply rename relations without having to deal with the hassle of calling `get_rename_sql` directly, have to create such an override in their own adapter. That is a bit tedious and probably not intended.

### Acceptance criteria

- All current tests still succeed

### Impact to Other Teams

I don't foresee any impact except that adapter maintainers can drop their custom override. The original behavior where the macro calls `alter table` in the end will still work. The difference in behavior would be that it no longer calls `alter table` for views, but `alter view` instead and likewise for materialized views.

### Will backports be required?

no

### Context

_No response_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.