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 snapshot_check_all_get_existing_columns: use adapter.get_columns_in_relation #5232

Merged
merged 1 commit into from
May 11, 2022

Conversation

jtcohen6
Copy link
Contributor

#5223 caused failing tests overnight in dbt-spark + dbt-databricks

The code was written to use the get_columns_in_relation macro, rather than the get_columns_in_relation adapter method (Python). We should aim for consistency between these—but elsewhere in dbt-core, we do call it as adapter.get_columns_in_relation, so we should do the same here.

Here's the breakdown:

  • For most "SQL adapters," the adapter method just redirects to the macro (SQL): source
  • In dbt-bigquery, the macro redirects to the adapter method, since all the actual logic is in Python (no SQL involved): source
  • In dbt-spark, the logic is written in both Python and SQL. The adapter method calls the macro, but it also does some of its own post-processing, which is missed if you just call the macro directly. I'll open a follow-up PR to rework this for consistency, but the change in this PR should be sufficient to get tests passing again.

Checklist

@jtcohen6 jtcohen6 added the Team:Adapters Issues designated for the adapter area of the code label May 11, 2022
@jtcohen6 jtcohen6 requested a review from a team May 11, 2022 10:39
@jtcohen6 jtcohen6 requested review from a team as code owners May 11, 2022 10:39
@cla-bot cla-bot bot added the cla:yes label May 11, 2022
jtcohen6 added a commit to dbt-labs/dbt-spark that referenced this pull request May 11, 2022
@jtcohen6
Copy link
Contributor Author

PR to check whether this fixes the failing tests in CI: dbt-labs/dbt-spark#353

Copy link
Contributor

@McKnight-42 McKnight-42 left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, great catch!

Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Can affirm that this is the only aberrant instance -- all other materializations within the global project are currently using adapter.get_columns_in_relation

@jtcohen6 jtcohen6 merged commit 72c17c4 into main May 11, 2022
@jtcohen6 jtcohen6 deleted the fixup-5223 branch May 11, 2022 14:48
@ueshin
Copy link

ueshin commented May 11, 2022

I confirmed the integration tests pass now. Thanks for the quick fix!

agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants