Update copy command for s3 to redshift

[sunki-hong]

Update copy command for s3_to_redshift to be able to select column names while copying files from s3 to redshift.

COPY tablename (column1 [,column2, ...]) 

Related aws documentation (Link),

---

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

[mik-laj]

Can you copy the result of this function to the test?

[sunki-hong]

Hi @mik-laj :) Thanks for the review.

Could you explain it in more detail?

Do you want the result of op._build_copy_query to be asserted with expected plain copy text?
I thought that line 105 assert_equal_ignore_multiple_spaces(self, mock_run.call_args[0][0], copy_query) checks the result

FYI expected copy_query looks like this

COPY schema.table (column_1, column_2)
FROM 's3://bucket/key'
with credentials
'aws_access_key_id=aws_access_key_id;aws_secret_access_key=aws_secret_access_key'
;

[mik-laj]

FYI expected copy_query looks like this

Can you add it to tests?

[sunki-hong]

Oh then like this?

Suggested change

	copy_query = op._build_copy_query(credentials_block, copy_options)
	copy_query = op._build_copy_query(credentials_block, copy_options)
	expected_copy_query = '''
	COPY schema.table (column_1, column_2)
	FROM 's3://bucket/key'
	with credentials
	'aws_access_key_id=aws_access_key_id;aws_secret_access_key=aws_secret_access_key'
	;
	'''
	assert_equal_ignore_multiple_spaces(self, expected_copy_query, copy_query)

[mik-laj]

Exactly. Building this SQL is crucial in integrating with a thirty-patty service, so it should show up in tests.

[mik-laj]

Can you add tests for SQL without columns also?

[sunki-hong]

Implemented !

SQL without columns
https://github.com/apache/airflow/blob/d1dc18d9c84c2b3d1f3005a69f93a1adc59476b8/tests/providers/amazon/aws/transfers/test_s3_to_redshift.py#L62-L69

SQL with columns
https://github.com/apache/airflow/blob/d1dc18d9c84c2b3d1f3005a69f93a1adc59476b8/tests/providers/amazon/aws/transfers/test_s3_to_redshift.py#L109-L116

[mik-laj]

here we have access to the query, so we can drop of invoking private methods and compare this value. Thus, this test will still check implementations, but will depend less on the internal details.

[sunki-hong]

Thanks for the comment @mik-laj ;)
So would it be nicer to check execution with plain text rather than invoking private method op._build_copy_query like this?

From checking execution by invoking private method

        copy_query = op._build_copy_query(credentials_block, copy_options)
        expected_copy_query = '''
                                COPY schema.table
                                FROM 's3://bucket/key'
                                with credentials
                                'aws_access_key_id=aws_access_key_id;aws_secret_access_key=aws_secret_access_key'
                                ;
                             '''
        assert_equal_ignore_multiple_spaces(self, expected_copy_query, copy_query)

        assert mock_run.call_count == 1
        assert access_key in copy_query
        assert secret_key in copy_query
        assert_equal_ignore_multiple_spaces(self, mock_run.call_args[0][0], copy_query)

To checking execution by plain text

        copy_query = '''
                        COPY schema.table
                        FROM 's3://bucket/key'
                        with credentials
                        'aws_access_key_id=aws_access_key_id;aws_secret_access_key=aws_secret_access_key'
                        ;
                     '''
        assert mock_run.call_count == 1
        assert_equal_ignore_multiple_spaces(self, mock_run.call_args[0][0], copy_query)

[mik-laj]

Yes. Exactly. We should only check the places where the class is connecting to another class, not its internal details.

[sunki-hong]

Done :)
Changed all copy_query to plain text
Thanks for all the comment @mik-laj

[sunki-hong]

@mik-laj Hi, can you take a look and check if it is okay to merge?