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

Configure Snowflake Schema via Config #210

Merged

Conversation

podung
Copy link
Contributor

@podung podung commented Nov 10, 2023

NOTE: Original PR and discussion is here: #180. The following description is pasted from that original PR


I'd like the ability to optionally specify the snowflake_schema via config.

Example of Problematic Script

CREATE OR REPLACE FUNCTION {{ db_name }}.{{ schema_name }}.Get_Customer_Name_By_ID(
  ID integer
)
RETURNS varchar
LANGUAGE SQL
as
$$
  SELECT FIRST_NAME || ' ' || LAST_NAME
  FROM CUSTOMER
  WHERE ID = ID
$$;

Schemachange Error

snowflake.connector.errors.ProgrammingError: 090106 (22000): Cannot perform SELECT. This session does not have a current schema. Call 'USE SCHEMA', or use a qualified name.

Explanation

The schemachange docs specify

schemachange is designed to be very lightweight and not impose to many limitations. Each change script can have any number of SQL statements within it and must supply the necessary context, like database and schema names

For most objects, it is sufficient to just declare the db name and schema name in the proc / function / table declaration. However some objects (namely SQL UDTFs in this example) in snowflake seem to ensure the object can compile. Since a schema is not provided on the SELECT ... FROM CUSTOMER statement, and no USE SCHEMA directive has been set, the error occurs.

Work-Arounds

  1. In the UDTF, a fully qualified name can be given (e.g. Select ... FROM {{ db_name }}.{{ schema_name }}.CUSTOMER).

    • This is less-than-desirable for our use case, as it causes the resultant object DDL to include the db/schema in the UDTF body. We have a project requirement to keep object DDLs identical between databases (dev/test/prod/etc).
  2. In the migration, before the CREATE OR REPLACE directive, we can simply add the statement USE SCHEMA {{ schema_name }};

    • This work-around is acceptable, but we have to reach for it in many of our migrations

Why this change is useful

Our team has a requirement to keep the object DDLs identical in each environment (Dev, test, prod, etc). There are a few instances where snowflake requires fully qualified object names to verify compilation during object creation (namely: SQL UDTFs). Instead of having to add the USE SCHEMA {{ schema_name }}; directive in many of our migration files, we'd like to just supply the schema up front and have schemcachange execute the statement for us at the beginning of the run.

PR Tasks

  • Make Snowflake_Schema configuration optional (how it is currently coded it will blow up if not supplied)
  • Ensure snowflake schema can be supplied via CLI as well
  • Modify Documentation
  • Add / modify any tests as necessary

If you are interested in this PR I would be happy to complete the above tasks. Thanks for the library and for the consideration.

sfc-gh-tmathew and others added 4 commits November 6, 2023 15:32
* Update issue templates: Bug Report, Questions and Enhancement templates
Initial workflow for contributing to schemachange repository
…for_contribution

Adding Contribution guidelines
@podung podung marked this pull request as ready for review November 10, 2023 22:29
@sfc-gh-tmathew
Copy link
Collaborator

@podung we will review it and run some tests before merging the PR for the next release. Thank you for your contribution.

@sfc-gh-tmathew sfc-gh-tmathew added enhancement New feature or request Development in Progress Will be addressed in upcoming release labels Nov 15, 2023
Copy link
Collaborator

@sfc-gh-twhite sfc-gh-twhite left a comment

Choose a reason for hiding this comment

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

LGTM, I thought schema was here already!

@podung
Copy link
Contributor Author

podung commented Nov 15, 2023

@podung we will review it and run some tests before merging the PR for the next release. Thank you for your contribution.

Appreciate your time in review and being willing to accept this PR. Thanks for the library.

Copy link
Collaborator

@sfc-gh-jhansen sfc-gh-jhansen left a comment

Choose a reason for hiding this comment

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

Sorry, hit approved too quickly. See comment below, but we're missing the version number change and CHANGELOG entry.

Copy link
Collaborator

@sfc-gh-jhansen sfc-gh-jhansen left a comment

Choose a reason for hiding this comment

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

I haven't tested the changes, but the core changes look good. We do need to update the version number and the CHANGELOG still.

@sfc-gh-tmathew
Copy link
Collaborator

sfc-gh-tmathew commented Nov 15, 2023

@podung Could you please include updates to the following files

  • Setup.cfg: Update the version value to 3.6.1
  • cli.py: Update the version to 3.6.1
  • CHANGELOG: Include the change summary entry for 3.6.1

@podung podung force-pushed the allow_configuring_default_schema branch from 16bb5ee to b0285bf Compare November 15, 2023 20:57
@podung
Copy link
Contributor Author

podung commented Nov 15, 2023

@podung Could you please include updates to the following files

* [Setup.cfg](https://github.com/Snowflake-Labs/schemachange/blob/master/setup.cfg): Update the version value to 3.6.1

* [cli.py](https://github.com/Snowflake-Labs/schemachange/blob/master/schemachange/cli.py): Update the version to 3.6.1

* [CHANGELOG](https://github.com/Snowflake-Labs/schemachange/blob/master/CHANGELOG.md): Include the change summary entry for 3.6.1

Done. Thanks again and let me know if I can do anything else

@sfc-gh-tmathew sfc-gh-tmathew changed the base branch from master to dev November 15, 2023 22:01
Copy link
Collaborator

@sfc-gh-tmathew sfc-gh-tmathew left a comment

Choose a reason for hiding this comment

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

LGTM now. We will run some tests before merging the code into 3.6.1 release

schemachange/cli.py Show resolved Hide resolved
@sfc-gh-tmathew
Copy link
Collaborator

I haven't tested the changes, but the core changes look good. We do need to update the version number and the CHANGELOG still.

We will test before merging but the changes so far look good.

@sfc-gh-tmathew sfc-gh-tmathew merged commit 50aee2f into Snowflake-Labs:dev Nov 15, 2023
1 check passed
Copy link
Collaborator

@sfc-gh-jhansen sfc-gh-jhansen 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, thanks!

@sfc-gh-tmathew sfc-gh-tmathew mentioned this pull request Nov 16, 2023
sfc-gh-tmathew added a commit that referenced this pull request Nov 16, 2023
* Update issue templates (#206)

* Update issue templates: Bug Report, Questions and Enhancement templates

* Create CONTRIBUTING.md

Initial workflow for contributing to schemachange repository

* Add support for specifying the schema via config

* Documentation: Include details on configuring the default schema

* Updated version and changelog

---------

Co-authored-by: podung <joe.depung@gmail.com>
Co-authored-by: Tyler White <90005851+sfc-gh-twhite@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development in Progress Will be addressed in upcoming release enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants