[AIRFLOW-5726] Delete table as file name in RedshiftToS3Transfer #6396

JavierLopezT · 2019-10-23T14:06:00Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-5726
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
- In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

Now it is possible to have whatever file name for the file that will be stored in S3 and there is no limitation for table name. There is still the possibility to keep this choice though.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

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
- If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

JavierLopezT · 2019-10-24T08:25:07Z

I have a problem here with a part of the documentation, the one defining the param s3_key.

If I leave it's definition as a single line ':param s3_key: reference to a specific S3 key. If table_as_file_name is set to False, this param must include the desired file name',
it doesn't pass the pylint check due to line length.

If I split it in two lines as:
1st line: :param s3_key: reference to a specific S3 key. If table_as_file_name is set
2nd line: to False, this param must include the desired file name
It doesn't pass the documentation test.

Any idea on how to handle this? Thank you very much

OmerJog · 2019-10-27T13:18:25Z

@JavierLopezT I'm not really sure I'm following.
The error is:
airflow/operators/redshift_to_s3_operator.py:40:0: C0301: Line too long (135/110) (line-too-long)

You can have more than one row example:
https://github.com/apache/airflow/blob/master/airflow/gcp/operators/bigquery.py#L72

codecov-io · 2019-10-28T10:03:23Z

Codecov Report

Merging #6396 into master will increase coverage by 2.91%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6396      +/-   ##
==========================================
+ Coverage   80.59%   83.51%   +2.91%     
==========================================
  Files         626      672      +46     
  Lines       36243    37594    +1351     
==========================================
+ Hits        29211    31396    +2185     
+ Misses       7032     6198     -834

Impacted Files	Coverage Δ
airflow/operators/redshift_to_s3_operator.py	`97.29% <100%> (+0.15%)`	⬆️
airflow/contrib/sensors/cassandra_record_sensor.py	`0% <0%> (-100%)`	⬇️
airflow/contrib/hooks/aws_firehose_hook.py	`0% <0%> (-100%)`	⬇️
airflow/contrib/sensors/cassandra_table_sensor.py	`0% <0%> (-100%)`	⬇️
...low/contrib/sensors/aws_redshift_cluster_sensor.py	`0% <0%> (-100%)`	⬇️
airflow/contrib/hooks/redshift_hook.py	`0% <0%> (-75%)`	⬇️
airflow/kubernetes/volume_mount.py	`44.44% <0%> (-55.56%)`	⬇️
airflow/kubernetes/volume.py	`52.94% <0%> (-47.06%)`	⬇️
airflow/kubernetes/pod_launcher.py	`45.25% <0%> (-46.72%)`	⬇️
airflow/example_dags/example_python_operator.py	`63.33% <0%> (-31.12%)`	⬇️
... and 243 more

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 74d2a0d...297f06f. Read the comment docs.

Solving errors More errors Trying to pass documentation test indendation comment Delete table as file name in RedshiftToS3Transfer

JavierLopezT · 2019-10-28T10:36:20Z

@JavierLopezT I'm not really sure I'm following.
The error is:
airflow/operators/redshift_to_s3_operator.py:40:0: C0301: Line too long (135/110) (line-too-long)

You can have more than one row example:
https://github.com/apache/airflow/blob/master/airflow/gcp/operators/bigquery.py#L72

I was missing the indentation on the 2nd row, thanks. Everything seems fine now

kaxil · 2019-10-28T22:16:19Z

It looks good but can you please add tests or update the current tests !

JavierLopezT · 2019-10-30T09:54:22Z

It looks good but can you please add tests or update the current tests !

How can I do that? I thought that passing the TRAVIS CI checks was enough and I don't really find any documentation in Airflow contributing guiding through the process to add tests. Could you help me please?

kaxil · 2019-10-30T22:03:16Z

We have got this in the PR guidelines that you ticked :D

kaxil · 2019-10-30T22:04:26Z

add one here: https://github.com/apache/airflow/blob/master/tests/operators/test_redshift_to_s3_operator.py

JavierLopezT · 2019-11-12T16:55:08Z

Thanks for the clarification @kaxil

I have already added the test (I think)

kaxil

LGTM. But would like other committers that have AWS experience to review it too.

@potiuk @ashb any comments?

airflow/operators/redshift_to_s3_operator.py

Co-Authored-By: Ash Berlin-Taylor <[email protected]>

ashb

Also you need to add a test covering table_as_file_name=False case.

tests/operators/test_redshift_to_s3_operator.py

OmerJog · 2019-11-15T15:12:56Z

@JavierLopezT
The added test is failing:

======================================================================
62) ERROR: test_execute_false (tests.operators.test_redshift_to_s3_operator.TestRedshiftToS3Transfer)
----------------------------------------------------------------------
   Traceback (most recent call last):
   TypeError: test_execute_false() missing 2 required positional arguments: 'mock_run' and 'mock_session'

kaxil · 2019-11-15T15:36:27Z

tests/operators/test_redshift_to_s3_operator.py

+        assert mock_run.call_count == 1
+        assertEqualIgnoreMultipleSpaces(self, mock_run.call_args[0][0], unload_query)
+
+    def test_execute_false(self, mock_run, mock_session):


You can use parameterized in def test_execute instead of creating new test

Example:

@parameterized([ (2, 2, 4), (2, 3, 8), (1, 9, 1), (0, 9, 0), ]) def test_pow(base, exponent, expected): assert_equal(math.pow(base, exponent), expected)

kaxil · 2019-11-20T15:36:26Z

tests/operators/test_redshift_to_s3_operator.py

@@ -31,7 +32,8 @@ class TestRedshiftToS3Transfer(unittest.TestCase):

    @mock.patch("boto3.session.Session")
    @mock.patch("airflow.hooks.postgres_hook.PostgresHook.run")
-    def test_execute(self, mock_run, mock_session):
+    @parameterized([(True), (False)])


Suggested change

@parameterized([(True), (False)])

@parameterized([(True, ), (False, )])

kaxil · 2019-11-20T17:20:32Z

tests/operators/test_redshift_to_s3_operator.py

@@ -31,7 +32,8 @@ class TestRedshiftToS3Transfer(unittest.TestCase):

    @mock.patch("boto3.session.Session")
    @mock.patch("airflow.hooks.postgres_hook.PostgresHook.run")
-    def test_execute(self, mock_run, mock_session):
+    @parameterized([(True, ), (False, )])


Suggested change

@parameterized([(True, ), (False, )])

@parameterized.expand([(True, ), (False, )])

This should hopefully fix the failing test

