Add truncate table (before copy) option to S3ToRedshiftOperator #9246

JavierLopezT · 2020-06-12T09:53:26Z

This PR adds the truncate argument (bool), which makes a truncate right before the copy statement, within the same transaction. Also, the color has been changed to a more vivid one.

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Unit tests coverage for changes (not needed for documentation changes)
Target Github ISSUE in description if exists
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

feluelle

I am not sure all those functionality should be in a transfer operator. It gets really complex. Where do we limit it?

feluelle · 2020-06-24T08:16:16Z

I think the functionality you added is useful, but this can also be done in separate tasks which then makes it also easier to monitor i.e. every step you can check in the UI.

JavierLopezT · 2020-06-24T09:01:16Z

Got it. So I will remove from the PR the functionalities of deleting s3 file, duplicates checking and executing previous queries, and leave the other things. Is that okay?

feluelle · 2020-06-24T11:02:16Z

Got it. So I will remove from the PR the functionalities of deleting s3 file, duplicates checking and executing previous queries, and leave the other things. Is that okay?

By different tasks I mean different Airflow tasks - not different PR's.

feluelle · 2020-06-24T11:08:51Z

I think also the "truncate" functionality can be done by a different PostgresOperator task. WDYT?

JavierLopezT · 2020-06-25T09:42:16Z

While I agree that the rest of functionalities can be done in separates operators (for instance, you may want to delete several s3 files at the end of your dag, or you don't want to check duplicates, or you may execute previous queries in a single operator at the beginning of the dag), I think the "truncate" is more "fittable" to be in this operator, because it is way simpler than the others (just one line) and it is just kind of preparation for the copy, not an extra step. Also, I believe the log/errors that can be thrown because of the truncate are simple enough to be monitored in the same task of the COPY. Anyway, if you think I should also remove that from the PR, I'll do it. Thanks!

feluelle · 2020-06-25T09:48:03Z

Good explanation. I can see the "truncate" functionality stay in there. 👍

JeffryMAC · 2020-07-27T15:42:38Z

I think also the "truncate" functionality can be done by a different PostgresOperator task. WDYT?

Other than what @JavierLopezT mentioned if you separated Truncate & Copy you might end up with empty table.
You want the Truncate & Copy be executed one after another.

While this doesn't guaranty atomecy it's the closest thing you can have.

feluelle · 2020-07-27T15:46:38Z

Okay, but then we should at least use SQL transactions.

feluelle · 2020-07-30T08:39:35Z

@JavierLopezT WDYT? Can you use transactions when doing truncate + copy? :)

JavierLopezT · 2020-07-31T08:59:22Z

@feluelle Sure, I'll come back to this PR as soon as I can

JavierLopezT · 2020-08-06T11:14:10Z

Hello @feluelle. Do you think I can take the changes proposed in #7688 and put it in there or should I open a new MR?

feluelle · 2020-08-06T11:19:08Z

I would add it to a new PR.

Note that the PR you linked is proposing a change to RedshiftToS3 not S3ToRedshift like you do here.

JavierLopezT · 2020-09-02T11:02:19Z

Hello @feluelle, I have done the changes, and now comes the drama: TESTING

I think I have to add a test to check that the truncate works.
Is it enough to add a check that tests if the truncate_statement returns the correct string? Do I have to modify the already existing test to include the transaction part?

Thank you very much

airflow/providers/amazon/aws/transfers/s3_to_redshift.py

feluelle

@JavierLopezT can we rename transaction variable to someting more appropriate like sql or so?

feluelle · 2020-09-23T07:13:56Z

Javier I also removed the following from the description:

Modified a part in the copy statement, that will makes prior workflows incompatible. Not sure if I have to propose a discussion about this point somewhere else (If so, I will delete this part from this PR) or just create a note in UPDATING.md. The prior code was:
COPY {self.schema}.{self.table}
            FROM 's3://{self.s3_bucket}/{self.s3_key}/{self.table}'
            with credentials
            'aws_access_key_id={credentials.access_key};aws_secret_access_key={credentials.secret_key}'
            {copy_options};
