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.

Sign up for GitHub

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

Jump to bottom

fix: update connection string in Snowflake extractor to include wareh… #357

Merged
merged 5 commits into from
Sep 10, 2020
Merged

fix: update connection string in Snowflake extractor to include wareh… #357

merged 5 commits into from
Sep 10, 2020

Conversation

Alagappan
Copy link
Contributor

…ouse as param

Signed-off-by: Alagappan Sethuraman [email protected]

Summary of Changes

Updated Snowflake sample data loader script and DAG to include warehouse in the connection string. Without the warehouse param, snowflake throws an error asking to select a warehouse via USE [WAREHOUSE_NAME] command before executing extraction query.

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
  • PR passes make test

@Alagappan
Copy link
Contributor Author

cc @feng-tao or @jornh

@Alagappan
remove unused library
1011fca 
Signed-off-by: Alagappan Sethuraman <[email protected]>
@Alagappan
lint fixes
16cbfd7 
Signed-off-by: Alagappan Sethuraman <[email protected]>
jornh
jornh reviewed Sep 3, 2020
View reviewed changes
example/dags/snowflake_sample_dag.py Outdated
database=SNOWFLAKE_DATABASE,
warehouse=warehouse,
)
return "snowflake://%s:%s@%s" % (user, password, account, warehouse)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return "snowflake://%s:%s@%s" % (user, password, account, warehouse)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess one return statement will do!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. updated it. 👍

example/scripts/sample_snowflake_data_loader.py Outdated
database=SNOWFLAKE_DATABASE,
warehouse=warehouse,
)
return "snowflake://%s:%s@%s" % (user, password, account, warehouse)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return "snowflake://%s:%s@%s" % (user, password, account, warehouse)

Same here

@Alagappan
Remove extra return statements
28bfe9b 
Signed-off-by: Alagappan Sethuraman <[email protected]>
@Alagappan
Copy link
Contributor Author

ping. can we merge this one, please?

feng-tao
feng-tao reviewed Sep 4, 2020
View reviewed changes
example/dags/snowflake_sample_dag.py Outdated
@@ -69,15 +69,23 @@
SUPPORTED_SCHEMA_SQL_IN_CLAUSE = "('{schemas}')".format(schemas="', '".join(SUPPORTED_SCHEMAS))

# SNOWFLAKE CONFIGs
SNOWFLAKE_DATABASE_KEY = 'YOUR_SNOWFLAKE_DB_NAME'
SNOWFLAKE_DATABASE = 'YOUR_SNOWFLAKE_DB_NAME'
Copy link
Member

Choose a reason for hiding this comment

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

why we make this change when it still uses

amundsendatabuilder/databuilder/extractor/snowflake_metadata_extractor.py

Line 60 in 804b20c

SNOWFLAKE_DATABASE_KEY = 'snowflake_database'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I think we went back and forth on the other PR. Let me just use what is in snowflake_metadata_extractor.py to keep them consistent for now.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good, yeap, let's keep it consistent.

feng-tao
feng-tao reviewed Sep 4, 2020
View reviewed changes
example/dags/snowflake_sample_dag.py
return "snowflake://%s:%s@%s" % (user, password, account)
# specify a warehouse to connect to.
warehouse = 'yourwarehouse'
return 'snowflake://{user}:{password}@{account}/{database}?warehouse={warehouse}'.format(
Copy link
Member

@feng-tao feng-tao Sep 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am not familiar with snowflake connection string format, you need to put the doc on how it defines the conn string as comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I will add some comments and make it clear. Will update both the files.

feng-tao
feng-tao reviewed Sep 4, 2020
View reviewed changes
example/scripts/sample_snowflake_data_loader.py Outdated
@@ -31,7 +30,7 @@
# Disable snowflake logging
logging.getLogger("snowflake.connector.network").disabled = True

SNOWFLAKE_DATABASE_KEY = 'YourSnowflakeDbName'
SNOWFLAKE_DATABASE = 'YourSnowflakeDbName'
Copy link
Member

Choose a reason for hiding this comment

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

same above

feng-tao
feng-tao reviewed Sep 4, 2020
View reviewed changes
Copy link
Member

@feng-tao feng-tao left a comment

Choose a reason for hiding this comment

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

and has this been tested?

@feng-tao
Copy link
Member

feng-tao commented Sep 4, 2020

Also Alagappan, I will be out until next Tue, so my response will be slow in the mean time. hope it is ok.

@Alagappan
Copy link
Contributor Author

Also Alagappan, I will be out until next Tue, so my response will be slow in the mean time. hope it is ok.

No problem. thanks for the heads up!

@Alagappan
Copy link
Contributor Author

and has this been tested?
Yes, I tested the sample scripts and found this issue. and pushing this PR as a fix

@feng-tao
Copy link
Member

feng-tao commented Sep 9, 2020

let me know if you update the pr based on the discussion.

@Alagappan
PR comments
16b6931 
Signed-off-by: Alagappan <[email protected]>
@Alagappan
Copy link
Contributor Author

@feng-tao updated PR based on comments. Let me know if this is good. thanks!

@feng-tao feng-tao merged commit a11d206 into amundsen-io:master Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jornh jornh jornh left review comments

@feng-tao feng-tao feng-tao approved these changes

@allisonsuarez allisonsuarez Awaiting requested review from allisonsuarez allisonsuarez is a code owner

@dikshathakur3119 dikshathakur3119 Awaiting requested review from dikshathakur3119 dikshathakur3119 is a code owner

@jinhyukchang jinhyukchang Awaiting requested review from jinhyukchang jinhyukchang is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Alagappan @feng-tao @jornh