Unfortunately, it is still failing =(

kaxil · 2019-11-21T15:44:48Z

tests/operators/test_redshift_to_s3_operator.py

@@ -31,7 +32,8 @@ class TestRedshiftToS3Transfer(unittest.TestCase):

    @mock.patch("boto3.session.Session")
    @mock.patch("airflow.hooks.postgres_hook.PostgresHook.run")
-    def test_execute(self, mock_run, mock_session):
+    @parameterized.expand([(True, ), (False, )])
+    def test_execute(self, mock_run, mock_session, boolean_value):


I am going to try and run this my test on my machine and let you know

kaxil · 2019-11-21T16:44:37Z

@JavierLopezT Apply the following code and the test will pass. Passes on my local machine:

diff --git a/tests/operators/test_redshift_to_s3_operator.py b/tests/operators/test_redshift_to_s3_operator.py
index 5fd8d46e3..baa4aad32 100644
--- a/tests/operators/test_redshift_to_s3_operator.py
+++ b/tests/operators/test_redshift_to_s3_operator.py
@@ -30,10 +30,13 @@ from airflow.utils.tests import assertEqualIgnoreMultipleSpaces

 class TestRedshiftToS3Transfer(unittest.TestCase):

+    @parameterized.expand([
+        [True, "key/table_"],
+        [False, "key"],
+    ])
     @mock.patch("boto3.session.Session")
     @mock.patch("airflow.hooks.postgres_hook.PostgresHook.run")
-    @parameterized.expand([(True, ), (False, )])
-    def test_execute(self, mock_run, mock_session, boolean_value):
+    def test_execute(self, table_as_file_name, expected_s3_key, mock_run, mock_session,):
         access_key = "aws_access_key_id"
         secret_key = "aws_secret_access_key"
         mock_session.return_value = Session(access_key, secret_key)
@@ -42,7 +45,6 @@ class TestRedshiftToS3Transfer(unittest.TestCase):
         s3_bucket = "bucket"
         s3_key = "key"
         unload_options = ['HEADER', ]
-        table_as_file_name = boolean_value

         RedshiftToS3Transfer(
             schema=schema,
@@ -62,14 +64,14 @@ class TestRedshiftToS3Transfer(unittest.TestCase):
         select_query = "SELECT * FROM {schema}.{table}".format(schema=schema, table=table)
         unload_query = """
                     UNLOAD ('{select_query}')
-                    TO 's3://{s3_bucket}/{s3_key}/{table}_'
+                    TO 's3://{s3_bucket}/{s3_key}'
                     with credentials
                     'aws_access_key_id={access_key};aws_secret_access_key={secret_key}'
                     {unload_options};
                     """.format(select_query=select_query,
-                               table=table,
                                s3_bucket=s3_bucket,
-                               s3_key=s3_key,
+                               s3_key=expected_s3_key,
                                access_key=access_key,
                                secret_key=secret_key,
                                unload_options=unload_options)

kaxil · 2019-11-25T21:05:49Z

tests/operators/test_redshift_to_s3_operator.py

@@ -31,7 +32,11 @@ class TestRedshiftToS3Transfer(unittest.TestCase):

    @mock.patch("boto3.session.Session")
    @mock.patch("airflow.hooks.postgres_hook.PostgresHook.run")
-    def test_execute(self, mock_run, mock_session):
+    @parameterized.expand([


You have applied this to the wrong place, please check the diff #6396 (comment) I suggested 4 days back

It should be the following:

@parameterized.expand([ [True, "key/table_"], [False, "key"], ]) @mock.patch("boto3.session.Session") @mock.patch("airflow.hooks.postgres_hook.PostgresHook.run") def test_execute(self, table_as_file_name, expected_s3_key, mock_run, mock_session,):

Check the position of @parameterized.expand([

(cherry-picked from 4a17bca)

JavierLTAtr added 4 commits October 23, 2019 15:56

Delete table as file name in RedshiftToS3Transfer

b0e19e0

Solving errors

5288964

More errors

5d5234a

Trying to pass documentation test

a346206

mik-laj added the provider:amazon-aws AWS/Amazon - related issues label Oct 25, 2019

indendation comment

2a5527c

JavierLTAtr added 2 commits October 28, 2019 11:09

Delete table as file name in RedshiftToS3Transfer

9f49f0c

Solving errors More errors Trying to pass documentation test indendation comment Delete table as file name in RedshiftToS3Transfer

Merge branch 'master' of https://github.com/JavierLopezT/airflow

5237bc1

JavierLTAtr added 3 commits November 11, 2019 15:46

update test

076dafe

Pylint corrections

edf2ae9

whitespaces correction

f1a31e5

kaxil approved these changes Nov 12, 2019

View reviewed changes

ashb requested changes Nov 12, 2019

View reviewed changes

airflow/operators/redshift_to_s3_operator.py Outdated Show resolved Hide resolved

airflow/operators/redshift_to_s3_operator.py Outdated Show resolved Hide resolved

JavierLopezT and others added 6 commits November 13, 2019 12:11

Update airflow/operators/redshift_to_s3_operator.py

9f4d8e5

Co-Authored-By: Ash Berlin-Taylor <[email protected]>

simplification

b8a1eaf

updating test

e1fdcf2

updating test

bf6e4ee

requested change

90776fc

removing unused argument 'table'

f48314a

JavierLopezT requested a review from ashb November 14, 2019 11:02

ashb requested changes Nov 14, 2019

View reviewed changes

tests/operators/test_redshift_to_s3_operator.py Outdated Show resolved Hide resolved

JavierLTAtr added 4 commits November 14, 2019 16:57

requested changes

59219c7

removing whitespace in blank line

6076d53

deleting blank lines at the end

9dab845

add blank line at the end

ee2156e

kaxil reviewed Nov 15, 2019

View reviewed changes

JavierLTAtr added 3 commits November 20, 2019 12:38

adding parameterized test

4cc3c4a

solving errors

ae69ec7

isort correction

23b726d

kaxil reviewed Nov 20, 2019

View reviewed changes

kaxil review, additional commas

872f474

kaxil reviewed Nov 20, 2019

View reviewed changes

changing to parameterized.expand

0bdf7a0

kaxil reviewed Nov 21, 2019

View reviewed changes

JavierLTAtr added 2 commits November 22, 2019 10:01

kaxil review

6b56239

delete table argument

b157161

JavierLopezT requested a review from kaxil November 25, 2019 09:11

kaxil reviewed Nov 25, 2019

View reviewed changes

changing line parameterized.expand

297f06f

kaxil merged commit 4a17bca into apache:master Nov 26, 2019

kaxil pushed a commit that referenced this pull request Dec 18, 2019

[AIRFLOW-5726] Allow custom filename in RedshiftToS3Transfer (#6396)

20e022e

(cherry-picked from 4a17bca)

ashb pushed a commit that referenced this pull request Dec 19, 2019

[AIRFLOW-5726] Allow custom filename in RedshiftToS3Transfer (#6396)

31645d1

(cherry-picked from 4a17bca)

JavierLopezT mentioned this pull request Jan 7, 2020

Remove {table} line 94 #7023

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-5726] Delete table as file name in RedshiftToS3Transfer #6396

[AIRFLOW-5726] Delete table as file name in RedshiftToS3Transfer #6396

JavierLopezT commented Oct 23, 2019 •

edited

Loading

JavierLopezT commented Oct 24, 2019 •

edited

Loading

OmerJog commented Oct 27, 2019

codecov-io commented Oct 28, 2019 •

edited

Loading

JavierLopezT commented Oct 28, 2019

kaxil commented Oct 28, 2019

JavierLopezT commented Oct 30, 2019

kaxil commented Oct 30, 2019

kaxil commented Oct 30, 2019

JavierLopezT commented Nov 12, 2019

kaxil left a comment

ashb left a comment

OmerJog commented Nov 15, 2019

kaxil Nov 15, 2019

kaxil Nov 15, 2019

kaxil Nov 20, 2019

kaxil Nov 20, 2019

kaxil Nov 20, 2019

JavierLopezT Nov 21, 2019

kaxil Nov 21, 2019 •

edited

Loading

kaxil commented Nov 21, 2019 •

edited

Loading

kaxil Nov 25, 2019 •

edited

Loading

kaxil Nov 25, 2019 •

edited

Loading

kaxil Nov 25, 2019

	@parameterized([(True), (False)])
	@parameterized([(True, ), (False, )])

	@parameterized([(True, ), (False, )])
	@parameterized.expand([(True, ), (False, )])

[AIRFLOW-5726] Delete table as file name in RedshiftToS3Transfer #6396

[AIRFLOW-5726] Delete table as file name in RedshiftToS3Transfer #6396

Conversation

JavierLopezT commented Oct 23, 2019 • edited Loading

Jira

Description

Tests

Commits

Documentation

JavierLopezT commented Oct 24, 2019 • edited Loading

OmerJog commented Oct 27, 2019

codecov-io commented Oct 28, 2019 • edited Loading

Codecov Report

JavierLopezT commented Oct 28, 2019

kaxil commented Oct 28, 2019

JavierLopezT commented Oct 30, 2019

kaxil commented Oct 30, 2019

kaxil commented Oct 30, 2019

JavierLopezT commented Nov 12, 2019

kaxil left a comment

Choose a reason for hiding this comment

ashb left a comment

Choose a reason for hiding this comment

OmerJog commented Nov 15, 2019

kaxil Nov 15, 2019

Choose a reason for hiding this comment

kaxil Nov 15, 2019

Choose a reason for hiding this comment

kaxil Nov 20, 2019

Choose a reason for hiding this comment

kaxil Nov 20, 2019

Choose a reason for hiding this comment

kaxil Nov 20, 2019

Choose a reason for hiding this comment

JavierLopezT Nov 21, 2019

Choose a reason for hiding this comment

kaxil Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

kaxil commented Nov 21, 2019 • edited Loading

kaxil Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

kaxil Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

kaxil Nov 25, 2019

Choose a reason for hiding this comment

JavierLopezT commented Oct 23, 2019 •

edited

Loading

JavierLopezT commented Oct 24, 2019 •

edited

Loading

codecov-io commented Oct 28, 2019 •

edited

Loading

kaxil Nov 21, 2019 •

edited

Loading

kaxil commented Nov 21, 2019 •

edited

Loading

kaxil Nov 25, 2019 •

edited

Loading

kaxil Nov 25, 2019 •

edited

Loading