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

New SnowflakeQueryFromFile Task and SQLServerExecute Fix #4363

Merged
merged 33 commits into from Aug 30, 2021

Conversation

kvnkho
Copy link
Contributor

@kvnkho kvnkho commented Apr 6, 2021

Summary

This creates a new task for SnowflakeQueryFromFile. Some users have been looping through .sql files with the ShellTask so this should help them.

The second fix is fixing the MS SQL Server execute Task. This was broken because the library can not take in kwargs. This fix is too hard to test I think.

Checklist

Edit - I edited the SnowflakeQueryFromFile to SnowflakeQueriesFromFile. It now takes in multiple SQL queries and returns a list of results of the queries.

This PR:

  • adds new tests (if appropriate)
  • adds a change file in the changes/ directory (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

@kvnkho kvnkho requested a review from jcrist as a code owner April 6, 2021 20:52
@kvnkho kvnkho changed the title [WIP] SnowflakeQueryFromFile New SnowflakeQueryFromFile Task and SQLServerExecute Fix Apr 6, 2021
@kvnkho
Copy link
Contributor Author

kvnkho commented Apr 12, 2021

This is done.

zanieb
zanieb previously requested changes Apr 26, 2021
Copy link
Contributor

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

What is the SQLServerExecute fix?

src/prefect/tasks/snowflake/snowflake.py Outdated Show resolved Hide resolved
tests/tasks/snowflake/test_snowflake.py Outdated Show resolved Hide resolved
tests/tasks/snowflake/test_snowflake.py Show resolved Hide resolved
@kvnkho
Copy link
Contributor Author

kvnkho commented Apr 26, 2021

What is the SQLServerExecute fix?

cursor.execute(query=query, vars=data) breaks because execute cannot take kwargs.

@kvnkho kvnkho requested a review from zangell44 as a code owner June 25, 2021 18:44
src/prefect/tasks/mysql/mysql.py Outdated Show resolved Hide resolved
src/prefect/tasks/mysql/mysql.py Outdated Show resolved Hide resolved
src/prefect/tasks/snowflake/snowflake.py Outdated Show resolved Hide resolved
src/prefect/tasks/snowflake/snowflake.py Show resolved Hide resolved
kvnkho and others added 3 commits June 27, 2021 10:27
Co-authored-by: Zach Angell <42625717+zangell44@users.noreply.github.com>
@kvnkho
Copy link
Contributor Author

kvnkho commented Jul 7, 2021

I believe this is done now.

zangell44
zangell44 previously approved these changes Jul 7, 2021
Copy link
Collaborator

@zangell44 zangell44 left a comment

Choose a reason for hiding this comment

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

lgtm

@kvnkho
Copy link
Contributor Author

kvnkho commented Aug 13, 2021

I updated this. Couple of changes.

  1. I wrote a test for the SqlServerExecute fix. This test makes sure that kwargs are not passed to the .execute() method because that throws errors (seen in the linked issue)
  2. I updated SnowflakeQueriesFromFile to match the latest updates to SnowflakeQuery from master. Mainly, it takes in a cursor_type that can change the return.
  3. I removed MySQL changes from this PR from last time because there were too many merge conflicts with the latest master. It doesn't belong here anyway.

@kvnkho kvnkho requested review from zanieb and removed request for jcrist August 13, 2021 07:00
@zanieb
Copy link
Contributor

zanieb commented Aug 13, 2021

I'm a bit confused about the docs failure, can you push a commit to retrigger? git commit --allow-empty -m "Force docs rebuild"

Comment on lines 329 to 331
except Exception as error:
conn.close()
raise error

Copy link
Contributor

@zanieb zanieb Aug 27, 2021

Choose a reason for hiding this comment

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

This can be entirely omitted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just making sure, you mean to omit the entire except block right? My concern was that the error won't be raised, but I guess it's passing the test for the error to pass through.

@zanieb zanieb merged commit c720962 into PrefectHQ:master Aug 30, 2021
@zanieb zanieb mentioned this pull request Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snowflake QueryFromFile Task SQL Server SqlServerExecute Task does not work
4 participants