-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Support session reuse in RedshiftDataOperator #42218
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
Conversation
vincbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from your new feature, you seem to fix many bugs, thanks for doing it!
vincbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting "request changes" so that it is not merged out of the blue. Since this PR contains a breaking change, that means the next release of the amazon provider package will be a major one. We (AWS team) want to take this opportunity to remove deprecated stuff. This is just a way to control when this PR will be merged
|
@borismo Just checking in, are you working on this one? |
|
@vincbeck , yes, but on my free time. I'll try to make some progress soon. |
|
The |
Interesting, I'd like to see the error from compat tests. Could you solve the non-db tests please? |
Sure, done in 87da7b9 |
|
That worked :) Merging it now |
|
@eladkal This is a breaking change in Amazon provider package. So you know, the next release should be a major version bump. AWS team will take advantage of this to remove deprecated stuff from Amazon provider package |
Recently, AWS added session reuse to the Redshift Data API. It allows to, say, create a temporary table in one statement, and select from it in a subsequent one.
I think it would be useful that
RedshiftDataOperatorsupports this new feature.Decisions
databasearguments now optional because when the session ID is provided, boto3 doesn't allow database to be specified. Because this becomes a named argument, I had to move it aftersql. If users of the operator and hook rely on the position to pass the argument that could break thingsdataclassinstead of a dict to make it harder to make a mistake when indexing the keys. If users use the hook in their tasks or custom operators, this could also break thingsparse_statement_resposnemethod. It's a public one. So, again, there is a risk of breaking change.To do
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.