And the new is:
COPY {self.schema}.{self.table}
            FROM 's3://{self.s3_bucket}/{self.s3_key}'
            with credentials
            'aws_access_key_id={credentials.access_key};aws_secret_access_key={credentials.secret_key}'
            {copy_options};
So we don't have to have the table as the file name. I have read here and somewhere else complains about the existence of the table at the end of the from.

..as this isn't in your changes / PR ?!

JavierLopezT · 2020-09-23T07:57:31Z

Javier I also removed the following from the description:
Modified a part in the copy statement, that will makes prior workflows incompatible. Not sure if I have to propose a discussion about this point somewhere else (If so, I will delete this part from this PR) or just create a note in UPDATING.md. The prior code was:
COPY {self.schema}.{self.table}
            FROM 's3://{self.s3_bucket}/{self.s3_key}/{self.table}'
            with credentials
            'aws_access_key_id={credentials.access_key};aws_secret_access_key={credentials.secret_key}'
            {copy_options};
And the new is:
COPY {self.schema}.{self.table}
            FROM 's3://{self.s3_bucket}/{self.s3_key}'
            with credentials
            'aws_access_key_id={credentials.access_key};aws_secret_access_key={credentials.secret_key}'
            {copy_options};
So we don't have to have the table as the file name. I have read here and somewhere else complains about the existence of the table at the end of the from.
..as this isn't in your changes / PR ?!

Oh yes, I have just seen that this was already solved in August in c635804#diff-85c770282ec360127508910595a47fd1
Thanks!

JavierLopezT · 2020-09-23T09:48:50Z

The template_field is also added in #10890. I'll edit my description again

JavierLopezT · 2020-10-07T15:20:18Z

@JavierLopezT can we rename transaction variable to someting more appropriate like sql or so?

Done

tests/providers/amazon/aws/transfers/test_s3_to_redshift.py

feluelle · 2020-10-08T15:08:28Z

LGTM @JavierLopezT. Let's wait until the issue on master is fixed and then I am merging this one.

feluelle · 2020-10-09T07:48:22Z

@JavierLopezT There are so many failing checks, can you rebase once more and if this is fine I am merging it. :)

JavierLopezT · 2020-10-21T13:41:15Z

@JavierLopezT There are so many failing checks, can you rebase once more and if this is fine I am merging it. :)

Done. Not sure if I have done it correctly though. (I followed https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#id9)

airflow/providers/amazon/aws/transfers/s3_to_redshift.py

feluelle · 2020-10-22T07:57:42Z

@JavierLopezT test seems to fail. Can you look at it, please? :)

airflow/providers/amazon/aws/transfers/s3_to_redshift.py

turbaszek

LGTM 👌 @feluelle ?

feluelle

Yes, merging it.

…he#9246) - add table arg to jinja template fields - change ui_color Co-authored-by: javier.lopez <javier.lopez@promocionesfarma.com>

boring-cyborg bot added the provider:amazon AWS/Amazon - related issues label Jun 12, 2020

feluelle self-requested a review June 14, 2020 19:03

feluelle reviewed Jun 24, 2020

View reviewed changes

JavierLTPromofarma force-pushed the s3toredshift branch from f0d2642 to 805781b Compare September 2, 2020 09:33

feluelle reviewed Sep 3, 2020

View reviewed changes

airflow/providers/amazon/aws/transfers/s3_to_redshift.py Outdated Show resolved Hide resolved

JeffryMAC mentioned this pull request Sep 12, 2020

Should we stay flexible to copy files from s3 to Redshift when using S3ToRedshiftTransfer? #10895

Closed

turbaszek reviewed Sep 13, 2020

View reviewed changes

airflow/providers/amazon/aws/transfers/s3_to_redshift.py Outdated Show resolved Hide resolved

feluelle reviewed Sep 23, 2020

