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

More consistent results from get_columns_in_relation #355

Merged
merged 4 commits into from
May 16, 2022

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented May 11, 2022

resolves #354

Description

Context in the issue. Changes here:

  • Rename get_columns_in_relation macro to get_columns_in_relation_raw
  • New get_columns_in_relation macro that just returns adapter.get_columns_in_relation

This should also fix CI, parallel to the change in dbt-labs/dbt-core#5232 (being tested in #353)

Checklist

  • I have signed the CLA
  • 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 and added information about my change to the "dbt-spark next" section.

@cla-bot cla-bot bot added the cla:yes label May 11, 2022
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.

This looks good to me for:

  • establishing full parity between the get_columns_in_relation macro and adapter.get_columns_in_relation.

My understanding is that once this PR is merged, then dbt-labs/dbt-core#5232 can be merged.

@jtcohen6
Copy link
Contributor Author

@dbeatty10 Thanks for the quick review!

Both of these PRs solve the issue independently. We could say that this PR renders dbt-labs/dbt-core#5232 unnecessary, but I'd still prefer the consistency there, too, around how we call this method in our internal macros.

@jtcohen6 jtcohen6 marked this pull request as ready for review May 16, 2022 11:42
@jtcohen6 jtcohen6 merged commit fad79e3 into main May 16, 2022
@jtcohen6 jtcohen6 deleted the fix/get-columns-in-relation-consistency branch May 16, 2022 12:37
francescomucio pushed a commit to francescomucio/dbt-spark that referenced this pull request May 16, 2022
* More consistent results from get_columns_in_relation

* Not dispatched, full name

* Add changelog entry
francescomucio pushed a commit to francescomucio/dbt-spark that referenced this pull request May 16, 2022
* More consistent results from get_columns_in_relation

* Not dispatched, full name

* Add changelog entry
@@ -168,11 +168,15 @@
{%- endcall -%}
{% endmacro %}

{% macro spark__get_columns_in_relation(relation) -%}
{% call statement('get_columns_in_relation', fetch_result=True) %}
{% macro spark__get_columns_in_relation_raw(relation) -%}
Copy link
Contributor

@ueshin ueshin May 22, 2022

Choose a reason for hiding this comment

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

@jtcohen6 Is it possible to make it dispatch pattern? We might want to override this. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I submitted a PR #365, could you take a look at it? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @ueshin -- utilizing the dispatch pattern makes good sense. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-628] Inconsistency between get_columns_in_relation method + macro
3 participants