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

Implement Async Snowflake SQL API Operator #478

Merged
merged 1 commit into from
Jul 12, 2022
Merged

Implement Async Snowflake SQL API Operator #478

merged 1 commit into from
Jul 12, 2022

Conversation

bharanidharan14
Copy link
Contributor

@bharanidharan14 bharanidharan14 commented Jun 28, 2022 •
edited
Loading

Implemented Async Snowflake SQL API Operator to support multiple SQL statements sequentially, which is the behavior of the SnowflakeOperator, the Snowflake SQL API allows for submitting multiple SQL statements in a single request. In combination with aiohttp, this may be an option for creating a SnowflakeSQLOperatorAsync that matches the query submission behavior of the SnowflakeOperator.

  • Added Snowflake Async SQL API operator, hooks, trigger
  • Added Example DAG
  • Added Doc string
  • Exception Handling in case of errors
  • Added Test case for Hook, Triggers, Operator
  • Added Example Documentation

Screenshot 2022-07-05 at 12 26 17 AM

Screenshot 2022-07-07 at 10 05 42 AM

Documentation
https://www.notion.so/astronomerio/Snowflake-Async-Operators-2a62522b1f534a02bde099b104216c41

closes: #477

@codecov
Copy link

codecov bot commented Jun 28, 2022 •
edited
Loading

Codecov Report

Merging #478 (cc67d41) into main (cc67d41) will not change coverage.
The diff coverage is n/a.

❗ Current head cc67d41 differs from pull request most recent head f30cc9e. Consider uploading reports for the commit f30cc9e to get more accurate results

@@           Coverage Diff           @@
##             main     #478   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files          70       70           
  Lines        3744     3744           
=======================================
  Hits         3677     3677           
  Misses         67       67

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc67d41...f30cc9e. Read the comment docs.

@bharanidharan14 bharanidharan14 force-pushed the snowflake-sql-api branch 3 times, most recently from 1a84b0d to a06fc94 Compare June 30, 2022 21:53
@bharanidharan14 bharanidharan14 marked this pull request as ready for review June 30, 2022 23:04
@bharanidharan14 bharanidharan14 force-pushed the snowflake-sql-api branch 5 times, most recently from 3fbb09a to 4565244 Compare July 1, 2022 09:48
phanikumv
phanikumv requested changes Jul 1, 2022
View reviewed changes
Copy link
Collaborator

@phanikumv phanikumv left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • On first glance, the conventions of the class names should change to use Camel case.(SQL should change to Sql, API to Api)

    SnowflakeSQLOperatorAsync
    SnowflakeSQLAPITrigger
    SnowflakeSQLAPIHookAsync

  • What about naming this operator as SnowflakeOperatorAsync and renaming the earlier one as SnowflakeParallel* , as this Operator is better in terms of submitting sequential multiple queries?

@bharanidharan14
Copy link
Contributor Author

  • On first glance, the conventions of the class names should change to use Camel case.(SQL should change to Sql, API to Api)
    SnowflakeSQLOperatorAsync
    SnowflakeSQLAPITrigger
    SnowflakeSQLAPIHookAsync
  • What about naming this operator as SnowflakeOperatorAsync and renaming the earlier one as SnowflakeParallel* , as this Operator is better in terms of submitting sequential multiple queries?

Yes I guess we should rename this operator as SnowflakeOperatorAsync and need to change the old one as SnowflakeParallelOperator

@phanikumv
Copy link
Collaborator

  • On first glance, the conventions of the class names should change to use Camel case.(SQL should change to Sql, API to Api)
    SnowflakeSQLOperatorAsync
    SnowflakeSQLAPITrigger
    SnowflakeSQLAPIHookAsync
  • What about naming this operator as SnowflakeOperatorAsync and renaming the earlier one as SnowflakeParallel* , as this Operator is better in terms of submitting sequential multiple queries?

Yes I guess we should rename this operator as SnowflakeOperatorAsync and need to change the old one as SnowflakeParallelOperator

Lets do this change so that there wont be DAG facing changes for the user. I also think that we can deprecate the old one, and no need of parallel operator as this operator already contains all the functionality needed

@pankajastro
Copy link
Contributor

@bharanidharan14 Please consider documenting example DAG too.

@bharanidharan14
Copy link
Contributor Author

example DAG

Added Documentation for the example DAG

@bharanidharan14 bharanidharan14 force-pushed the snowflake-sql-api branch 3 times, most recently from 93d8334 to a207bab Compare July 4, 2022 18:13
@bharanidharan14 bharanidharan14 force-pushed the snowflake-sql-api branch 2 times, most recently from 5d4adb7 to 303265e Compare July 4, 2022 18:58
@bharanidharan14
Copy link
Contributor Author

@pankajastro Addressed your review comments please take a look at it

@bharanidharan14 @rajaths010494
Implement Async Snowflake SQL API Operator
f30cc9e 
Implement Async Snowflake SQL API Operator

Implement Async Snowflake SQL API Operator

Implement Async Snowflake SQL API Operator

Covered few more details about the operator and snowflake SQL API in doc string

Implement Async Snowflake SQL API Operator

Implement Async Snowflake SQL API Operator

Implement Async Snowflake SQL API Operator

Implement Async Snowflake SQL API Operator

Implement Async Snowflake SQL API Operator

Implement Async Snowflake SQL API Operator

Implemented Async Snowflake SQL API Operator to support multiple  SQL statements sequentially, which is the behavior of the SnowflakeOperator,  the Snowflake SQL API allows for submitting multiple SQL statements in a single request. In combination with aiohttp, this may be an option for creating a SnowflakeSQLOperatorAsync that matches the query submission behavior of the SnowflakeOperator.

Test case

Add Test case

Added Test case for Snowflake SQL API Trigger and Operator

Test case fix

Test case fix

Skip code coverage for import

Doc fix

Add example DAG Documenting

-  Added Example DAG Documentation
- Changed the class name to camel case

Docs FIx

Fix doc

Update Doc

Fix doc string

Move SnowflakeSqlApiOperatorAsync to snowflake.py file

Import fix

Co-Authored-By: Rajath <92459020+rajaths010494@users.noreply.github.com>
@bharanidharan14 bharanidharan14 mentioned this pull request Jul 11, 2022
Closed
5 tasks
@bharanidharan14 bharanidharan14 requested review from rajaths010494 and removed request for rajaths010494 and pankajastro July 12, 2022 05:22
@bharanidharan14
Copy link
Contributor Author

@pankajastro @phanikumv @pankajkoti @rajaths010494 Please review this PR

phanikumv
phanikumv approved these changes Jul 12, 2022
View reviewed changes
Copy link
Collaborator

@phanikumv phanikumv left a comment

Choose a reason for hiding this comment

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

LGTM

@bharanidharan14 bharanidharan14 merged commit 991cae7 into main Jul 12, 2022
@bharanidharan14 bharanidharan14 deleted the snowflake-sql-api branch July 12, 2022 15:42
@tatiana tatiana mentioned this pull request Jul 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phanikumv phanikumv phanikumv approved these changes

@rajaths010494 rajaths010494 rajaths010494 approved these changes

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

@pankajastro pankajastro Awaiting requested review from pankajastro pankajastro 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.

Implement Async Snowflake SQL API Operator
4 participants
@bharanidharan14 @phanikumv @pankajastro @rajaths010494