View reviewed changes

feluelle changed the title ~~Few new functionalities and changes in S3ToRedshiftOperator~~ Add truncate table (before copy) option to S3ToRedshiftOperator Sep 23, 2020

turbaszek requested a review from feluelle September 27, 2020 17:08

turbaszek approved these changes Sep 27, 2020

View reviewed changes

feluelle reviewed Oct 8, 2020

View reviewed changes

tests/providers/amazon/aws/transfers/test_s3_to_redshift.py Outdated Show resolved Hide resolved

feluelle approved these changes Oct 8, 2020

View reviewed changes

JavierLTPromofarma added 8 commits October 21, 2020 15:35

using transactions

37cbdca

pylint

b3f47c0

changes

d51985e

pylint

7069d99

static check

9570b16

mocks

1d1ea0c

rename transaction to sql

30df620

f-strings test

0a90b24

JavierLTPromofarma force-pushed the s3toredshift branch from a97c580 to 0a90b24 Compare October 21, 2020 13:35

potix2 reviewed Oct 21, 2020

View reviewed changes

airflow/providers/amazon/aws/transfers/s3_to_redshift.py Outdated Show resolved Hide resolved

potix2 reviewed Oct 21, 2020

View reviewed changes

airflow/providers/amazon/aws/transfers/s3_to_redshift.py Outdated Show resolved Hide resolved

fix test

aa13da9

feluelle reviewed Oct 23, 2020

View reviewed changes

airflow/providers/amazon/aws/transfers/s3_to_redshift.py Outdated Show resolved Hide resolved

airflow/providers/amazon/aws/transfers/s3_to_redshift.py Outdated Show resolved Hide resolved

potix2 suggestions

4c8f221

turbaszek approved these changes Oct 28, 2020

View reviewed changes

feluelle approved these changes Oct 28, 2020

View reviewed changes

feluelle merged commit db121f7 into apache:master Oct 28, 2020

JavierLopezT deleted the s3toredshift branch January 20, 2021 19:21

Add truncate table (before copy) option to S3ToRedshiftOperator #9246

Add truncate table (before copy) option to S3ToRedshiftOperator #9246

Conversation

JavierLopezT commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feluelle left a comment

Choose a reason for hiding this comment

Uh oh!

feluelle commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JavierLopezT commented Jun 24, 2020

Uh oh!

feluelle commented Jun 24, 2020

Uh oh!

feluelle commented Jun 24, 2020

Uh oh!

JavierLopezT commented Jun 25, 2020

Uh oh!

feluelle commented Jun 25, 2020

Uh oh!

JeffryMAC commented Jul 27, 2020

Uh oh!

feluelle commented Jul 27, 2020

Uh oh!

feluelle commented Jul 30, 2020

Uh oh!

JavierLopezT commented Jul 31, 2020

Uh oh!

JavierLopezT commented Aug 6, 2020

Uh oh!

feluelle commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JavierLopezT commented Sep 2, 2020

Uh oh!

Uh oh!

Uh oh!

feluelle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feluelle commented Sep 23, 2020

Uh oh!

JavierLopezT commented Sep 23, 2020

Uh oh!

JavierLopezT commented Sep 23, 2020

Uh oh!

JavierLopezT commented Oct 7, 2020

Uh oh!

Uh oh!

feluelle commented Oct 8, 2020

Uh oh!

feluelle commented Oct 9, 2020

Uh oh!

JavierLopezT commented Oct 21, 2020

Uh oh!

Uh oh!

Uh oh!

feluelle commented Oct 22, 2020

Uh oh!

Uh oh!

Uh oh!

turbaszek left a comment

Choose a reason for hiding this comment

Uh oh!

feluelle left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JavierLopezT commented Jun 12, 2020 •

edited

Loading

feluelle commented Jun 24, 2020 •

edited

Loading

feluelle commented Aug 6, 2020 •

edited

Loading

feluelle left a comment •

edited

Loading