Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: snapshots failing because of wrong mysql colum quoting #171

Merged

Conversation

outinim
Copy link

@outinim outinim commented Apr 4, 2024

Description

When I do snapshots on a table on which i add columns: it use the function (if I read correctly the file dbt/include/mariadb/macros/materializations/snapshot/snapshot.sql) {% do create_columns(target_relation, missing_columns) %}
which compiles into (for my source table users with a new column team) into
alter table sandbox.users_snapshot add column "team" varchar(10);
But it needs to be add column team to work in mysql

Checklist

  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md with information about my change

Copy link
Collaborator

@mwallace582 mwallace582 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some comments, hopefully they're helpful. It'd would also be great if you'd be able to add a unit test for this new functionality.

@outinim
Copy link
Author

outinim commented Apr 7, 2024

Something like this is better ? Thanks for the reviews guys

@@ -59,7 +70,7 @@
| rejectattr('name', 'equalto', 'DBT_UNIQUE_KEY')
| list %}

{% do create_columns(target_relation, missing_columns) %}
{% do mysql5__create_columns(target_relation, missing_columns) %}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change (and the others like it) is not necessary. The create_columns() macro will automatically call the adapter's specific macro if one exists (which it now does).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made this change myself so that we can get this merged.

@mwallace582
Copy link
Collaborator

Sorry for the delay in taking a look at your changes. This is looking a lot better, and I've just got the one new comment.

It'd also be great if you'd be able to add some tests around this, although I'd be willing to merge this without them given the small scope of the changes.

@mwallace582 mwallace582 added the ok to test This pull request can run integration tests label May 27, 2024
@mwallace582 mwallace582 merged commit e609d4d into dbeatty10:1.7.latest May 27, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok to test This pull request can run integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants