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

quoting config not working with 1.8 #1075

Merged
merged 7 commits into from
Jun 7, 2024
Merged

quoting config not working with 1.8 #1075

merged 7 commits into from
Jun 7, 2024

Conversation

McKnight-42
Copy link
Contributor

@McKnight-42 McKnight-42 commented Jun 7, 2024

resolves #1074
docs dbt-labs/docs.getdbt.com/#

Problem

During decoupling work we updated some naming conventions in some macros that seem to go around quotiing_policy, reverting to put us back in good standing.

Solution

revert to using schema_relation where able in the adapters.sql if schema_relation is a string, otherwise use schema_relation.include(indentifier=False)

  • interactions in test seem to suggest both version are needed see tests/functional/adapter/list_relations_tests/test_pagination.py

update tests in tests/functional/adapter/list_relations_tests/test_pagination.py to use a SnowflakeRelation object instead of a dict

add a new test based of the edgecase from issue.

Todo:

  • Make change in macro
  • Update current tests
  • make new test
  • backport to 1.8.latest after merge

Checklist

  • I have read the contributing guide and understand what's expected of me
  • 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
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@McKnight-42 McKnight-42 self-assigned this Jun 7, 2024
@McKnight-42 McKnight-42 requested a review from a team as a code owner June 7, 2024 15:30
@cla-bot cla-bot bot added the cla:yes label Jun 7, 2024
@mikealfare
Copy link
Contributor

We do this in two spots, so this line would also need updating.

schema_relation = {"database": database, "schema": schema}
kwargs = {"schema_relation": schema_relation}
schema_relation = SnowflakeRelation.create(database=database, schema=schema)
kwargs = {"schema_relation": schema_relation.render()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to render here? Are we testing if the macro can take a relation object or a string (or both I suppose)?

Copy link
Contributor

Choose a reason for hiding this comment

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

you can't pass a proper python object to run-operation AFAIK so this is an easy workaround

@colin-rogers-dbt colin-rogers-dbt merged commit 57285d5 into main Jun 7, 2024
18 checks passed
@colin-rogers-dbt colin-rogers-dbt deleted the mcknight/1074 branch June 7, 2024 21:43
github-actions bot pushed a commit that referenced this pull request Jun 7, 2024
* revert change for quoting policy issue

* make revet in both places

* update format to ignore identifier name

* update test to use SnowflakeRelation and render out the schema_relation, update macros to have condtion to check based on type

* remove unneeded comments from updating test

* add test based on quoting schema names

* update name to more generic my_model

(cherry picked from commit 57285d5)
colin-rogers-dbt pushed a commit that referenced this pull request Jun 10, 2024
* quoting config not working with 1.8  (#1075)

* revert change for quoting policy issue

* make revet in both places

* update format to ignore identifier name

* update test to use SnowflakeRelation and render out the schema_relation, update macros to have condtion to check based on type

* remove unneeded comments from updating test

* add test based on quoting schema names

* update name to more generic my_model

(cherry picked from commit 57285d5)

* local pre-commit run

---------

Co-authored-by: Matthew McKnight <[email protected]>
Co-authored-by: McKnight-42 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] quoting config not working with 1.8
3 participants